Skip to content

chore: Change pytest fixtures to be function-scoped instead of session-scoped #2899

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.

Sign up for GitHub

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
feast-ci-bot merged 5 commits into from
Jul 5, 2022
Merged

chore: Change pytest fixtures to be function-scoped instead of session-scoped #2899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3717bdd
Test to trigger registry conflicts
felixwang9817 Jun 30, 2022
4523963
Switch environment and associated fixtures to being function scoped
felixwang9817 Jul 1, 2022
0b7c3d8
Format
felixwang9817 Jul 1, 2022
9e27fce
Switch type tests to function-scoped fixtures
felixwang9817 Jul 1, 2022
69b5352
Format
felixwang9817 Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions sdk/python/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def start_test_local_server(repo_path: str, port: int):
fs.serve("localhost", port, no_access_log=True)


@pytest.fixture(scope="session")
@pytest.fixture
def environment(request, worker_id):
e = construct_test_environment(
request.param, worker_id=worker_id, fixture_request=request
Expand Down Expand Up @@ -293,7 +293,7 @@ def pytest_generate_tests(metafunc: pytest.Metafunc):
)


@pytest.fixture(scope="session")
@pytest.fixture
def feature_server_endpoint(environment):
if (
not environment.python_feature_server
Expand Down Expand Up @@ -344,12 +344,12 @@ def _free_port():
return sock.getsockname()[1]


@pytest.fixture(scope="session")
@pytest.fixture
def universal_data_sources(environment) -> TestData:
return construct_universal_test_data(environment)


@pytest.fixture(scope="session")
@pytest.fixture
def e2e_data_sources(environment: Environment):
df = create_dataset()
data_source = environment.data_source_creator.create_data_source(
Expand Down
2 changes: 1 addition & 1 deletion sdk/python/tests/integration/e2e/test_go_feature_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
)


@pytest.fixture(scope="session")
@pytest.fixture
def initialized_registry(environment, universal_data_sources):
fs = environment.feature_store

Expand Down
8 changes: 2 additions & 6 deletions sdk/python/tests/integration/registration/test_universal_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ class TypeTestConfig:


@pytest.fixture(
params=OFFLINE_TYPE_TEST_CONFIGS,
scope="session",
ids=[str(c) for c in OFFLINE_TYPE_TEST_CONFIGS],
params=OFFLINE_TYPE_TEST_CONFIGS, ids=[str(c) for c in OFFLINE_TYPE_TEST_CONFIGS],
)
def offline_types_test_fixtures(request, environment):
config: TypeTestConfig = request.param
Expand All @@ -80,9 +78,7 @@ def offline_types_test_fixtures(request, environment):


@pytest.fixture(
params=ONLINE_TYPE_TEST_CONFIGS,
scope="session",
ids=[str(c) for c in ONLINE_TYPE_TEST_CONFIGS],
params=ONLINE_TYPE_TEST_CONFIGS, ids=[str(c) for c in ONLINE_TYPE_TEST_CONFIGS],
)
def online_types_test_fixtures(request, environment):
return get_fixtures(request, environment)
Expand Down
23 changes: 23 additions & 0 deletions sdk/python/tests/unit/test_registry_conflict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import pytest

from feast.entity import Entity


@pytest.mark.integration
def test_apply_first_entity(environment):
Copy link
Member

@achals achals Jul 2, 2022

Choose a reason for hiding this comment

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

If the Scopes have been changed i don't see a huge point to these tests - even if there's a race condition in the future it seems like the repro for that will need it's own test. Wdyt?

Copy link
Collaborator Author

@felixwang9817 felixwang9817 Jul 5, 2022

Choose a reason for hiding this comment

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

yeah this is fair; I'll remove this in some future PR when I clean up test suite

entity = Entity(name="first")
fs = environment.feature_store
fs.apply([entity])

entities = fs.list_entities()
assert len(entities) == 1


@pytest.mark.integration
def test_apply_second_entity(environment):
entity = Entity(name="second")
fs = environment.feature_store
fs.apply([entity])

entities = fs.list_entities()
assert len(entities) == 1