-
Notifications
You must be signed in to change notification settings - Fork 994
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
Use request.addfinalizer instead of the yield based approach in integ tests #2089
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
import logging | ||
import multiprocessing | ||
from datetime import datetime, timedelta | ||
from sys import platform | ||
|
@@ -34,6 +35,8 @@ | |
construct_universal_entities, | ||
) | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def pytest_configure(config): | ||
if platform in ["darwin", "windows"]: | ||
|
@@ -137,21 +140,33 @@ def simple_dataset_2() -> pd.DataFrame: | |
@pytest.fixture( | ||
params=FULL_REPO_CONFIGS, scope="session", ids=[str(c) for c in FULL_REPO_CONFIGS] | ||
) | ||
def environment(request): | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be debug level? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah |
||
e = construct_test_environment(request.param) | ||
|
||
def cleanup(): | ||
logger.info("Running cleanup in %s, Request: %s", worker_id, request.param) | ||
e.feature_store.teardown() | ||
|
||
request.addfinalizer(cleanup) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but isn't this code equivalent to the previous version with context managers? |
||
return e | ||
|
||
|
||
@pytest.fixture() | ||
def local_redis_environment(): | ||
with construct_test_environment( | ||
IntegrationTestRepoConfig(online_store=REDIS_CONFIG) | ||
) as e: | ||
yield e | ||
def local_redis_environment(request, worker_id): | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
e.feature_store.teardown() | ||
|
||
request.addfinalizer(cleanup) | ||
return e | ||
|
||
|
||
@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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, why do we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't. Removing. |
||
entities = construct_universal_entities() | ||
datasets = construct_universal_datasets( | ||
entities, environment.start_date, environment.end_date | ||
|
@@ -160,18 +175,24 @@ def universal_data_sources(environment): | |
datasets, environment.data_source_creator | ||
) | ||
|
||
yield entities, datasets, datasources | ||
def cleanup(): | ||
environment.data_source_creator.teardown() | ||
|
||
environment.data_source_creator.teardown() | ||
request.addfinalizer(cleanup) | ||
|
||
return entities, datasets, datasources | ||
|
||
|
||
@pytest.fixture(scope="session") | ||
def e2e_data_sources(environment: Environment): | ||
def e2e_data_sources(environment: Environment, request): | ||
df = create_dataset() | ||
data_source = environment.data_source_creator.create_data_source( | ||
df, environment.feature_store.project, field_mapping={"ts_1": "ts"}, | ||
) | ||
|
||
yield df, data_source | ||
def cleanup(): | ||
environment.data_source_creator.teardown() | ||
|
||
request.addfinalizer(cleanup) | ||
|
||
environment.data_source_creator.teardown() | ||
return df, data_source |
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.
why is
tmp_path_factory
needed here? doesn't look like it's being usedThere 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.
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.