-
Notifications
You must be signed in to change notification settings - Fork 908
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
(#3231) Don't refresh local package info during upgrade no-ops #3248
Conversation
I'm not sure if this is the best implementation of this general idea, I'd welcome a second look at this. If it does look ok, then I should be able to get a test set up for it. |
Probably one test to check that a dependency upgrade gets detected/refreshed, and one test to check that a dependency install (via an upgrade) gets detected/refreshed, for when the parent package gets upgraded second. |
0ee9377
to
853e26a
Compare
@TheCakeIsNaOH I've force pushed to your branch after updating to the head of the develop branch. I've also removed that extra empty line from your commit. If you have an opportunity to take a look at the tests I added, it'd be appreciated. I have run the new integration tests with a revert commit removing the fix, and the added test for the count of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for getting tests added @corbob, they look good. |
This optimizes the flow of the upgrade command. First, it uses the list of locally installed package from the if all is specified check, instead of throwing that away. Then, it will re-use the list for each package upgrade attempt, only refreshing the list when a package upgrade/install is attempted. This speeds up upgrade all runs, especially when a large (200+) number of packages are installed.
Add integration tests that ensure we don't run the local package listing more than once per upgraded package.
853e26a
to
cd92db1
Compare
Thanks for getting this fixed up @TheCakeIsNaOH 👍 |
Description Of Changes
This optimizes the flow of the upgrade command. First, it uses the
list of locally installed package from the if all is specified check,
instead of throwing that away. Then, it will re-use the list for each
package upgrade attempt, only refreshing the list when a package
upgrade/install is attempted. This speeds up upgrade all runs,
especially when a large (200+) number of packages are installed.
Motivation and Context
Upgrades on my machine are really slow as compared with their speed on v1.4.0
Testing
packages.config
file:.\choco.exe install .\packages.config --skip-powershell
(Note, only an internal source will work for this, otherwise it will quickly hit the rate limits on CCR)Measure-Command { .\choco.exe upgrade all --ignore-http-cache --source=https://community.chocolatey.org/api/v2 --skip-powershell }
Measure-Command { .\choco.exe upgrade all --ignore-http-cache --source=https://community.chocolatey.org/api/v2 --skip-powershell }
againOperating Systems Testing
Change Types Made
Change Checklist
Related Issue
Fixes #3231