Skip to content

chore(ci): misc ci fixes, workflow renames, and system test concurrency - #9058

Closed
bshaffer wants to merge 10 commits into
mainfrom
misc-ci-fixes
Closed

chore(ci): misc ci fixes, workflow renames, and system test concurrency#9058
bshaffer wants to merge 10 commits into
mainfrom
misc-ci-fixes

Conversation

@bshaffer

@bshaffer bshaffer commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Misc test improvements and fixes, namely:

  • Renames conformance-test and conformance to bigtable-conformance and storage-conformance respectively.
  • Increases Windows pack-n-play test timeouts to 10 minutes and adds --no-audit --no-fund to speed up installs.
  • Scopes --no-experimental-require-module strictly to Node 22 in CI scripts to prevent ESM crashes on Node 24 and 26.
  • Prevents Bigtable's reapInstances cleanup script from deleting active test instances missing a time_created label.
  • Fixes Bigtable OpenTelemetry timestamp clamping (endTime = startTime + 1ns) to prevent Cloud Monitoring INVALID_ARGUMENT export errors.
  • Fixes Firestore GCB system test contention by running standard and enterprise databases in two concurrent streams instead of four.

We will be able to comment out triggering all unit tests on ci/ script once the sharding PR (#9021) lands.

@bshaffer
bshaffer requested review from a team as code owners August 3, 2026 15:39

@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 several CI and testing improvements, including test sharding and strict mode support in run_conditional_tests.sh, migration to the standard URL API in nodejs-googleapis-common, and robust error cause handling in gcp-metadata. Feedback on these changes highlights a missing system-test:enterprise:emulator:grpc script in firestore/package.json that was accidentally removed, a potential issue with prematurely unwrapping error causes in gcp-metadata, and the need to initialize the STRICT variable in run_conditional_tests.sh to prevent unbound variable errors.

Comment thread handwritten/firestore/package.json Outdated
Comment thread core/packages/gcp-metadata/src/index.ts Outdated
Comment thread ci/run_conditional_tests.sh Outdated
@bshaffer

bshaffer commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist please review!

@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 several CI and testing improvements, including support for strict mode, dry-run sharding, and test sharding in run_conditional_tests.sh, updated Mocha reporter configurations, and increased test timeouts. It also replaces the deprecated url.parse with new URL in nodejs-googleapis-common, refines error handling in gcp-metadata, and updates instance reaping logic in bigtable system tests. Feedback is provided for gcp-metadata to ensure that nested errors within an AggregateError are also correctly unwrapped to prevent them from resolving to 'UNKNOWN' error codes.

Comment thread core/packages/gcp-metadata/src/index.ts Outdated
Comment thread ci/run_conditional_tests.sh Outdated
Comment thread ci/run_conditional_tests.sh Outdated

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.

Can you move this to an independent PR? And include the command you used to generate it?

@bshaffer bshaffer Aug 3, 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.

I didn't generate it, owlbot generates it and automatically adds it to PRs which don't have it but touch files in the directory (because it's out of date)...

I actually did create a separate PR for it, but since tests fail on it without THIS PR, I thought it was appropriate to put it in this one. Here it is (I closed it, but just reopened it if you'd rather merge it first): #9057

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.

Thanks for explaining, I don't love Owlbot creating commits.

For now, ideally you could push a commit to #9057 to just breaks caused by owlbot and submit that first.

@bshaffer bshaffer Aug 3, 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.

I also think it's very strange that owlbot commits directly to branches, and I don't like it at all.

As for #9057, we can just ignore the failing bigtable-system-tests knowing that they'll be fixed by this PR, and merge it.

Note: I think we should disable bigtable-system-tests anyway because they take 90 minutes to run and are flakey. Until they are faster and more reliable they are only slowing us down...

Comment thread ci/run_single_test.sh Outdated

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.

Thanks for explaining, I don't love Owlbot creating commits.

For now, ideally you could push a commit to #9057 to just breaks caused by owlbot and submit that first.

@bshaffer

bshaffer commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

I have broken out this PR into smaller, component-isolated PRs as requested:

@bshaffer bshaffer closed this Aug 3, 2026
@bshaffer
bshaffer deleted the misc-ci-fixes branch August 3, 2026 19:17
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