Clean up import side effects in tests #6241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Various tests were relying on the side effects of tests that nosetest runs before they ran. These include: - 3 in test_action_alias_utils.py::TestInjectImmutableParameters - 2 in test_jinja_render_data_filters.py - 1 in test_logging_middleware.py - 9 in test_operators.py::SearchOperatorTest - 3 in test_util_payload.py In particular, the oslo config initialization from the tests in st2common/tests/unit/services/ happens before these test ran, obscuring their dependence on this initialization. Pants runs each test file separately for fine-grained caching.
This looks like copy pasta as some of these files have a comment saying that running this before importing something else is required. However, by importing st2tests, that already implicitly happens in st2tests/st2tests/base.py. Then tests_config.parse_args() gets called again in the class init. Plus, I reviewed all the other imports, and none of them have import time side effects that matter for oslo config bits. So, these calls are not necessary, and the comments about them are wrong.
pants runs each test file separately. test_workflow_rerun only worked under nosetest because earlier files already did the monkey_patch. Without this, running this file in isolation, with either nosetest or pytest, hangs.
…ffects importing anything form st2tests already handles running st2tests.config.parse_args() on import before loading the files from st2common that need those side effects. So, rely on that, and on the db test case base classes for running parse_args() where appropriate. The import side-effects are unfortunate, but this reduces how many places are making those changes.
…fects importing anything form st2tests already handles running st2tests.config.parse_args() on import before loading the files from st2common that need those side effects. So, rely on that, and on the db test case base classes for running parse_args() where appropriate. The import side-effects are unfortunate, but this reduces how many places are making those changes.
cognifloyd
requested review from
winem,
nzlosh,
mamercad,
rush-skills,
guzzijones,
amanda11 and
a team
pull-request-size
bot
added
the
size/L
amanda11
approved these changes
Sep 16, 2024
nzlosh
approved these changes
Sep 16, 2024
This was referenced Sep 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When pants runs tests via pytest, it runs each file separately to facilitate fine-grained caching of test results (that can be invalidated based on the inferred deps of that test file). This revealed a variety of areas where tests rely on import-time side effects that happen when previous tests ran. So, this cleans up those side effects so that each test file can run in isolation.
In some cases, I was able to move the side effects from import-time to
setUpClasstime which feels cleaner to me.As I identified imports with side-effects, I reviewed other places they were imported to consistently import them early with a comment explaining the dependency on import side-effects.
These commits were extracted from my wip work in #6202.