feat(ci): improve test performance - #9021
Conversation
| # echo "skipping trigger of tests for now: tracking in #7540" | ||
| echo "change detected in ci, we should test everything" | ||
| echo "result of git diff ${GIT_DIFF_ARG} ci:" | ||
| git diff ${GIT_DIFF_ARG} ci | ||
| GIT_DIFF_ARG="" |
There was a problem hiding this comment.
As tests now take ~30 minutes (down from 2 hours), we can reenable this and close #7540.
Once we add PNPM workspaces and turbo caching, this number goes down to ~15!!
There was a problem hiding this comment.
Code Review
This pull request parallelizes test execution and adds sharding support in the CI conditional test runner, configures the Mocha reporter and silences pnpm installation in the single test runner, and updates mock discovery URLs in the googleapis-common tests. A critical syntax error was identified in the conditional test runner script, where leftover loop and conditional control structures will cause the Bash script to fail.
This reverts commit 37e6bcf.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces test sharding and dynamic matrix generation for GitHub Actions in the CI scripts, updates pnpm install flags, and transitions from the deprecated url.parse to the standard URL API. Additionally, it increases test timeouts, skips certain tests on Windows, handles nested error causes in gcp-metadata, and ensures startTime does not exceed endTime in Bigtable metrics. Feedback focuses on improving shell script robustness by quoting variables (BUILD_TYPE, d, and test_script) to prevent syntax or word-splitting issues, and avoiding direct mutation of the options parameter in the OpenTelemetry test stub.
westarle
left a comment
There was a problem hiding this comment.
A few comments, I think this is a really great idea!
|
|
||
| if [[ -n "${SHARD_TOTAL}" && -n "${SHARD_INDEX}" ]]; then | ||
| if (( SHARD_TOTAL > 0 && i % SHARD_TOTAL != SHARD_INDEX )); then | ||
| continue |
There was a problem hiding this comment.
We should just keep an eye on whether there are differences between the shard durations (in case multiple "heavy" targets land in a single shard consistently.)
There was a problem hiding this comment.
Yes, I've already seen this happen that Shard 0 is a little heavier, probably because of the bigtable and pack-n-play tests. If a big discrepancy emerges (right now, it seems like just a few minutes, so I am not sure it's a big concern), we could take the known longer tests and manually separate them? What are your thoughts on the best way to accomplish this?
There was a problem hiding this comment.
we could take the known longer tests and manually separate them
This it what I'd do.
right now, it seems like just a few minutes, so I am not sure it's a big concern
+1
There was a problem hiding this comment.
could this be in another PR?
There was a problem hiding this comment.
The bigtable unit tests were failing, so I added these changes to fix them... however I'm not 100% sure THIS specific file update is needed (I suspect it is). I can verify!
| run_id: context.runId, | ||
| }); | ||
| const nodeVersion = '${{ matrix.node-version }}'; | ||
| const shardJobs = jobs.filter(j => j.name.includes(`units (Node ${nodeVersion},`)); |
There was a problem hiding this comment.
can we reuse test_dirs from setup?
There was a problem hiding this comment.
I went with this way because:
- Computing
test_dirswithgit difftakes ~1 second. Passing it via GitHub Actions adds ~5-10 seconds of network transfer and disk I/O per shard. - Because we have to calculate on both Windows vs. Linux, this way allows each OS runner to apply platform-specific handling (e.g., path separators and windows_exempt_tests) natively without needing serialization/translation across platforms.
- It allows
ci/run_conditional_tests.shto be executed standalone
| it('should run tests', async () => { | ||
| it('should run tests', async function () { | ||
| this.timeout(120000); // 2 minutes | ||
| if (process.platform === 'win32') { |
There was a problem hiding this comment.
I'd be more comfortable doing this after we set up a postsubmit for the coverage. It seems like Windows would cause more problems in packaging...
There was a problem hiding this comment.
honestly after testing, this did not affect performance that much, and windows STILL took 30-40 minutes, so I think removing it is the right call
| echo "pnpm install --ignore-scripts --engine-strict --prod --pnpmfile \"${PNPMFILE_PATH}\"; pnpm install --pnpmfile \"${PNPMFILE_PATH}\"" | ||
| pnpm install --ignore-scripts --engine-strict --prod --pnpmfile "${PNPMFILE_PATH}"; pnpm install --pnpmfile "${PNPMFILE_PATH}" | ||
| echo "pnpm install --reporter=silent --engine-strict --pnpmfile \"${PNPMFILE_PATH}\"" | ||
| pnpm install --reporter=silent --engine-strict --pnpmfile "${PNPMFILE_PATH}" |
There was a problem hiding this comment.
Are errors still reported with --reporter=silent?
There was a problem hiding this comment.
Yes, errors are still reported! --reporter=silent suppresses stdout, but errors are still sent to stderr.
westarle
left a comment
There was a problem hiding this comment.
Also, would you be OK with breaking the test fixes out to their own little PRs?
Conditional Test Sharding: Dynamically shards tests when a large number of packages are modified. Adds a job that rolls up shard results to satisfy branch protection. For what this looks like with just ONE package modified, see chore: test sharding with one package #9027
Pipeline Speed: Reduces GitHub Action checkout fetch-depth to 2, which saves ~4 minutes for each shard
Clean CI Logs: Switches Mocha to use
dotreporter and removes accidentalconsole.logfrom the Firestore conformance tests.Disambiguous Workflow Names: Renames
conformance-testandconformancetobigtable-conformanceandstorage-conformancerespectively.Test Fixes:
npm installurl.parse()withnew URL()innodejs-googleapis-commonto fix Mocha test crashesConfirm that it's okay to skip
pack-and-playunit tests on Windows (as these alone add 5-10 minutes).