Skip to content
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

isolate settings context before first test #158

Merged

Conversation

rababerladuseladim
Copy link
Contributor

@rababerladuseladim rababerladuseladim commented Apr 16, 2024

PR Context

  • blocked by: feature/mx-1502 wikidata results refactor mex-extractors#39 as testing is only possible after the mex-common dependency in mex-extractors is updated to v0.24.0
  • Bug: the first test that is executed does not receive isolated settings. It therefore receives the production data as they are in mex-assets. If the test uses data from mex-assets, it will get a different result than if it is not executed first.
  • Reproduce: in mex-extractors, execute pdm run pytest .\tests\sumo\test_extract.py::test_extract_cc1_data_model_nokeda
    • passes if not executed as first test
    • fails if executed in isolation / as first test

Fixed

  • isolate settings context before first test

Copy link
Contributor

@erichesse erichesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm the bug, I couldn't confirm the fix. when I install mex-extractors witht "mex-common@git+https://github.com/robert-koch-institut/[email protected]@hotfix/isolate-settings-context-before-first-test" in pyproject.toml, the error seems to persist. How did you test the fix?

Also, should there also be a hotfix version be bumped for mex-common?

@rababerladuseladim
Copy link
Contributor Author

I bumped the version and will create a new release once this is merged into main.

Regarding testing, I do not really know how to address this. Running the reproduction step on main of mex-extractors will results in this bug. Updating mex-common to 0.24.0 introduces a whole lot of breaking changes, which Kamran is currently addressing in his PR. Hence, testing this is only possible after Kamran finishes his PR. I tested this manually by applying the changed to the installed version of mex-common in the mex-extractors venv.

Copy link
Contributor

@erichesse erichesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test these changes, after tests are fixed in /pull/157 and PR is merged

…efore-first-test

# Conflicts:
#	CHANGELOG.md
#	pyproject.toml
@rababerladuseladim
Copy link
Contributor Author

@erichesse Could you review again? I created a branch using this version of mex-common in mex-extractors for testing: robert-koch-institut/mex-extractors#68

Copy link
Contributor

@erichesse erichesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Looks good now

@rababerladuseladim rababerladuseladim merged commit fc7ce4b into main May 21, 2024
7 checks passed
@rababerladuseladim rababerladuseladim deleted the hotfix/isolate-settings-context-before-first-test branch May 21, 2024 12:15
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.

2 participants