- Notifications
You must be signed in to change notification settings - Fork673
test: fix broken test if user had config files#2173
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
PR#2174 demonstrates the issue. |
Use `monkeypatch` to ensure that no config files are reported for thetest.Closes:#2172
34d44aa
to864fc12
CompareThere 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 catch@JohnVillalovos! We already have some scaffolding for mocking/prepping a clean config environment but perhaps it was not broad enough in where we placed it. It seems we should have a clean environment forall tests by adding that to the roottests/conftest.py
e.g.:
Move this
mock_clean_env
fixture to the root conftest withautouse=True
- no testever should break from a user-suppliedPYTHON_GITLAB_CFG
:python-gitlab/tests/unit/test_config.py
Lines 132 to 134 inf0152dc
@pytest.fixture defmock_clean_env(monkeypatch): monkeypatch.delenv("PYTHON_GITLAB_CFG",raising=False) Move more logic to that fixture that we already use to ensure an empty list of default files - same here, no testever should be affected by presence of local files:
python-gitlab/tests/unit/test_config.py
Lines 144 to 145 inf0152dc
withmonkeypatch.context()asm: m.setattr(config,"_DEFAULT_FILES", []) In any test that does specifically need that, override it again if needed (I think we mostly don't, as we explicitly define things) like we already do here:
python-gitlab/tests/unit/test_config.py
Line 138 inf0152dc
monkeypatch.setenv("PYTHON_GITLAB_CFG","/some/path")
More approaches are discussed herehttps://stackoverflow.com/questions/38748257/disable-autouse-fixtures-on-specific-pytest-marks in case the simple override is not enough.
WDYT? This would then also be more local/CI-agnostic and consistent across all runs and we can stop worrying about config even before we hit CI.
That sounds reasonable but I can't help but think this should be merged now and the improvements you mentioned be done afterwards. As at the moment tests are broken for anyone who has a config. I was going to work on arrays tomorrow and then could try to take a stab at your ideas. |
Sounds good also! I'll merge this for now. |
Use
monkeypatch
to ensure that no config files are reported for thetest.
Closes:#2172