test-cache: Fix XDG tests, make tests more consistent #880
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The tests on Linux were relying on XDG_CACHE_HOME being set to its fallback value of ∼/.cache, which made them fail on systems with custom values for XDG_CACHE_HOME. To account for this, we take the cue from the Windows tests and set XDG_CACHE_HOME to a known, custom, value. This incidentally also tests that our logic can account for custom values of XDG_CACHE_HOME, so this doesn't need its own test. The value of "/tmp/home/.cache" was chosen as a cross between the Windows value of "/tmp/AppData/Local" and the fallback "∼/.cache" Fixes: pypa#814
Several changes to the tests in test_cache.py to make them more consistent with each other - test_get_cache_dir: Follow all other tests in not casting to posix paths in checking paths are as expected, instead checking an equal Path object is roundtripped. Also, run the test simulating all platform's path logic, like the other tests. - test_get_cache_dir_old_pip: Remove logic checking whether _get_cache_dir accepts explicitly-set paths. It is duplicated from test_get_cache_dir, doesn't appear to be exercising any new codepath, and it is unclear why this case needs extra exercise - test_get_pip_cache, test_get_cache_dir_old_pip: follow the lead of test_get_cache_dir_pip_disabled_in_environment and inline the call to _get_cache_dir
|
OK, removed RFC markers from the commit messages. I'd appreciate you running the new |
Signed-off-by: William Woodruff <william@trailofbits.com>
|
Sorry for the delay here! I've added a Windows CI job, and it looks like it indeed needs some tweaks for actual Windows hosts. If you're able to make those tweaks then please do; otherwise I'll push some fixes here when I have the free time. |
|
Good that you added a windows runner - these seem to have been broken even without my changes - but I have no idea at first glance why it's failing. I don't have the time to investigate right now, but I'll see if I can sometime next week.
27 feb 2025 17:58:31 William Woodruff ***@***.***>:
…
[Imagen][woodruffw][https://avatars.githubusercontent.com/u/3059210?s=20&v=4]*woodruffw* left a comment (pypa/pip-audit#880)[#880 (comment)]
Sorry for the delay here! I've added a Windows CI job, and it looks like it indeed needs some tweaks for actual Windows hosts. If you're able to make those tweaks then please do; otherwise I'll push some fixes here when I have the free time.
—
Reply to this email directly, view it on GitHub[#880 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAJTW5JPOWUOXRAUTU6ARIL2R4Y2NAVCNFSM6AAAAABW2YREL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOBYGM4TENRXGQ].
You are receiving this because you were mentioned.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solving the issues I raised in #814 (comment), this commit set (at least on my machine) fixes the tests on Linux systems with custom XDG variables.
While I was there, I noticed the tests were a little inconsistent, so this PR also contains a couple of commits addressing that. Three of the changes I'm unsure about, so I'd appreciate a second look at them before this is merged. (I've labeled this as a draft until the two RFCs below are resolved, since even if they are accepted their commit messages need to be reworded in light of that)
fix(test_cache): Correct XDG testing logic
The tests on Linux were relying on XDG_CACHE_HOME being set to its fallback value of
∼/.cache, which made them fail on systems with custom values forXDG_CACHE_HOME.To account for this, we take the cue from the Windows tests and set
XDG_CACHE_HOMEto a known, custom, value. This incidentally also tests that our logic can account for custom values ofXDG_CACHE_HOME, so this doesn't need its own test.The value of
/tmp/home/.cachewas chosen as a cross between the Windows value of/tmp/AppData/Localand the fallback∼/.cache(This commit is a MVP for the PR, the others can be ignored if desired)
Fixes: More respectful convention for caching #814
RFC: fix(test_cache): Make tests more consistent
test_get_cache_dir: Follow all other tests in not casting to posix paths in checking paths are as expected, instead checking an equal Path object is roundtripped. NOTE: This edit I'm least confident in, not least since I don't have a Windows machine available to test this on.test_get_cache_dir_old_pip: Remove logic checking whether _get_cache_dir accepts explicitly-set paths. It is duplicated fromtest_get_cache_dir, doesn't appear to be exercising any new codepath, and it is unclear why this case needs extra exercisetest_get_pip_cache,test_get_cache_dir_old_pip: follow the lead oftest_get_cache_dir_pip_disabled_in_environmentand inline the call to_get_cache_dirRFC: fix(test_cache): Cross-OS test_get_cache_dir
In view of my doubts above in re whether I broke
test_get_cache_diron Windows, run the test simulating all platforms. However, I am unsure that my understanding ofpathliband the relevant monkeypatching is correct, or if this commit is even needed.