- Notifications
You must be signed in to change notification settings - Fork675
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
JohnVillalovos commentedJul 23, 2022
PR#2174 demonstrates the issue. |
Use `monkeypatch` to ensure that no config files are reported for thetest.Closes:#2172
34d44aa to864fc12Compare
nejch left a comment
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 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_envfixture 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.
JohnVillalovos commentedJul 23, 2022
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. |
nejch commentedJul 23, 2022
Sounds good also! I'll merge this for now. |
Use
monkeypatchto ensure that no config files are reported for thetest.
Closes:#2172