-
-
Notifications
You must be signed in to change notification settings - Fork 614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add --cache-dir option to pip-compile #1022
Add --cache-dir option to pip-compile #1022
Conversation
bee73ac
to
d2164de
Compare
Codecov Report
@@ Coverage Diff @@
## master #1022 +/- ##
==========================================
- Coverage 99.11% 99.11% -0.01%
==========================================
Files 34 34
Lines 2364 2360 -4
Branches 303 302 -1
==========================================
- Hits 2343 2339 -4
Misses 11 11
Partials 10 10
Continue to review full report at Codecov.
|
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.
Hello @richafrank,
Thanks for bringing this up! I like the intention. Is there a way to isolate the DependencyCache.cache_dir
explicitly for each test, rather than clear the cache? The workaround with clearing works, but if we want to run tests in parallel someday the isolation would help. The cache directory could be configured via an environment variable (e.g. PIP_TOOLS_CACHE_DIR
). Also, this could be useful for #395. Please, let me know what you think.
Thanks @atugushev . I can definitely make it more explicit! Since this is manifesting in the cli tests, my recommendation would be to add a Adding a cli arg in addition to the env var would mean more transparency and give more flexibility, allowing tests to run in parallel in the same process even. But that's more to change, so curious your thoughts. |
(I updated this branch with my suggestion from above, if that sounds good.) |
Unfortunately, as long as we have a default cache directory, one could easily add a test that relies on it unintentionally. That would be a good reason to add back the autouse |
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.
Thank you for working on it! I'd prefer to use an auto-used fixture wich sets the PIP_TOOLS_CACHE_DIR to a temporary directory globally. This should decrease the patch size, see the details below.
Thanks @atugushev . Sounds like I went a bit too explicit! Your suggestions look good to me. |
21e34ce
to
4aa8742
Compare
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.
Great! Thanks for accepting my suggestions. Left a few minor comments below:
Adds a --cache-dir arg to the compile cli and uses it for test isolation via monkeypatching of consumed env var
4aa8742
to
738ab48
Compare
👍 on the cleanup |
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.
I've looked deeply at the PR and found that cache dir is also using in PyPIRepository class. See the details below.
so instances don't share state across tests
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.
Awesome! 👏👏👏
@richafrank thanks for the contribution! |
Thanks @atugushev ! |
|
Ensure tests pass without dependencies cached from previous tests.
Running tests individually, I was seeing intermittent failures depending on whether the cache was populated or not, specifically
test_stdin
andtest_filter_pip_markers
. Also I refactored a test to use an existing fixture.Changelog-friendly one-liner: Add
--cache-dir
option topip-compile
.Contributor checklist