From 061758e02fbbce70bdd3b77f3815316f7656e3b8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:04:19 -0700 Subject: [PATCH 1/2] chore(ci): consolidate the 3-shard test matrix into one unsharded validate-tests run At the one-shot gate's PR volume the 2 extra runners per PR were a major driver of org-wide Actions concurrency saturation (observed: 60-run queue), and the shard seams were a recurring flake/complexity source. One slower job (~10-15 min added PR latency, timeout raised to 45) buys: - 2 fewer runners per PR, every PR - ONE honest whole-suite lcov (the exact signal npm run test:coverage gives locally), replacing 3 additive shard uploads -- codecov.yml's after_n_builds drops 3 -> 1 (the new always-lands floor; the narrow rees/control-plane premature-window this reopens self-corrects and is documented in place) - the global 90% threshold runs INLINE in full-suite runs (scoped runs keep COVERAGE_NO_THRESHOLDS via the same expression the merge job used), so validate-tests-merge is deleted outright - the whole duration-aware sharding stack retires: compute-test-shards.ts, fetch-test-timing.ts, test-timing-refresh.yml (one fewer scheduled workflow), their suites, and the blob-report artifact plumbing Codecov flags: shard-N -> backend. codecov-policy pins updated (the merge job's absence is now itself pinned). --- .github/workflows/ci.yml | 206 +++++---------------- .github/workflows/test-timing-refresh.yml | 47 ----- codecov.yml | 24 +-- scripts/compute-test-shards.ts | 146 --------------- scripts/fetch-test-timing.ts | 148 --------------- test/unit/codecov-policy.test.ts | 9 +- test/unit/compute-test-shards.test.ts | 119 ------------ test/unit/fetch-test-timing-script.test.ts | 84 --------- 8 files changed, 56 insertions(+), 727 deletions(-) delete mode 100644 .github/workflows/test-timing-refresh.yml delete mode 100644 scripts/compute-test-shards.ts delete mode 100644 scripts/fetch-test-timing.ts delete mode 100644 test/unit/compute-test-shards.test.ts delete mode 100644 test/unit/fetch-test-timing-script.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 438cdc1628..8b20f9a191 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,8 +46,8 @@ jobs: controlPlane: ${{ steps.filter.outputs.controlPlane }} observability: ${{ steps.filter.outputs.observability }} steps: - # Debounce rapid re-pushes to an open PR: every job below (validate-code, all 3 validate-tests - # shards, validate-tests-merge, security) has `needs: changes` in its dependency chain, so none of + # Debounce rapid re-pushes to an open PR: every job below (validate-code, validate-tests, + # security) has `needs: changes` in its dependency chain, so none of # them get queued for a runner until this job finishes -- and this job lives in the SAME PR-ref-scoped # concurrency group as everything else (top of file), so a newer push cancels this job outright while # it's asleep, well before it ever reaches "Filter changed paths". That means a rapid burst of pushes @@ -816,39 +816,35 @@ jobs: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} run: npm run bundle-analysis --workspace @loopover/ui - # The full-suite coverage run, sharded (#ci-shard-coverage). This alone was ~9-10 of the ~11 minutes a - # typical backend PR spent in validate-code, because vitest schedules whole test FILES atomically to - # workers -- one unsharded job with --maxWorkers=4 (a standard runner's real CPU budget) has no way to - # use more than 4-way parallelism no matter how many files exist. Splitting the vitest invocation itself - # across N parallel GitHub-hosted jobs (vitest's own --shard=/, not a hand-rolled file list) gives - # N*4-way parallelism instead, at zero extra $ on this public repo (standard runners are free regardless - # of job count -- more jobs consumes more of the shared free pool, not more billable minutes). - # Each shard uploads its own lcov.info + junit report to Codecov; multiple uploads for the same - # commit/PR are additive (Codecov merges them into one patch-coverage view), which is the standard - # pattern for matrix-split test suites. vitest.config.ts already had COVERAGE_NO_THRESHOLDS support - # wired in (a shard only exercises part of the tree, so the global 90% backstop threshold would always - # false-fail per-shard) -- this job is what finally turns it on; previously nothing ever set it. + # The full-suite coverage run (#ci-shard-coverage kept it OUT of validate-code so the ~25-minute suite + # never serializes with the fast drift/typecheck/build checks -- that split remains). It ran 2023-style + # as a 3-shard matrix for wall-clock; unsharded again 2026-07-24 (see the job's own header comment) to + # trade PR latency for 2 fewer runners per PR and the simpler, flake-free single-report pipeline. validate-tests: name: validate-tests needs: changes - # Also runs on REES/mcp/engine/miner/discoveryIndex-only changes: Codecov holds codecov/patch until - # `after_n_builds` (3) coverage uploads land (codecov.yml), and a PR scoped to just one of those - # self-contained packages doesn't otherwise touch `backend`, so without the shards it would upload no + # Also runs on REES/mcp/engine/miner/discoveryIndex-only changes: a PR scoped to just one of those + # self-contained packages doesn't otherwise touch `backend`, so without this job it would upload no # coverage report at all and codecov/patch would never post -- not even a vacuous pass, no check at # all, and the required `validate` aggregate treats a skipped job as success (#7318-#7321: 3 - # packages/loopover-miner/lib-only PRs merged this way with validate-tests never running). Running the - # shards guarantees the 3 uploads so Codecov posts a real patch verdict covering whichever package - # actually changed (review-enrichment/** via the `rees` flag from validate-code; packages/loopover-mcp, + # packages/loopover-miner/lib-only PRs merged this way with validate-tests never running). Running it + # guarantees the upload so Codecov posts a real patch verdict covering whichever package actually + # changed (review-enrichment/** via the `rees` flag from validate-code; packages/loopover-mcp, # packages/loopover-engine, packages/loopover-miner, and packages/discovery-index via their entries in - # vitest.config.ts's coverage.include, exercised by the root test/** suite these shards already run). + # vitest.config.ts's coverage.include, exercised by the root test/** suite this job already runs). # Draft PRs still skip the whole fan-out (#6448), so these triggers only apply to ready PRs. + # + # UNSHARDED (2026-07-24, queue-pressure + reliability consolidation): this ran as a 3-shard matrix + # (#ci-shard-coverage) to cut wall-clock; at the one-shot gate's PR volume those 2 extra runners per + # PR were a major driver of org-wide concurrency saturation (observed: 60-run queue), and the shard + # seams were a recurring flake/complexity source (blob merges, per-shard threshold suppression, + # duration-aware bin-packing, premature partial Codecov verdicts needing after_n_builds). One slower + # job trades ~10-15 min of PR latency for 2 fewer runners per PR, one honest whole-suite coverage + # report (the same signal `npm run test:coverage` gives locally), an inline global threshold check, + # and no merge job at all. if: ${{ github.event_name == 'push' || (github.event.pull_request.draft != true && (needs.changes.outputs.backend == 'true' || needs.changes.outputs.rees == 'true' || needs.changes.outputs.controlPlane == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.engine == 'true' || needs.changes.outputs.miner == 'true' || needs.changes.outputs.discoveryIndex == 'true')) }} runs-on: ubuntu-latest - timeout-minutes: 20 - strategy: - fail-fast: false - matrix: - shard: [1, 2, 3] + timeout-minutes: 45 env: VITE_LOOPOVER_API_ORIGIN: https://api.loopover.ai steps: @@ -938,38 +934,23 @@ jobs: key: turbo-tests-${{ hashFiles('package-lock.json') }}-${{ github.run_id }} - name: Prepare test reports dir run: mkdir -p reports/junit - # Restore-only -- this job never saves it, test-timing-refresh.yml is the sole writer, on its own - # schedule (pulling from Codecov's Test Analytics API, which already pools per-test durations - # across every push-to-main run). key never actually matches (no save ever uses this literal - # string); it exists only so the step always falls through to the restore-keys prefix match, - # picking whatever the most recent refresh wrote. A cache MISS here is always safe: see - # scripts/compute-test-shards.ts's fallback -- it splits evenly across shards when no timing - # data is available for a file (or none at all), the same balance vitest's own --shard already - # gives today, so this can only make shard balance better than today's baseline, never worse. - - name: Restore test timing cache - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: test-timing.json - key: test-timing-unused - restore-keys: | - test-timing- - - name: Test with coverage (shard ${{ matrix.shard }}/3) + - name: Test with coverage id: coverage env: - # Disables vitest.config.ts's own global 90% threshold check for THIS per-shard invocation -- a - # single shard only exercises part of the tree, so it would always false-fail. The global-threshold - # "catastrophe net" this disables is NOT dropped from CI, though: validate-tests-merge (below) merges - # all shards' coverage via vitest's own --mergeReports and re-checks the threshold against that - # merged (whole-suite) total, without COVERAGE_NO_THRESHOLDS set. - COVERAGE_NO_THRESHOLDS: "true" + # Disables vitest.config.ts's own global 90% threshold check ONLY for scoped-selection runs (the + # SCOPED_TEST_SELECTION expression below, verbatim): a scoped run exercises a narrow slice of the + # tree, so the whole-include-set threshold would always false-fail it (Codecov's patch gate still + # enforces real per-line coverage on the scoped diff). Full-suite runs keep the threshold INLINE -- + # unsharded, this job sees the whole suite, so the "loose catastrophe net" (e.g. a deleted test + # file) that previously required the separate validate-tests-merge job now just... runs here. + COVERAGE_NO_THRESHOLDS: ${{ (github.event_name == 'pull_request' && vars.SCOPED_TEST_SELECTION_ENABLED != 'false' && needs.changes.outputs.rees != 'true' && needs.changes.outputs.controlPlane != 'true' && needs.changes.outputs.engine != 'true' && needs.changes.outputs.backendConfig != 'true' && (needs.changes.outputs.backend == 'true' || needs.changes.outputs.miner == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.discoveryIndex == 'true')) && 'true' || '' }} # Same self-contained-test-file narrowing as the pre-sharding job had -- see the `mcpCliHarness`/ # `minerTestHarness` filter comments in the `changes` job above for the full rationale. SKIP_MCP_CLI_HARNESS: ${{ github.event_name == 'pull_request' && needs.changes.outputs.mcpCliHarness != 'true' }} SKIP_MINER_TEST_HARNESS: ${{ github.event_name == 'pull_request' && needs.changes.outputs.minerTestHarness != 'true' }} # Scoped test selection (#ci-scoped-test-selection): a PR touching miner/mcp/discoveryIndex/backend -- # never rees or engine -- runs vitest's own --changed against origin/main instead of the full - # ~2,900+-test suite, since the same 6-shard fan-out this job exists for (see header comment) is - # otherwise massive overkill for a diff touching a handful of files. engine is deliberately EXCLUDED + # ~2,900+-test suite, which is otherwise massive overkill for a diff touching a handful of files. engine is deliberately EXCLUDED # from this fast path: it's the one package other code imports through a built, gitignored dist/ that # --changed's import-graph analysis can't safely trace across a node_modules/package.json-exports # boundary -- verified by reproducing a real missed regression (editing packages/loopover-engine/ @@ -1055,7 +1036,7 @@ jobs: # scratch, and record it as a step output so "Verify coverage report exists" below can tell # this apart from a real crash that should still fail loudly. set -o pipefail - npm run test:coverage -- --maxWorkers=4 --shard=${{ matrix.shard }}/3 --reporter=default --reporter=blob --reporter=junit --outputFile.blob=blob-report/report-${{ matrix.shard }}.blob --outputFile.junit=reports/junit/vitest.xml "${EXCLUDE_ARGS[@]}" "${SCOPE_ARGS[@]}" 2>&1 | tee vitest-scoped-output.log + npm run test:coverage -- --maxWorkers=4 --reporter=default --reporter=junit --outputFile.junit=reports/junit/vitest.xml "${EXCLUDE_ARGS[@]}" "${SCOPE_ARGS[@]}" 2>&1 | tee vitest-scoped-output.log if grep -q "No test files found" vitest-scoped-output.log; then echo "no_tests_matched=true" >> "$GITHUB_OUTPUT" elif [ ! -s coverage/lcov.info ]; then @@ -1066,39 +1047,18 @@ jobs: # which is NOT a failed run; the junit report written above is the proof the suite ran. # Without this output, "Verify coverage report exists" fails the green run. The success # signal here is the test run itself, never the coverage file's size. - echo "scoped run passed but exercised no src/** files -- skipping coverage artifacts for this shard" + echo "scoped run passed but exercised no src/** files -- skipping coverage artifacts" echo "no_src_coverage=true" >> "$GITHUB_OUTPUT" fi else - # Duration-aware sharding (#ci-duration-aware-sharding), full-suite case only: vitest's own - # --shard splits by file COUNT alone, no duration awareness -- confirmed via real per-shard CI - # timing data to produce a consistent ~20-30% gap between the slowest and fastest of the 6 - # shards, every run sampled. Deliberately not applied to the scoped-selection branch above: - # that file set isn't known until vitest resolves --changed itself, so a precomputed - # assignment can't cover it without duplicating vitest's own dependency-graph resolution here. - # compute-test-shards.ts enforces its own hard invariant (the union of all 3 shards' files - # must exactly equal the discovered file set, no file missing or duplicated) and refuses to - # write output at all if that's ever violated, so a bug here fails this step loudly rather - # than silently dropping a test file from CI. - node --experimental-strip-types scripts/compute-test-shards.ts --shards=3 --timing=test-timing.json --output=shard-assignment.json - # #7767: capture via a checked assignment so a throw in the node one-liner (bad matrix.shard - # value, corrupt/missing shard-assignment.json) actually aborts this step -- the old - # `mapfile -t SHARD_FILES < <(node -e ...)` ran node in a subshell whose non-zero exit was - # swallowed (mapfile itself returns 0), leaving SHARD_FILES empty and silently making vitest - # run the entire suite instead of its slice. Same fix idiom as compose_file_args (#7765). - if ! SHARD_FILES_RAW="$(node -e "console.log(JSON.parse(require('fs').readFileSync('shard-assignment.json','utf8'))['${{ matrix.shard }}'].join('\n'))")"; then - echo "::error::failed to resolve shard file list for shard ${{ matrix.shard }}" - exit 1 - fi - mapfile -t SHARD_FILES <<< "$SHARD_FILES_RAW" - npm run test:coverage -- --maxWorkers=4 "${SHARD_FILES[@]}" --reporter=default --reporter=blob --reporter=junit --outputFile.blob=blob-report/report-${{ matrix.shard }}.blob --outputFile.junit=reports/junit/vitest.xml "${EXCLUDE_ARGS[@]}" + npm run test:coverage -- --maxWorkers=4 --reporter=default --reporter=junit --outputFile.junit=reports/junit/vitest.xml "${EXCLUDE_ARGS[@]}" fi - name: Test failure guidance if: ${{ failure() && steps.coverage.conclusion == 'failure' }} run: | - echo "::error title=Tests::The backend test coverage suite failed (shard ${{ matrix.shard }}/3)." - echo "Coverage itself is gated by Codecov on changed lines (codecov/patch), computed from all shards' merged lcov." - echo "Reproduce locally with: 'npm run test:coverage' (unsharded, runs the whole suite)." + echo "::error title=Tests::The backend test coverage suite failed." + echo "Coverage itself is gated by Codecov on changed lines (codecov/patch)." + echo "Reproduce locally with: 'npm run test:coverage' (the identical whole-suite run)." - name: Verify coverage report exists # Skipped when the scoped-selection branch above matched zero test files (steps.coverage.outputs. # no_tests_matched) -- a legitimately test-free diff (e.g. docs-only under a scoped path) writes no @@ -1113,17 +1073,7 @@ jobs: echo "::error title=Coverage::coverage/lcov.info is missing or empty" exit 1 fi - # Consumed by validate-tests-merge to re-check the global coverage threshold against all shards - # combined -- see this job's own header comment. - - name: Upload coverage blob report - if: ${{ success() && steps.coverage.outputs.no_tests_matched != 'true' && steps.coverage.outputs.no_src_coverage != 'true' }} - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: coverage-blob-shard-${{ matrix.shard }} - path: blob-report/report-${{ matrix.shard }}.blob - retention-days: 1 - # Direct upload for trusted contexts (push + same-repo PRs). Multiple shards' uploads for the same - # commit/PR are additive in Codecov -- see this job's own header comment. + # Direct upload for trusted contexts (push + same-repo PRs). - name: Upload coverage to Codecov # Same no_tests_matched skip as the verify step above (#8167 follow-up): with zero matched tests # there is no lcov to upload, and fail_ci_if_error would otherwise turn that non-event red. @@ -1132,7 +1082,7 @@ jobs: with: token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage/lcov.info - flags: shard-${{ matrix.shard }} + flags: backend disable_search: true override_branch: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.ref || github.ref_name }} override_commit: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} @@ -1149,7 +1099,7 @@ jobs: uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 with: files: ./coverage/lcov.info - flags: shard-${{ matrix.shard }} + flags: backend disable_search: true override_branch: ${{ github.event.pull_request.head.repo.owner.login }}:${{ github.event.pull_request.head.ref }} override_commit: ${{ github.event.pull_request.head.sha }} @@ -1165,7 +1115,7 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} files: ./reports/junit/vitest.xml report_type: test_results - flags: shard-${{ matrix.shard }} + flags: backend disable_search: true override_branch: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.ref || github.ref_name }} override_commit: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} @@ -1177,83 +1127,13 @@ jobs: with: files: ./reports/junit/vitest.xml report_type: test_results - flags: shard-${{ matrix.shard }} + flags: backend disable_search: true override_branch: ${{ github.event.pull_request.head.repo.owner.login }}:${{ github.event.pull_request.head.ref }} override_commit: ${{ github.event.pull_request.head.sha }} override_pr: ${{ github.event.pull_request.number }} fail_ci_if_error: false - # Re-checks vitest.config.ts's global 90% coverage threshold against all shards MERGED -- each shard - # above deliberately disables that check for itself (COVERAGE_NO_THRESHOLDS=true), since a single shard's - # partial view would always false-fail it. This is the "loose catastrophe net" (e.g. a deleted test file) - # vitest.config.ts's own comment describes; Codecov's patch gate (changed-lines only) doesn't cover a - # whole-repo regression outside the diff, so this is what actually restores that backstop for CI, using - # vitest's own --mergeReports against each shard's uploaded blob report. - # - # That "merged = whole-suite" assumption breaks when scoped test selection (#ci-scoped-test-selection, - # see the shard job's own SCOPED_TEST_SELECTION comment above) was active for this run: all 3 shards then - # ran the SAME narrow `--changed=origin/main` subset (not a partition of the ~2,900-test full suite), so - # merging them still only reconstructs that narrow slice's coverage -- correctly high for the files it - # touches, but the threshold judges the WHOLE include set, so it false-fails even a fully-tested scoped - # PR (confirmed live: a 3-file packages/loopover-mcp-only PR with 100% coverage on its own two touched - # test files still reported 0%/80% and failed). This job disables the threshold in that same case, via - # the identical SCOPED_TEST_SELECTION condition -- Codecov's patch gate already enforces real per-line - # coverage on a scoped PR's actual diff regardless, so nothing is lost by skipping the whole-suite - # backstop specifically when it can't see the whole suite. - validate-tests-merge: - name: validate-tests-merge - needs: [changes, validate-tests] - # Same trigger as validate-tests: whenever shards ran (REES/mcp/engine/miner/discoveryIndex-only PRs - # included), merge their reports too. - if: ${{ github.event_name == 'push' || (github.event.pull_request.draft != true && (needs.changes.outputs.backend == 'true' || needs.changes.outputs.rees == 'true' || needs.changes.outputs.controlPlane == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.engine == 'true' || needs.changes.outputs.miner == 'true' || needs.changes.outputs.discoveryIndex == 'true')) }} - runs-on: ubuntu-latest - timeout-minutes: 10 - steps: - - name: Checkout - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - with: - ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} - # Same cache key as validate-code/validate-tests -- a cache hit here is the common case since those - # jobs run concurrently and one of them usually wins the race to populate it first. save-cache: - # false -- this job only ever reads that cache, it never writes to it (see - # .github/actions/setup-workspace). - - name: Setup workspace - uses: ./.github/actions/setup-workspace - with: - save-cache: "false" - - name: Download all shards' blob reports - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 - with: - pattern: coverage-blob-shard-* - path: all-blob-reports - merge-multiple: true - - name: Merge shard coverage and check the global threshold - env: - # Mirrors validate-tests' own SCOPED_TEST_SELECTION condition exactly (this job already has - # `needs.changes` available). vitest.config.ts checks this var for TRUTHINESS, not `=== 'true'`, - # so the false branch must be an empty string (falsy), not the literal string "false" (which JS - # treats as truthy) -- that's why this is a `&& 'true' || ''` expression, not a bare boolean. - COVERAGE_NO_THRESHOLDS: ${{ (github.event_name == 'pull_request' && vars.SCOPED_TEST_SELECTION_ENABLED != 'false' && needs.changes.outputs.rees != 'true' && needs.changes.outputs.controlPlane != 'true' && needs.changes.outputs.engine != 'true' && needs.changes.outputs.backendConfig != 'true' && (needs.changes.outputs.backend == 'true' || needs.changes.outputs.miner == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.discoveryIndex == 'true')) && 'true' || '' }} - run: | - # Unlike the per-shard --changed run (validate-tests above), --mergeReports exits 1 (not 0) when - # every merged shard's report represents zero matched test files -- confirmed live: PR #8168, a - # pure docs-only scoped PR where all 3 shards legitimately matched nothing, printed vitest's own - # "No test files found, exiting with code 1" and failed here even with COVERAGE_NO_THRESHOLDS - # already disabling the threshold check that comment block above describes. Only treat that exact - # case as success, and only when COVERAGE_NO_THRESHOLDS is set (i.e. this run is the scoped case - # to begin with) -- in the unscoped full-suite case, "no test files found" while merging would be - # a real, surprising break worth failing loudly on, not a legitimate outcome to paper over. - set -o pipefail - if npx vitest run --coverage --mergeReports=all-blob-reports 2>&1 | tee vitest-merge-output.log; then - exit 0 - fi - if [ -n "$COVERAGE_NO_THRESHOLDS" ] && grep -q "No test files found" vitest-merge-output.log; then - echo "Merged report matched zero test files, which is expected for this scoped, test-free PR -- not a failure." - exit 0 - fi - exit 1 - # Diff-scoped security gate: fails only on vulnerabilities this PR introduces. # Ambient advisories in untouched deps are handled by Renovate + the scheduled # audit workflow, so one upstream CVE never blocks unrelated PRs. @@ -1283,7 +1163,7 @@ jobs: # Path-filtered jobs report "skipped", which is treated as success. validate: name: validate - needs: [changes, validate-code, validate-tests, validate-tests-merge, security] + needs: [changes, validate-code, validate-tests, security] if: ${{ always() }} # Pure result-aggregation (reads needs.*.result, echoes pass/fail) -- no build/test work, so it never # needed the self-hosted pool's cached toolchain (#2507). diff --git a/.github/workflows/test-timing-refresh.yml b/.github/workflows/test-timing-refresh.yml deleted file mode 100644 index 15fcfcf422..0000000000 --- a/.github/workflows/test-timing-refresh.yml +++ /dev/null @@ -1,47 +0,0 @@ -name: Test timing refresh - -# Pulls per-test-file historical duration data from Codecov's Test Analytics API (see -# scripts/fetch-test-timing.ts) and caches it for validate-tests' duration-aware shard bin-packer -# (scripts/compute-test-shards.ts, ci.yml's "Test with coverage" step) to consume. Runs on a schedule -# rather than per-PR: Codecov doesn't publish a numeric rate limit for this read endpoint, and this -# repo's PR volume (hundreds/day) makes "query fresh on every PR" a real risk of hitting one, for data -# that doesn't meaningfully change run-to-run anyway. - -on: - schedule: - - cron: "0 */6 * * *" # every 6 hours - workflow_dispatch: - -permissions: - contents: read - -concurrency: - group: test-timing-refresh - cancel-in-progress: true - -jobs: - refresh: - name: refresh - runs-on: ubuntu-latest - timeout-minutes: 10 - steps: - - name: Checkout - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - - name: Setup Node - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 - with: - node-version-file: .nvmrc - - name: Fetch test timing from Codecov - env: - CODECOV_API_TOKEN: ${{ secrets.CODECOV_API_TOKEN }} - run: node --experimental-strip-types scripts/fetch-test-timing.ts --output=test-timing.json - # Run_id-suffixed key (always a fresh entry, never a re-save of an existing one) + prefix - # restore-keys fallback on the consuming side (ci.yml's "Restore test timing cache" step) -- same - # accumulating-cache pattern already used for .turbo/cache and .tsbuildinfo elsewhere in this repo, - # since this data changes every run rather than being wholesale-replaced per some fixed input hash. - - name: Save test timing cache - if: ${{ !cancelled() }} - uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: test-timing.json - key: test-timing-${{ github.run_id }} diff --git a/codecov.yml b/codecov.yml index 99e5507877..d3214494cd 100644 --- a/codecov.yml +++ b/codecov.yml @@ -11,21 +11,15 @@ codecov: # Don't post a verdict until the CI run that produced the report has finished. require_ci_to_pass: true notify: - # The 3 backend test shards (validate-tests matrix, ci.yml) each upload their own lcov.info - # directly (flags: shard-N) -- additive by design, but Codecov re-evaluates and re-posts the - # status check after EVERY individual upload, not just the last one. Without this, codecov/patch - # can report a wildly wrong verdict (as low as ~20% "patch coverage") after only 1-2 of 3 shards - # have landed, before self-correcting once the rest arrive -- require_ci_to_pass above only checks - # the GH Actions run's own conclusion, it says nothing about upload completeness. Waiting for all - # 3 coverage uploads (JUnit test_results uploads are a separate stream and don't count here) before - # posting a status removes that premature-fail/premature-pass window entirely. - # - # Stays 3, not 4, even though review-enrichment adds a `rees` upload on REES PRs: 3 is the count that - # ALWAYS lands whenever validate-tests runs (the minimum floor), while the `rees` upload only exists on - # REES PRs. The `rees` report is produced by validate-code, which finishes its REES steps in seconds and - # so uploads well before any ~10-minute shard -- it is already merged in by the time the 3rd shard trips - # this threshold, so no premature window opens for the review-enrichment diff either. - after_n_builds: 3 + # The count that ALWAYS lands whenever validate-tests runs: since the 2026-07-24 unsharding, the + # backend suite produces ONE whole-suite lcov upload (flags: backend) instead of the former 3 shard + # uploads this threshold was sized for. It must stay 1, not 2, even though review-enrichment / + # control-plane add their own flag uploads on PRs that touch them -- those uploads only exist on such + # PRs, and a floor above the guaranteed minimum would leave codecov/patch permanently un-posted on + # every other PR. The narrow premature-window this reintroduces (a rees/control-plane upload landing + # and posting before the ~25-minute backend run finishes) self-corrects on the backend upload, and + # require_ci_to_pass above still holds the final verdict to the run's own conclusion. + after_n_builds: 1 coverage: status: diff --git a/scripts/compute-test-shards.ts b/scripts/compute-test-shards.ts deleted file mode 100644 index 18c6b34874..0000000000 --- a/scripts/compute-test-shards.ts +++ /dev/null @@ -1,146 +0,0 @@ -#!/usr/bin/env node -// Bin-packs the full test/**/*.test.ts file set into N balanced shards by historical duration (greedy -// LPT -- Longest Processing Time first: sort files descending by duration, repeatedly assign the next -// file to whichever shard currently has the smallest total), replacing vitest's own --shard (file -// COUNT only, no duration awareness -- confirmed via real per-shard CI timing data to produce a -// consistent ~20-30% gap between the slowest and fastest shard, every sampled run at the time). -// -// Deliberately scoped to the full-suite case only -- see ci.yml's "Test with coverage" step for how -// this output is (and is NOT) consumed. A PR using scoped test selection (--changed=origin/main) -// keeps vitest's own native --shard: that file set isn't known until vitest resolves --changed -// itself, so a precomputed assignment can't apply to it without duplicating vitest's own dependency- -// graph resolution here. -// -// HARD INVARIANT, checked before any output is written: the union of every shard's file list must -// equal EXACTLY the discovered test file set, with no file in more than one shard. A violation means -// this script would make CI silently never run some test file at all -- exactly the failure class that -// this whole session's Codecov-enforcement work exists to prevent, just introduced by the tool meant -// to speed that same pipeline up. This refuses to write ANY output if the invariant doesn't hold, -// rather than risk it: a hard failure here (a broken CI step everyone sees) is a wildly better outcome -// than a silent one (missing coverage nobody notices until something ships broken). - -import { existsSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; -import { join } from "node:path"; - -const TEST_ROOT = "test"; -const EXCLUDED_DIR = join(TEST_ROOT, "workers"); // mirrors vitest.config.ts's exclude: ["test/workers/**/*.test.ts"] - -const shardsArg = Number(process.argv.find((a) => a.startsWith("--shards="))?.split("=")[1] ?? 3); -const timingArg = process.argv.find((a) => a.startsWith("--timing="))?.split("=")[1]; -const outputArg = process.argv.find((a) => a.startsWith("--output="))?.split("=")[1]; -if (!outputArg) throw new Error("--output= is required"); - -function discoverTestFiles(dir: string): string[] { - const results: string[] = []; - for (const entry of readdirSync(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) { - if (full === EXCLUDED_DIR) continue; - results.push(...discoverTestFiles(full)); - } else if (entry.name.endsWith(".test.ts")) { - results.push(full); - } - } - return results; -} - -function loadTimingData(path: string | undefined): Record { - if (!path || !existsSync(path)) return {}; - const parsed = JSON.parse(readFileSync(path, "utf8")); - return parsed.averageSecondsByFile ?? {}; -} - -function median(values: readonly number[]): number { - if (values.length === 0) return 0; - const sorted = [...values].sort((a, b) => a - b); - const mid = Math.floor(sorted.length / 2); - return sorted.length % 2 === 0 ? (sorted[mid - 1]! + sorted[mid]!) / 2 : sorted[mid]!; -} - -type Shard = { index: number; files: string[]; total: number }; - -function packShards(files: readonly string[], durationByFile: Record, shardCount: number): Shard[] { - // New/untracked files (no historical row -- a file added since the last timing refresh, or the - // refresh workflow hasn't run yet at all) get the median of known files' durations rather than 0: - // treating an unknown file as free would let a burst of newly-added heavy test files land in the - // same shard unbalanced, silently reintroducing the exact imbalance this script exists to remove. - // If NO file has any known duration at all (the real state before the first timing refresh has ever - // run), median() of an empty array is 0 -- every file would tie at the same weight, which matters - // below, not just as an edge case: see the tie-break comment. - const knownDurations = Object.values(durationByFile); - const fallback = median(knownDurations); - - const weighted = files.map((file) => ({ file, duration: durationByFile[file] ?? fallback })).sort((a, b) => b.duration - a.duration); - - const shards: Shard[] = Array.from({ length: shardCount }, (_unused, index) => ({ index, files: [], total: 0 })); - // Picking the lightest shard via a plain reduce always resolves ties in favor of the FIRST shard - // (shard.total < min.total is never true between two equal totals, so the running minimum never - // moves off its starting candidate) -- harmless when durations vary, but catastrophic whenever many - // files tie at the same weight: every one of them would collapse onto shard 1 while the rest stay - // empty. This is the real, common case, not a theoretical one -- it's exactly what happens on this - // script's very first run, before any timing data has ever been fetched (every file falls back to - // the same value, verified by running this script with no --timing argument against this repo's - // real ~1000 test files: without the rotation below, 100% of them landed in shard 1). Rotating the - // tie-break starting point after every assignment makes N equal-weight files distribute round-robin - // across all shards instead, regardless of whether the tied weight happens to be zero or not. - let tiebreakStart = 0; - for (const { file, duration } of weighted) { - let lightest = shards[tiebreakStart]!; - for (let offset = 1; offset < shardCount; offset += 1) { - const candidate = shards[(tiebreakStart + offset) % shardCount]!; - if (candidate.total < lightest.total) lightest = candidate; - } - lightest.files.push(file); - lightest.total += duration; - tiebreakStart = (lightest.index + 1) % shardCount; - } - return shards; -} - -function assertInvariant(files: readonly string[], shards: readonly Shard[]): void { - const original = new Set(files); - const seen = new Set(); - const duplicates: string[] = []; - for (const shard of shards) { - for (const file of shard.files) { - if (seen.has(file)) duplicates.push(file); - seen.add(file); - } - } - const missing = files.filter((file) => !seen.has(file)); - const extra = [...seen].filter((file) => !original.has(file)); - - if (missing.length > 0 || extra.length > 0 || duplicates.length > 0) { - const details = [ - missing.length > 0 ? `missing from every shard: ${JSON.stringify(missing)}` : null, - duplicates.length > 0 ? `assigned to more than one shard: ${JSON.stringify(duplicates)}` : null, - extra.length > 0 ? `assigned but not in the discovered file set: ${JSON.stringify(extra)}` : null, - ] - .filter(Boolean) - .join("; "); - throw new Error(`compute-test-shards: shard-assignment invariant violated -- ${details}`); - } -} - -if (!existsSync(TEST_ROOT)) { - throw new Error(`compute-test-shards: ${TEST_ROOT}/ does not exist -- run this from the repo root`); -} -const files = discoverTestFiles(TEST_ROOT).sort(); // sorted for deterministic ordering before weighting -if (files.length === 0) throw new Error(`compute-test-shards: discovered zero test files under ${TEST_ROOT}/ -- refusing to write an empty assignment`); - -const durationByFile = loadTimingData(timingArg); -const shards = packShards(files, durationByFile, shardsArg); -assertInvariant(files, shards); - -const assignment: Record = {}; -shards.forEach((shard, index) => { - assignment[String(index + 1)] = shard.files; -}); - -writeFileSync(outputArg, JSON.stringify(assignment)); - -const knownCount = files.filter((f) => f in durationByFile).length; -console.log(`Assigned ${files.length} files to ${shardsArg} shards (${knownCount} with known timing, ${files.length - knownCount} using the fallback estimate).`); -shards.forEach((shard, index) => { - console.log(` shard ${index + 1}: ${shard.files.length} files, ~${shard.total.toFixed(1)}s estimated`); -}); diff --git a/scripts/fetch-test-timing.ts b/scripts/fetch-test-timing.ts deleted file mode 100644 index f9cfda6115..0000000000 --- a/scripts/fetch-test-timing.ts +++ /dev/null @@ -1,148 +0,0 @@ -#!/usr/bin/env node -// Fetches per-test-file historical duration data from Codecov's Test Analytics API and aggregates it -// into a per-file average, for the test-shard bin-packer (scripts/compute-test-shards.ts) to consume. -// Codecov already ingests a JUnit report per shard on every push to main (see ci.yml's coverage-upload -// steps, report_type: test_results) and pools it across runs -- this reads that pooled history back -// out instead of this repo tracking its own duration history from scratch. -// -// Filtered to branch=main deliberately: a PR's own JUnit upload is override_branch'd to that PR's own -// branch name (see ci.yml's upload steps), not "main" -- so branch=main naturally selects only -// push-triggered, full-unscoped-suite runs, which is exactly the population the shard bin-packer needs -// (duration-aware sharding only applies to the full-suite case; see compute-test-shards.ts). -// -// Requires a Codecov personal API access token (Codecov Settings -> Access -> Generate Token), NOT the -// existing CODECOV_TOKEN secret -- that one is an upload-only token and doesn't authenticate this read -// API. Codecov's docs don't publish a numeric rate limit for this endpoint, so this is deliberately run -// on a schedule (test-timing-refresh.yml), not per-PR. -// -// Uses /test-analytics/, not /test-results/: the latter is now deprecated (confirmed by actually -// running this script -- Codecov returns a 301 with "This endpoint has been deprecated. Please use -// /test-analytics/ instead."). Verified via Codecov's live OpenAPI schema (api.codecov.io/api/v2/schema/) -// that /test-analytics/ returns the identical PaginatedTestrunList wrapper and Testrun field shape -// (filename, duration_seconds, commit_sha, etc.) -- a URL rename, not a schema change, so nothing else -// in this file needed to change. -// -// #7457: aggregateByFile (and the retry-decision helper) are named exports so unit tests can cover the -// per-commit-then-across-commits averaging without hitting the live Codecov driver. That driver runs -// only when this file is the process entrypoint. - -import { writeFileSync } from "node:fs"; -import { pathToFileURL } from "node:url"; - -export type CodecovTestRunRow = { - filename?: string | null | undefined; - commit_sha?: string | null | undefined; - duration_seconds?: number | null | undefined; -}; - -type CodecovTestAnalyticsPage = { - results: CodecovTestRunRow[]; - next: string | null; -}; - -export const RETRYABLE_STATUS: ReadonlySet = new Set([429, 500, 502, 503, 504]); - -/** True when a Codecov response status should be retried (and attempt budget remains). */ -export function shouldRetryCodecovFetch(status: number, attempt: number, maxAttempts: number): boolean { - return RETRYABLE_STATUS.has(status) && attempt < maxAttempts; -} - -export function aggregateByFile(rows: readonly CodecovTestRunRow[]): Record { - // Per-file duration must be averaged ACROSS RUNS, not just summed across every row: a file with many - // test cases would otherwise dwarf a file with few, and a file that appears in many historical rows - // (many runs) would inflate further with each additional run pooled in -- neither reflects "how long - // does this file actually take in a single run." So first sum each file's rows *within* a single - // commit (that commit's real per-run file duration), then average those per-commit totals across all - // commits the file appears in. - const perCommitTotals = new Map>(); - for (const row of rows) { - if (!row.filename || row.duration_seconds == null) continue; - if (!perCommitTotals.has(row.filename)) perCommitTotals.set(row.filename, new Map()); - const commits = perCommitTotals.get(row.filename)!; - commits.set(row.commit_sha, (commits.get(row.commit_sha) ?? 0) + row.duration_seconds); - } - - const averages: Record = {}; - for (const [filename, commits] of perCommitTotals) { - const totals = [...commits.values()]; - averages[filename] = totals.reduce((sum, value) => sum + value, 0) / totals.length; - } - return averages; -} - -async function fetchWithRetry(url: string, token: string, maxAttempts = 4): Promise { - for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { - const response = await fetch(url, { - headers: { Authorization: `bearer ${token}`, Accept: "application/json" }, - }); - if (response.ok) return response.json() as Promise; - if (!shouldRetryCodecovFetch(response.status, attempt, maxAttempts)) { - throw new Error(`Codecov API error ${response.status} on ${url}: ${await response.text()}`); - } - const delayMs = 2 ** attempt * 1000; - console.warn(`Codecov API returned ${response.status} (attempt ${attempt}/${maxAttempts}), retrying in ${delayMs}ms`); - await new Promise((resolve) => setTimeout(resolve, delayMs)); - } - throw new Error("unreachable"); -} - -async function fetchAllTestRuns({ - owner, - repoName, - token, - maxPages, -}: { - owner: string; - repoName: string; - token: string; - maxPages: number; -}): Promise<{ rows: CodecovTestRunRow[]; truncated: boolean }> { - const rows: CodecovTestRunRow[] = []; - let url: string | null = `https://api.codecov.io/api/v2/gh/${owner}/repos/${repoName}/test-analytics/?branch=main&page_size=100`; - let pages = 0; - while (url && pages < maxPages) { - const body = await fetchWithRetry(url, token); - rows.push(...body.results); - url = body.next; - pages += 1; - } - return { rows, truncated: url !== null }; -} - -async function main() { - const maxPages = Number(process.argv.find((a) => a.startsWith("--max-pages="))?.split("=")[1] ?? 20); - const outputArg = process.argv.find((a) => a.startsWith("--output=")); - const outputPath = outputArg ? outputArg.split("=")[1] : null; - - const repo = process.env.GITHUB_REPOSITORY; - if (!repo) throw new Error("GITHUB_REPOSITORY is required (e.g. JSONbored/loopover)"); - const [owner, repoName] = repo.split("/"); - const token = process.env.CODECOV_API_TOKEN; - if (!token) throw new Error("CODECOV_API_TOKEN is required"); - - const { rows, truncated } = await fetchAllTestRuns({ owner: owner!, repoName: repoName!, token, maxPages }); - const averageSecondsByFile = aggregateByFile(rows); - - const report = { - fetchedAt: new Date().toISOString(), - sourceRowCount: rows.length, - fileCount: Object.keys(averageSecondsByFile).length, - truncated, // true if MAX_PAGES was hit before the API ran out of pages -- more history existed than was pulled - averageSecondsByFile, - }; - - const json = JSON.stringify(report, null, 2); - if (outputPath) { - writeFileSync(outputPath, json); - console.log(`Wrote ${report.fileCount} files' timing data (from ${report.sourceRowCount} rows) to ${outputPath}`); - } else { - process.stdout.write(`${json}\n`); - } -} - -if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) { - main().catch((err) => { - console.error(err); - process.exit(1); - }); -} diff --git a/test/unit/codecov-policy.test.ts b/test/unit/codecov-policy.test.ts index f306938ce6..e965b6d3c6 100644 --- a/test/unit/codecov-policy.test.ts +++ b/test/unit/codecov-policy.test.ts @@ -75,7 +75,7 @@ describe("Codecov policy", () => { expect(String(testResultsUpload.if)).toContain("no_src_coverage != 'true'"); // The scoped branch must derive BOTH outputs from the run itself (vitest's own stdout + exit status), // never from re-deriving selection logic -- pin the detector lines so they can't silently vanish. - const coverageStep = steps[stepNames.indexOf("Test with coverage (shard ${{ matrix.shard }}/3)")]!; + const coverageStep = steps[stepNames.indexOf("Test with coverage")]!; expect(String(coverageStep.run)).toContain('grep -q "No test files found" vitest-scoped-output.log'); expect(String(coverageStep.run)).toContain('echo "no_src_coverage=true"'); expect(String(verifyStep.run)).toContain("coverage/lcov.info is missing or empty"); @@ -254,8 +254,9 @@ describe("Codecov policy", () => { const validateTests = nestedRecord(workflow, ["jobs", "validate-tests"]); expect(String(validateTests.if)).toContain("needs.changes.outputs.rees == 'true'"); - const validateTestsMerge = nestedRecord(workflow, ["jobs", "validate-tests-merge"]); - expect(String(validateTestsMerge.if)).toContain("needs.changes.outputs.rees == 'true'"); + // Unsharded (2026-07-24): the merge job is gone -- the whole-suite threshold runs inline in + // validate-tests, so the only invariant left to pin is that no stale merge job lingers. + expect((workflow as { jobs?: Record }).jobs?.["validate-tests-merge"]).toBeUndefined(); }); it("captures control-plane node:test coverage for Codecov (#7743)", () => { @@ -295,7 +296,5 @@ describe("Codecov policy", () => { const validateTests = nestedRecord(workflow, ["jobs", "validate-tests"]); expect(String(validateTests.if)).toContain("needs.changes.outputs.controlPlane == 'true'"); - const validateTestsMerge = nestedRecord(workflow, ["jobs", "validate-tests-merge"]); - expect(String(validateTestsMerge.if)).toContain("needs.changes.outputs.controlPlane == 'true'"); }); }); diff --git a/test/unit/compute-test-shards.test.ts b/test/unit/compute-test-shards.test.ts deleted file mode 100644 index e4dd73f31e..0000000000 --- a/test/unit/compute-test-shards.test.ts +++ /dev/null @@ -1,119 +0,0 @@ -import { spawnSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; - -const SCRIPT = join(process.cwd(), "scripts/compute-test-shards.ts"); - -function run(args: string[]) { - return spawnSync("node", ["--experimental-strip-types", SCRIPT, ...args], { cwd: process.cwd(), encoding: "utf8" }); -} - -function discoverRealTestFiles(): string[] { - // Independent re-implementation (not importing the script's own discoverTestFiles) so this test - // can't pass merely because both sides share the same bug. - const results: string[] = []; - function walk(dir: string) { - for (const entry of readdirSync(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) { - if (full === join("test", "workers")) continue; - walk(full); - } else if (entry.name.endsWith(".test.ts")) { - results.push(full); - } - } - } - walk("test"); - return results; -} - -let tmpDir: string; -afterEach(() => { - if (tmpDir) rmSync(tmpDir, { recursive: true, force: true }); -}); - -describe("compute-test-shards.ts", () => { - it("with no timing data, splits the real repo's test files evenly across shards (round-robin fallback)", () => { - tmpDir = mkdtempSync(join(tmpdir(), "shard-test-")); - const outputPath = join(tmpDir, "assignment.json"); - const result = run([`--shards=6`, `--output=${outputPath}`]); - expect(result.status, result.stderr).toBe(0); - - const assignment = JSON.parse(readFileSync(outputPath, "utf8")) as Record; - const shardCounts = Object.values(assignment).map((files) => files.length); - const expected = discoverRealTestFiles(); - const total = shardCounts.reduce((sum, count) => sum + count, 0); - - expect(total).toBe(expected.length); - // Regression test (#ci-duration-aware-sharding): before the tie-break rotation fix, every file with - // an identical (fallback) duration collapsed onto shard 1, leaving shards 2-6 completely empty -- - // the real, common case here (no timing data has ever been fetched yet). Assert every shard got a - // roughly even share, not just that the total is right. - for (const count of shardCounts) { - expect(count).toBeGreaterThan(Math.floor(expected.length / 6) - 2); - expect(count).toBeLessThan(Math.ceil(expected.length / 6) + 2); - } - }); - - it("the union of all shards exactly equals the discovered file set, with no file duplicated", () => { - tmpDir = mkdtempSync(join(tmpdir(), "shard-test-")); - const outputPath = join(tmpDir, "assignment.json"); - const result = run([`--shards=6`, `--output=${outputPath}`]); - expect(result.status, result.stderr).toBe(0); - - const assignment = JSON.parse(readFileSync(outputPath, "utf8")) as Record; - const allAssigned = Object.values(assignment).flat(); - const expected = discoverRealTestFiles(); - - expect(new Set(allAssigned).size).toBe(allAssigned.length); // no duplicates - expect(new Set(allAssigned)).toEqual(new Set(expected)); // exact set match, nothing missing or extra - }); - - it("balances weighted (real-shaped) synthetic durations far more evenly than an unweighted split would", () => { - tmpDir = mkdtempSync(join(tmpdir(), "shard-test-")); - const timingPath = join(tmpDir, "timing.json"); - const outputPath = join(tmpDir, "assignment.json"); - - const files = discoverRealTestFiles(); - const averageSecondsByFile: Record = {}; - files.forEach((file, index) => { - // A handful of heavy outliers (every 47th file), everything else small -- shaped like a real - // suite (most files fast, a few slow ones dominate wall-clock if they land in the same shard). - averageSecondsByFile[file] = index % 47 === 0 ? 20 : 0.5; - }); - writeFileSync(timingPath, JSON.stringify({ averageSecondsByFile })); - - const result = run([`--shards=6`, `--timing=${timingPath}`, `--output=${outputPath}`]); - expect(result.status, result.stderr).toBe(0); - - const assignment = JSON.parse(readFileSync(outputPath, "utf8")) as Record; - const shardTotals = Object.values(assignment).map((shardFiles) => - shardFiles.reduce((sum, file) => sum + (averageSecondsByFile[file] ?? 0), 0), - ); - const maxTotal = Math.max(...shardTotals); - const minTotal = Math.min(...shardTotals); - // LPT bin-packing's worst-case bound is well under 34% over optimal; require well inside that, - // since real per-file variance here is much smaller than the pathological cases that bound covers. - expect((maxTotal - minTotal) / maxTotal).toBeLessThan(0.15); - }); - - it("refuses to write output when test/ exists but contains zero test files", () => { - tmpDir = mkdtempSync(join(tmpdir(), "shard-test-")); - const outputPath = join(tmpDir, "assignment.json"); - mkdirSync(join(tmpDir, "test")); - const result = spawnSync("node", [SCRIPT, "--shards=6", `--output=${outputPath}`], { cwd: tmpDir, encoding: "utf8" }); - expect(result.status).not.toBe(0); - expect(result.stderr).toContain("discovered zero test files"); - }); - - it("fails with a clear error, not a raw ENOENT stack trace, when test/ doesn't exist at all", () => { - tmpDir = mkdtempSync(join(tmpdir(), "shard-test-")); - const outputPath = join(tmpDir, "assignment.json"); - const result = spawnSync("node", [SCRIPT, "--shards=6", `--output=${outputPath}`], { cwd: tmpDir, encoding: "utf8" }); - expect(result.status).not.toBe(0); - expect(result.stderr).toContain("does not exist -- run this from the repo root"); - expect(result.stderr).not.toContain("ENOENT"); - }); -}); diff --git a/test/unit/fetch-test-timing-script.test.ts b/test/unit/fetch-test-timing-script.test.ts deleted file mode 100644 index 20e05b7072..0000000000 --- a/test/unit/fetch-test-timing-script.test.ts +++ /dev/null @@ -1,84 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - aggregateByFile, - RETRYABLE_STATUS, - shouldRetryCodecovFetch, -} from "../../scripts/fetch-test-timing.js"; - -describe("fetch-test-timing.ts (#7457)", () => { - describe("aggregateByFile", () => { - it("sums multiple rows for the same file within one commit before averaging across commits", () => { - // Two test cases in commit A (2s + 3s = 5s that run), one case in commit B (1s). Average across - // commits must be (5 + 1) / 2 = 3 — NOT the flat mean of the three rows ((2+3+1)/3 = 2), which would - // be the "many test cases dwarf few" failure mode the function's header comment warns about. - const averages = aggregateByFile([ - { filename: "test/unit/a.test.ts", commit_sha: "aaa", duration_seconds: 2 }, - { filename: "test/unit/a.test.ts", commit_sha: "aaa", duration_seconds: 3 }, - { filename: "test/unit/a.test.ts", commit_sha: "bbb", duration_seconds: 1 }, - ]); - - expect(averages).toEqual({ "test/unit/a.test.ts": 3 }); - }); - - it("averages the same file across commits instead of summing every historical row", () => { - // Same file in three commits at 10s each: average is 10, not 30 (the "many historical rows inflate - // further with every pooled run" failure mode). - const averages = aggregateByFile([ - { filename: "test/unit/b.test.ts", commit_sha: "c1", duration_seconds: 10 }, - { filename: "test/unit/b.test.ts", commit_sha: "c2", duration_seconds: 10 }, - { filename: "test/unit/b.test.ts", commit_sha: "c3", duration_seconds: 10 }, - ]); - - expect(averages).toEqual({ "test/unit/b.test.ts": 10 }); - }); - - it("skips rows with a missing filename or null/undefined duration_seconds", () => { - const averages = aggregateByFile([ - { filename: "", commit_sha: "c1", duration_seconds: 5 }, - { filename: null, commit_sha: "c1", duration_seconds: 5 }, - { filename: undefined, commit_sha: "c1", duration_seconds: 5 }, - { filename: "test/unit/c.test.ts", commit_sha: "c1", duration_seconds: null }, - { filename: "test/unit/c.test.ts", commit_sha: "c1", duration_seconds: undefined }, - { filename: "test/unit/c.test.ts", commit_sha: "c1", duration_seconds: 4 }, - { filename: "test/unit/c.test.ts", commit_sha: "c2", duration_seconds: 6 }, - ]); - - expect(averages).toEqual({ "test/unit/c.test.ts": 5 }); - }); - - it("aggregates independent files without cross-contaminating their averages", () => { - const averages = aggregateByFile([ - { filename: "test/unit/fast.test.ts", commit_sha: "c1", duration_seconds: 1 }, - { filename: "test/unit/slow.test.ts", commit_sha: "c1", duration_seconds: 2 }, - { filename: "test/unit/slow.test.ts", commit_sha: "c1", duration_seconds: 2 }, - { filename: "test/unit/slow.test.ts", commit_sha: "c2", duration_seconds: 8 }, - ]); - - expect(averages).toEqual({ - "test/unit/fast.test.ts": 1, - "test/unit/slow.test.ts": 6, // commit c1 summed to 4, then (4 + 8) / 2 - }); - }); - - it("returns an empty object for an empty input", () => { - expect(aggregateByFile([])).toEqual({}); - }); - }); - - describe("shouldRetryCodecovFetch", () => { - it("retries the documented transient statuses while attempts remain", () => { - for (const status of RETRYABLE_STATUS) { - expect(shouldRetryCodecovFetch(status, 1, 4)).toBe(true); - expect(shouldRetryCodecovFetch(status, 3, 4)).toBe(true); - } - }); - - it("does not retry a non-retryable status or the final attempt", () => { - expect(shouldRetryCodecovFetch(400, 1, 4)).toBe(false); - expect(shouldRetryCodecovFetch(401, 1, 4)).toBe(false); - expect(shouldRetryCodecovFetch(404, 1, 4)).toBe(false); - expect(shouldRetryCodecovFetch(429, 4, 4)).toBe(false); - expect(shouldRetryCodecovFetch(500, 4, 4)).toBe(false); - }); - }); -}); From 5d96270d94407864e45dbf75685b79ddce54b706 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:15:56 -0700 Subject: [PATCH 2/2] test(ci): update the CI-topology pins for the unsharded validate-tests job Four tests across workflow-runner-labels, ci-skip-draft-prs, and ci-composite-setup-workspace still pinned the 3-shard shape (the merge job in validate's needs, the merge job's draft-skip condition, and its save-cache:false composite call). All now pin the single-job topology; codecov-policy already pins the merge job's absence. --- test/unit/ci-composite-setup-workspace.test.ts | 5 ++--- test/unit/ci-dependency-cache.test.ts | 2 +- test/unit/ci-skip-draft-prs.test.ts | 10 +++++----- test/unit/workflow-runner-labels.test.ts | 13 +++++-------- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/test/unit/ci-composite-setup-workspace.test.ts b/test/unit/ci-composite-setup-workspace.test.ts index 5a84239311..fdc5109d60 100644 --- a/test/unit/ci-composite-setup-workspace.test.ts +++ b/test/unit/ci-composite-setup-workspace.test.ts @@ -31,8 +31,8 @@ function jobSteps(workflow: Record, jobName: string): Array { it.each([ { job: "validate-code", saveCache: undefined }, { job: "validate-tests", saveCache: undefined }, - { job: "validate-tests-merge", saveCache: "false" }, ])("$job invokes the composite action (save-cache: $saveCache)", ({ job, saveCache }) => { const steps = jobSteps(readYaml(".github/workflows/ci.yml"), job); diff --git a/test/unit/ci-dependency-cache.test.ts b/test/unit/ci-dependency-cache.test.ts index 0e02791e05..da42e35c07 100644 --- a/test/unit/ci-dependency-cache.test.ts +++ b/test/unit/ci-dependency-cache.test.ts @@ -31,7 +31,7 @@ function jobSteps(workflow: Record, jobName: string): Array { return value as Record; } -// Regression guard: draft PRs were consuming the exact same 11-job GitHub-hosted-runner fan-out as ready -// PRs (validate-code, the 6-way validate-tests shard matrix, validate-tests-merge, security), which both +// Regression guard: draft PRs were consuming the exact same heavy GitHub-hosted-runner fan-out as ready +// PRs (validate-code, the full-suite validate-tests coverage run, security), which both // (a) let contributors farm bot labels/AI review/screenshots for free while sitting in draft, and (b) // materially worsened the account's shared-runner queue contention for every other PR in flight. The heavy // jobs now require the PR not be a draft; the cheap `changes` and `validate` (result-aggregation) jobs still @@ -29,7 +29,7 @@ describe("ci.yml skips the heavy jobs for draft pull requests", () => { expect(pullRequest.types).toEqual(["opened", "synchronize", "reopened", "ready_for_review"]); }); - it.each(["validate-code", "validate-tests", "validate-tests-merge"])( + it.each(["validate-code", "validate-tests"])( "%s's if-condition requires github.event.pull_request.draft != true alongside the existing push/path-filter checks", (jobName) => { const job = record(jobs[jobName], `jobs.${jobName}`); @@ -45,9 +45,9 @@ describe("ci.yml skips the heavy jobs for draft pull requests", () => { expect(String(job.if)).toBe("${{ github.event_name == 'pull_request' && github.event.pull_request.draft != true }}"); }); - it("validate still aggregates all four gated jobs and treats a skipped dependency as success", () => { + it("validate still aggregates all three gated jobs and treats a skipped dependency as success", () => { const job = record(jobs.validate, "jobs.validate"); - expect(job.needs).toEqual(["changes", "validate-code", "validate-tests", "validate-tests-merge", "security"]); + expect(job.needs).toEqual(["changes", "validate-code", "validate-tests", "security"]); expect(String(job.if)).toBe("${{ always() }}"); }); }); diff --git a/test/unit/workflow-runner-labels.test.ts b/test/unit/workflow-runner-labels.test.ts index 9945621df1..268c846259 100644 --- a/test/unit/workflow-runner-labels.test.ts +++ b/test/unit/workflow-runner-labels.test.ts @@ -17,7 +17,7 @@ describe("workflow runner labels", () => { expect(workflow).not.toContain("|| 'self-hosted'"); expect(workflow).not.toContain('"fork-ci"'); expect(workflow).toContain("validate-code:"); - expect(workflow).toContain("needs: [changes, validate-code, validate-tests, validate-tests-merge, security]"); + expect(workflow).toContain("needs: [changes, validate-code, validate-tests, security]"); expect(workflow).not.toContain("\n lint:\n"); expect(workflow).not.toContain("\n test:\n"); expect(workflow).not.toContain("\n workers:\n"); @@ -29,14 +29,11 @@ describe("workflow runner labels", () => { expect(changesJob).toContain("runs-on: ubuntu-latest"); const validateCodeJob = workflow.slice(workflow.indexOf("\n validate-code:\n"), workflow.indexOf("\n validate-tests:\n")); expect(validateCodeJob).toContain("runs-on: ubuntu-latest"); - // validate-tests (#ci-shard-coverage) is the matrix-sharded full-suite coverage run, split out of - // validate-code so the dominant ~9-10min step no longer serializes with the much-faster checks. - const validateTestsJob = workflow.slice(workflow.indexOf("\n validate-tests:\n"), workflow.indexOf("\n validate-tests-merge:\n")); + // validate-tests (#ci-shard-coverage) is the unsharded full-suite coverage run, split out of + // validate-code so the long suite never serializes with the much-faster checks (unsharded again + // 2026-07 -- see the job's header comment in ci.yml; the old merge job is gone with the shards). + const validateTestsJob = workflow.slice(workflow.indexOf("\n validate-tests:\n"), workflow.indexOf("\n security:\n")); expect(validateTestsJob).toContain("runs-on: ubuntu-latest"); - // validate-tests-merge re-checks the global coverage threshold against all shards merged -- see its - // own header comment in ci.yml. - const validateTestsMergeJob = workflow.slice(workflow.indexOf("\n validate-tests-merge:\n"), workflow.indexOf("\n security:\n")); - expect(validateTestsMergeJob).toContain("runs-on: ubuntu-latest"); const securityJob = workflow.slice(workflow.indexOf("\n security:\n"), workflow.indexOf("\n validate:\n")); expect(securityJob).toContain("runs-on: ubuntu-latest"); const validateJob = workflow.slice(workflow.indexOf("\n validate:\n"));