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 all 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
52 changes: 52 additions & 0 deletions .librarian/generator-input/client-post-processing/firestore-integration.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,9 @@ replacements:
"freezegun",
]
count: 1
# TODO(https://github.com/googleapis/google-cloud-python/issues/17429):
# Temporary post-processing rule to add pytest-xdist dependency.
# Remove this once gapic-generator includes pytest-xdist by default.

@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.

I'm surprised firestore uses a generated noxfile. I thought most veneers had their own customized copies

It looks like bigtable has the entire noxfile contents in a post-processing script now? This seems like something we might want to revisit, but probably out of scope for this PR

@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.

Yes. Firestore is not completely handwritten. We override things via post-processing. There's many ways to go about this. This is more so a design change i.e. we could make it completely handwritten or have the entire file in post processing script or implement something on the generator side too.

But yes, out of scope for this work.

- paths: [
packages/google-cloud-firestore/noxfile.py
]
Expand All @@ -574,6 +577,7 @@ replacements:
"pytest-asyncio",
"six",
"pyyaml",
"pytest-xdist",
]
count: 1
- paths: [
Expand All @@ -584,6 +588,54 @@ replacements:
after: |
"pytest-asyncio==0.21.2",
count: 2
# TODO(https://github.com/googleapis/google-cloud-python/issues/17429):

@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.

What's the plan for these TODOs? Can you make sure we follow through with them, instead of leaving them in the backlog?

@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.

Well! Depends if we want to work on flaky tests within this fixit and have the time for it. I'll sync on this one and if not a priority, we can close this bug.

@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 #17439.

# Temporary post-processing rule to inject `-n auto` for Firestore parallel tests.
# This rule should be removed once the generator template changes are released
# and the generator version is updated in librarian.yaml.

@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.

Is the plan to include -n auto for all system tests?

@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.

Let's have this discussion on this PR: #17430

- paths: [
packages/google-cloud-firestore/noxfile.py
]
before: |
# Run py.test against the system tests\.
if system_test_exists:
session\.run\(
"py.test",
"--quiet",
f"--junitxml=system_\{session\.python\}_sponge_log\.xml",
system_test_path,
\*session\.posargs,
\)
if system_test_folder_exists:
session\.run\(
"py.test",
"--quiet",
f"--junitxml=system_\{session\.python\}_sponge_log\.xml",
system_test_folder_path,
\*session\.posargs,
\)
after: |
# Run py.test against the system tests.
if system_test_exists:
session.run(
"py.test",
"-n",
"auto",
"--quiet",
f"--junitxml=system_{session.python}_sponge_log.xml",
system_test_path,
*session.posargs,
)
if system_test_folder_exists:
session.run(
"py.test",
"-n",
"auto",
"--quiet",
f"--junitxml=system_{session.python}_sponge_log.xml",
system_test_folder_path,
*session.posargs,
)
count: 1
- paths: [
"packages/google-cloud-firestore/docs/conf.py",
]
Expand Down
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
48 changes: 32 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,11 @@ def test_watch_document(client, cleanup, database):
doc_ref.set({"first": "Jane", "last": "Doe", "born": 1900})
cleanup(doc_ref.delete)

sleep(1)
# TODO(https://github.com/googleapis/google-cloud-python/issues/17428):
# Investigate why these sleep/polling delays are needed for listener tests.
# Having arbitrary delays is fragile and can lead to flakiness.
# Explore event-driven synchronization.
sleep(0.2)
Comment thread
ohmayr marked this conversation as resolved.

# Setup listener
def on_snapshot(docs, changes, read_time):
Expand All @@ -2349,12 +2353,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 @@ -2384,15 +2388,19 @@ def on_snapshot(docs, changes, read_time):

collection_ref.on_snapshot(on_snapshot)

# TODO(https://github.com/googleapis/google-cloud-python/issues/17428):
# Investigate why these sleep/polling delays are needed for listener tests.
# Having arbitrary delays is fragile and can lead to flakiness.
# Explore event-driven synchronization.
# 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 +2419,11 @@ def test_watch_query(client, cleanup, database):
doc_ref.set({"first": "Jane", "last": "Doe", "born": 1900})
cleanup(doc_ref.delete)

sleep(1)
# TODO(https://github.com/googleapis/google-cloud-python/issues/17428):
# Investigate why these sleep/polling delays are needed for listener tests.
# Having arbitrary delays is fragile and can lead to flakiness.
# Explore event-driven synchronization.
sleep(0.2)

# Setup listener
def on_snapshot(docs, changes, read_time):
Expand All @@ -2429,10 +2441,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 +2818,11 @@ def on_snapshot(docs, changes, read_time):
on_snapshot.failed = None
query_ref.on_snapshot(on_snapshot)

sleep(1)
# TODO(https://github.com/googleapis/google-cloud-python/issues/17428):
# Investigate why these sleep/polling delays are needed for listener tests.
# Having arbitrary delays is fragile and can lead to flakiness.
# Explore event-driven synchronization.
sleep(0.2)

doc_ref1.set({"first": "Ada", "last": "Lovelace", "born": 1815})
cleanup(doc_ref1.delete)
Expand All @@ -2823,10 +2839,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