Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 23, 2022
Merged

Conversation

JohnVillalovos
Copy link
Member

Use monkeypatch to ensure that no config files are reported for the
test.

Closes: #2172

@JohnVillalovos JohnVillalovos requested a review from nejch July 23, 2022 18:30
@JohnVillalovos JohnVillalovos marked this pull request as draft July 23, 2022 19:28
@JohnVillalovos JohnVillalovos marked this pull request as ready for review July 23, 2022 19:34
@JohnVillalovos
Copy link
Member Author

PR #2174 demonstrates the issue.

Use `monkeypatch` to ensure that no config files are reported for the
test.

Closes: #2172
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/config_test_fix branch from 34d44aa to 864fc12 Compare July 23, 2022 21:34
Copy link
Member

@nejch nejch left a 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 for all tests by adding that to the root tests/conftest.py e.g.:

  1. Move this mock_clean_env fixture to the root conftest with autouse=True - no test ever should break from a user-supplied PYTHON_GITLAB_CFG:

    @pytest.fixture
    def mock_clean_env(monkeypatch):
    monkeypatch.delenv("PYTHON_GITLAB_CFG", raising=False)

  2. Move more logic to that fixture that we already use to ensure an empty list of default files - same here, no test ever should be affected by presence of local files:

    with monkeypatch.context() as m:
    m.setattr(config, "_DEFAULT_FILES", [])

  3. 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:

    monkeypatch.setenv("PYTHON_GITLAB_CFG", "/some/path")

More approaches are discussed here https://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
Copy link
Member Author

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
Copy link
Member

nejch commented Jul 23, 2022

Sounds good also! I'll merge this for now.

@nejch nejch merged commit 1ecbc7c into main Jul 23, 2022
@nejch nejch deleted the jlvillal/config_test_fix branch July 23, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit tests are failing locally
2 participants