-
Notifications
You must be signed in to change notification settings - Fork 146
avoid using actual user configurations during tests #864
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
avoid using actual user configurations during tests #864
Conversation
@pdgendt We could also create some invalid user config in the CI system (at the default path) to ensure that this corrupt config is never used in CI. Otherwise tests could potentially touch real configs again in future (e.g. when they use |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
==========================================
- Coverage 84.52% 84.34% -0.18%
==========================================
Files 11 11
Lines 3366 3366
==========================================
- Hits 2845 2839 -6
- Misses 521 527 +6 |
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.
Thanks for the fast fix! This looks good IMO.
I like the idea, would even be better if we could verify that locally before CI, but not sure if that's easy. |
13052d0
to
0ed2765
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.
Some styling fixes
I have added this new method |
0ed2765
to
0dfd479
Compare
I will rebase once your PR #865 is merged and run |
Update this PR instead, so we can go ahead with this one. I'm afraid the formatting PR will need some more eyes and time. |
0dfd479
to
476f4ae
Compare
I haven't looked at the code yet but it reminded me 6cf0a02... was this lost in the transition to |
I don't think so, it just wasn't noticed when new tests were added. |
Can confirm this issue exists in v1.5.0 running with |
Let's tackle this in a follow-up PR 👍 Once a second reviewer has approved, this PR can be merged, correct? |
476f4ae
to
c565af6
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.
Pull Request Overview
This PR introduces test isolation for west configuration files to prevent tests from interfering with user's actual global and system configuration. The main change adds an autouse pytest fixture that sets environment variables to redirect config file locations during test execution.
Key changes:
- Added autouse fixture to isolate global and system config files during tests
- Refactored existing config test fixtures to use new context managers
- Introduced reusable
update_env
andchdir
context managers
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
tests/conftest.py | Added autouse fixture for config isolation and utility context managers |
tests/test_config.py | Updated to use new update_env context manager |
tests/test_manifest.py | Refactored to use update_env instead of manual environment handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
c565af6
to
99c1f84
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
99c1f84
to
2f34090
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@mbolivar PTAL, as you're the original author of the environment mechanisms in tests. I think the fix is elegant and more Pythonic. |
2f34090
to
3bdab91
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.
I had a quick look and spotted nothing wrong, thanks for the fix.
Add a new automatically applied pytest fixture to prevent tests from using or modifying the user's actual global or system configuration. To support this, two context manager helpers are added: - `update_env(env)` — temporarily updates environment variables within a `with` block - `chdir(path)` — temporarily changes the current working directory within a `with` block
3bdab91
to
aa0b156
Compare
Fixes #862
A new pytest fixture is added which is automatically applied (
autouse=True
).This fixture sets the west environment variables
WEST_CONFIG_SYSTEM
andWEST_CONFIG_GLOBAL
during each test, in order to prevent it from using or modifying the user's actual global or system configuration.Note: The environment variables are inherited from
subprocess
, so that also thesubprocess
calls do not use or touch the user's real configs.