Skip to content

ci: configure systems tests using cloud build - #18976

Draft
parthea wants to merge 10 commits into
mainfrom
add-gcb-google-cloud-bigquery-storage
Draft

parthea wants to merge 10 commits into
mainfrom
add-gcb-google-cloud-bigquery-storage

Conversation

@parthea

@parthea parthea commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Towards b/578989906

@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 introduces a new Cloud Build configuration (ci/cloudbuild/system-test.yaml) to run system tests in shards, featuring an inline Python script to filter packages and a pytest shim to handle Google Application Credentials. The review feedback highlights two important issues: first, a potential mismatch in package path filtering due to trailing slashes that could silently skip tests, and second, a risk of unexpected module shadowing caused by a trailing colon in PYTHONPATH when it is initially empty.

Comment thread ci/cloudbuild/system-test.yaml Outdated
Comment on lines +49 to +56
enabled = {
p.strip().rstrip("/") + "/"
for p in os.environ.get("ENABLED_PACKAGES", "").replace(",", " ").split()
if p.strip()
}
if enabled:
orig_get_packages = gps.get_packages
gps.get_packages = lambda: [p for p in orig_get_packages() if p in enabled]

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

There is a potential mismatch between the package paths returned by get_package_shards.py and the filtered ENABLED_PACKAGES. Adding a trailing slash + "/" to the enabled set while checking p in enabled (where p might not have a trailing slash) will cause the check to fail. This results in gps.get_packages() returning an empty list, which silently skips all system tests and reports an immediate green build.

To prevent this, normalize both the enabled packages and the package paths from orig_get_packages() by stripping trailing slashes before comparison.

      enabled = {
          p.strip().rstrip("/")
          for p in os.environ.get("ENABLED_PACKAGES", "").replace(",", " ").split()
          if p.strip()
      }
      if enabled:
          orig_get_packages = gps.get_packages
          gps.get_packages = lambda: [p for p in orig_get_packages() if p.strip().rstrip("/") in enabled]

Comment thread ci/cloudbuild/system-test.yaml Resolved
@parthea parthea closed this Nov 4, 2026
@parthea parthea reopened this Nov 4, 2026
@parthea parthea closed this Nov 4, 2026
@parthea parthea reopened this Nov 4, 2026
@parthea parthea closed this Nov 5, 2026
@parthea parthea reopened this Nov 5, 2026
@parthea parthea closed this Nov 5, 2026
@parthea parthea reopened this Nov 5, 2026
@parthea parthea closed this Nov 5, 2026
@parthea parthea reopened this Nov 5, 2026
@parthea parthea changed the title ci: configure systems tests using cloud build for google-cloud-bigquery-storage ci: configure systems tests using cloud build Nov 5, 2026

This branch has not been deployed

No deployments
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.

1 participant