fix(ci): test script fixes for node 22 and 26 - #9038
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates package management and script execution from 'npm' to 'pnpm' across multiple packages, updates test coverage configurations, and enhances 'bigtable' system tests to gracefully handle 'RESOURCE_EXHAUSTED' quota errors and cleanup failures. Additionally, type casting is added to GCS headers to resolve TypeScript compilation issues. The reviewer feedback highlights several redundant nested 'try-catch' blocks in the updated test files that result in dead code, as well as a duplicated 'pnpm' installation command in a CI script.
|
@gemini-code-assist review please! |
There was a problem hiding this comment.
Code Review
This pull request updates system tests to handle RESOURCE_EXHAUSTED quota errors gracefully by switching arrow functions to regular functions to support Mocha's this.skip(), adds type assertions to satisfy TypeScript compiler checks, and configures NODE_OPTIONS for Node v22. The review feedback highlights a critical issue where NODE_OPTIONS is overwritten in run_single_test.sh, discarding parent configurations, and identifies several potential resource leaks in bigtable.ts where instances or clusters are not cleaned up if tests fail before the delete calls.
|
@gemini-code-assist anything else? |
|
The changes in this pull request look solid and effectively address the CI stability issues, quota handling, and Node 26 compatibility. The use of One minor observation: your PR description mentions pinning Otherwise, the code looks good and ready for merge. |
misc test fixes needed for #9021 and #8973:
--no-experimental-require-modulespecifically to Node v22 inci/run_conditional_tests.shso Node v24/v26 ESM imports continue to work withc8.ci/run_single_test.shto append toNODE_OPTIONSrather than overwriting it, preserving parent CI environment configurations.