Use request.addfinalizer instead of the yield based approach in integ tests #2089
Conversation
Signed-off-by: Achal Shah <achals@gmail.com>
achals
requested review from
adchia,
felixwang9817,
tsotnet,
woop and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #2089 +/- ##
===========================================
- Coverage 83.44% 58.01% -25.44%
===========================================
Files 100 100
Lines 8077 8088 +11
===========================================
- Hits 6740 4692 -2048
- Misses 1337 3396 +2059
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
sdk/python/tests/conftest.py
Outdated
| def environment(request): | ||
| with construct_test_environment(request.param) as e: | ||
| yield e | ||
| def environment(request, tmp_path_factory, worker_id): |
There was a problem hiding this comment.
why is tmp_path_factory needed here? doesn't look like it's being used
There was a problem hiding this comment.
Ah yeah. Using pytest-xdist, each worker creates its own version of fixtures. I had a version of this PR that creates the fixture in a single worker and shares it across workers by pickling it, but that won't work because when a single fixture is used across workers, there's data corruption (because multiple processes write to the same registry location or sqlite online store).
tmp_path_factory is a holdover from there. Removing.
sdk/python/tests/conftest.py
Outdated
| with construct_test_environment(request.param) as e: | ||
| yield e | ||
| def environment(request, tmp_path_factory, worker_id): | ||
| logger.info("Worker: %s, Request: %s", worker_id, request.param) |
There was a problem hiding this comment.
should this be debug level?
sdk/python/tests/conftest.py
Outdated
| e = construct_test_environment(IntegrationTestRepoConfig(online_store=REDIS_CONFIG)) | ||
|
|
||
| def cleanup(): | ||
| logger.info("Running cleanup in %s, Request: %s", worker_id, request.param) |
There was a problem hiding this comment.
what is request.param supposed to represent here?
There was a problem hiding this comment.
request.param refers to the config used to create the environment, from params=FULL_REPO_CONFIGS
sdk/python/tests/conftest.py
Outdated
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def universal_data_sources(environment): | ||
| def universal_data_sources(request, environment, tmp_path_factory, worker_id): |
There was a problem hiding this comment.
again, why do we have tmp_path_factory and worker_id here?
Signed-off-by: Achal Shah <achals@gmail.com>
Signed-off-by: Achal Shah <achals@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, felixwang9817 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| def cleanup(): | ||
| e.feature_store.teardown() | ||
|
|
||
| request.addfinalizer(cleanup) |
There was a problem hiding this comment.
but isn't this code equivalent to the previous version with context managers?
I believe pytest under the hook does the same addfinalizer, https://github.com/pytest-dev/pytest/blob/main/src/_pytest/fixtures.py#L921
Signed-off-by: Achal Shah achals@gmail.com
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: