Skip to content

fix: make test_utils unique_resource_id parallel-safe#17440

Merged
ohmayr merged 2 commits into
mainfrom
fix-testutils-unique-resource-id
Jun 12, 2026
Merged

fix: make test_utils unique_resource_id parallel-safe#17440
ohmayr merged 2 commits into
mainfrom
fix-testutils-unique-resource-id

Conversation

@ohmayr

@ohmayr ohmayr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

This PR shifts the parallel-safety logic (os.getpid()) out of the individual firestore system tests, baking it directly into the shared test_utils package.

This ensures that all packages in the monorepo using pytest-xdist will automatically generate unique collection/resource names that never collide across parallel workers, rather than requiring each library to manually append PID strings to unique_resource_id().

@ohmayr ohmayr requested review from a team as code owners June 12, 2026 02:21

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request updates the unique_resource_id helper in google-cloud-testutils to automatically include the process ID (PID) in the generated ID, and simplifies the Firestore system tests which previously appended the PID manually. It also updates the Firestore post-processing configuration and noxfile.py to treat google-cloud-testutils as a local dependency. The review feedback suggests using millisecond resolution (1000 * time.time()) instead of second resolution in the CI environment path of unique_resource_id to prevent potential resource ID collisions when multiple resources are created within the same second.

Comment on lines +80 to +81
else:
return "%s%s%s%d" % (delimiter, build_id, delimiter, time.time())
return "%s%s%s%d%s%d" % (delimiter, build_id, delimiter, time.time(), delimiter, pid)

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

In CI environments (where build_id is present), using time.time() with %d formatting provides only 1-second resolution. If a test suite or parallel workers within the same build create multiple unique resources within the same second, this can lead to resource ID collisions.

Using millisecond resolution (1000 * time.time()) in the else block, consistent with the if block, will significantly reduce the likelihood of such collisions.

Suggested change
else:
return "%s%s%s%d" % (delimiter, build_id, delimiter, time.time())
return "%s%s%s%d%s%d" % (delimiter, build_id, delimiter, time.time(), delimiter, pid)
else:
return "%s%s%s%d%s%d" % (delimiter, build_id, delimiter, 1000 * time.time(), delimiter, pid)

@chalmerlowe chalmerlowe left a comment

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.

LGTM

@ohmayr ohmayr merged commit ac1f5d5 into main Jun 12, 2026
32 checks passed
@ohmayr ohmayr deleted the fix-testutils-unique-resource-id branch June 12, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants