Skip to content

chore(firestore): optimize system tests runtime #17418

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
ohmayr merged 4 commits into from
Jun 11, 2026
Merged

chore(firestore): optimize system tests runtime #17418

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9066c33
wip
ohmayr Jun 10, 2026
a3b5b1d
update unique resource id
ohmayr Jun 11, 2026
e11bb0e
change sleep time
ohmayr Jun 11, 2026
2e448a7
address PR feedback
ohmayr Jun 11, 2026
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
5 changes: 5 additions & 0 deletions packages/google-cloud-firestore/noxfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
"pytest-asyncio==0.21.2",
"six",
"pyyaml",
"pytest-xdist",
Comment thread
ohmayr marked this conversation as resolved.
]
SYSTEM_TEST_LOCAL_DEPENDENCIES: List[str] = []
SYSTEM_TEST_DEPENDENCIES: List[str] = []
Expand Down Expand Up @@ -402,6 +403,8 @@ def system(session):
if system_test_exists:
session.run(
"py.test",
"-n",
"auto",
Comment on lines +406 to +407

@gemini-code-assist gemini-code-assist Bot Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When running system tests in parallel with pytest-xdist (-n auto), the default distribution mode is --dist=load, which distributes individual tests to any available worker. This can cause tests from the same module to run on different workers, meaning module-scoped fixtures (like query_docs which was changed to scope="module" in this PR) will be executed multiple times (once per worker), defeating the caching benefit.

To ensure that all tests within a single module run on the same worker and preserve the efficiency of module-scoped fixtures, consider using --dist=loadfile.

Suggested change
"-n",
"auto",
"-n",
"auto",
"--dist",
"loadfile",

"--quiet",
f"--junitxml=system_{session.python}_sponge_log.xml",
system_test_path,
Expand All @@ -410,6 +413,8 @@ def system(session):
if system_test_folder_exists:
session.run(
"py.test",
"-n",
"auto",
Comment on lines +416 to +417

@gemini-code-assist gemini-code-assist Bot Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When running system tests in parallel with pytest-xdist (-n auto), the default distribution mode is --dist=load, which distributes individual tests to any available worker. This can cause tests from the same module to run on different workers, meaning module-scoped fixtures (like query_docs which was changed to scope="module" in this PR) will be executed multiple times (once per worker), defeating the caching benefit.

To ensure that all tests within a single module run on the same worker and preserve the efficiency of module-scoped fixtures, consider using --dist=loadfile.

Suggested change
"-n",
"auto",
"-n",
"auto",
"--dist",
"loadfile",

"--quiet",
f"--junitxml=system_{session.python}_sponge_log.xml",
system_test_folder_path,
Expand Down
2 changes: 1 addition & 1 deletion packages/google-cloud-firestore/tests/system/test__helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
MISSING_DOCUMENT = "No document to update: "
DOCUMENT_EXISTS = "Document already exists: "
ENTERPRISE_MODE_ERROR = "only allowed on ENTERPRISE mode"
UNIQUE_RESOURCE_ID = unique_resource_id("-")
UNIQUE_RESOURCE_ID = unique_resource_id("-") + "-" + str(os.getpid())

@daniel-sanche daniel-sanche Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If unique_resource_id() isn't unique enough, this logic should probably be added there, instead of appended afterwards

@ohmayr ohmayr Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! We can address this as a follow up or determine why it won't work.

@ohmayr ohmayr Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #17440.

EMULATOR_CREDS = EmulatorCreds()
FIRESTORE_EMULATOR = os.environ.get(_FIRESTORE_EMULATOR_HOST) is not None
FIRESTORE_OTHER_DB = os.environ.get("SYSTEM_TESTS_DATABASE", "system-tests-named-db")
Expand Down
32 changes: 16 additions & 16 deletions packages/google-cloud-firestore/tests/system/test_system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,7 @@ def test_unicode_doc(client, cleanup, database):
assert snapshot2.reference.id == explicit_doc_id


@pytest.fixture
@pytest.fixture(scope="module")
def query_docs(client, database):
Comment on lines +1271 to 1272

@gemini-code-assist gemini-code-assist Bot Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If the database fixture is function-scoped, defining query_docs with scope="module" will raise a ScopeMismatch error. To prevent this, the database fixture must also be configured with scope="module" or higher, or query_docs should remain function-scoped.

collection_id = "qs" + UNIQUE_RESOURCE_ID
sub_collection = "child" + UNIQUE_RESOURCE_ID
Expand Down Expand Up @@ -1297,13 +1297,13 @@ def query_docs(client, database):
operation()


@pytest.fixture
@pytest.fixture(scope="module")
def collection(query_docs):
collection, _, _ = query_docs
return collection


@pytest.fixture
@pytest.fixture(scope="module")
def query(collection):
return collection.where(filter=FieldFilter("a", "==", 1))

Expand Down Expand Up @@ -2336,7 +2336,7 @@ def test_watch_document(client, cleanup, database):
doc_ref.set({"first": "Jane", "last": "Doe", "born": 1900})
cleanup(doc_ref.delete)

sleep(1)
sleep(0.2)
Comment thread
ohmayr marked this conversation as resolved.

# Setup listener
def on_snapshot(docs, changes, read_time):
Expand All @@ -2349,12 +2349,12 @@ def on_snapshot(docs, changes, read_time):
# Alter document
doc_ref.set({"first": "Ada", "last": "Lovelace", "born": 1815})

sleep(1)
sleep(0.2)

for _ in range(10):
for _ in range(50):
if on_snapshot.called_count > 0:
break
sleep(1)
sleep(0.2)

if on_snapshot.called_count not in (1, 2):
raise AssertionError(
Expand Down Expand Up @@ -2385,14 +2385,14 @@ def on_snapshot(docs, changes, read_time):
collection_ref.on_snapshot(on_snapshot)

# delay here so initial on_snapshot occurs and isn't combined with set
sleep(1)
sleep(0.2)

doc_ref.set({"first": "Ada", "last": "Lovelace", "born": 1815})

for _ in range(10):
for _ in range(50):
if on_snapshot.born == 1815:
break
sleep(1)
sleep(0.2)

if on_snapshot.born != 1815:
raise AssertionError(
Expand All @@ -2411,7 +2411,7 @@ def test_watch_query(client, cleanup, database):
doc_ref.set({"first": "Jane", "last": "Doe", "born": 1900})
cleanup(doc_ref.delete)

sleep(1)
sleep(0.2)

# Setup listener
def on_snapshot(docs, changes, read_time):
Expand All @@ -2429,10 +2429,10 @@ def on_snapshot(docs, changes, read_time):
# Alter document
doc_ref.set({"first": "Ada", "last": "Lovelace", "born": 1815})

for _ in range(10):
for _ in range(50):
if on_snapshot.called_count == 1:
return
sleep(1)
sleep(0.2)

if on_snapshot.called_count != 1:
raise AssertionError(
Expand Down Expand Up @@ -2806,7 +2806,7 @@ def on_snapshot(docs, changes, read_time):
on_snapshot.failed = None
query_ref.on_snapshot(on_snapshot)

sleep(1)
sleep(0.2)

doc_ref1.set({"first": "Ada", "last": "Lovelace", "born": 1815})
cleanup(doc_ref1.delete)
Expand All @@ -2823,10 +2823,10 @@ def on_snapshot(docs, changes, read_time):
doc_ref5.set({"first": "Ada", "last": "lovelace", "born": 1815})
cleanup(doc_ref5.delete)

for _ in range(10):
for _ in range(50):
if on_snapshot.last_doc_count == 5:
break
sleep(1)
sleep(0.2)

if on_snapshot.failed:
raise on_snapshot.failed
Expand Down
6 changes: 3 additions & 3 deletions packages/google-cloud-firestore/tests/system/test_system_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,7 +1243,7 @@ async def test_list_collections_with_read_time(client, cleanup, database):
}


@pytest_asyncio.fixture
@pytest_asyncio.fixture(scope="module")

@gemini-code-assist gemini-code-assist Bot Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In pytest-asyncio (especially with the pinned version 0.21.2 in noxfile.py), the default event_loop fixture is function-scoped. Defining an async fixture with scope="module" (like query_docs, collection, and async_query) will raise a ScopeMismatch error during test collection or execution.

To resolve this, you must define a module-scoped event_loop fixture in this file or in conftest.py to override the default one.

@pytest.fixture(scope="module")
def event_loop():
    loop = asyncio.get_event_loop_policy().new_event_loop()
    yield loop
    loop.close()


@pytest_asyncio.fixture(scope="module")

async def query_docs(client):
collection_id = "qs" + UNIQUE_RESOURCE_ID
sub_collection = "child" + UNIQUE_RESOURCE_ID
Expand Down Expand Up @@ -1272,13 +1272,13 @@ async def query_docs(client):
await operation()


@pytest_asyncio.fixture
@pytest_asyncio.fixture(scope="module")
async def collection(query_docs):
collection, _, _ = query_docs
yield collection


@pytest_asyncio.fixture
@pytest_asyncio.fixture(scope="module")
async def async_query(collection):
return collection.where(filter=FieldFilter("a", "==", 1))

Expand Down
Loading