Skip to content

Commit 71646d3

Browse files
committed
ci: restore the global coverage-threshold backstop across sharded coverage
The AI review agent correctly flagged that sharding disabled vitest.config.ts's global 90% coverage-threshold check everywhere in CI (COVERAGE_NO_THRESHOLDS=true on every shard, unconditionally), leaving only Codecov's patch gate -- which only covers changed lines, not a whole-repo regression outside the diff (e.g. a deleted test file). Add a validate-tests-merge job: each shard now also writes a vitest blob report (--reporter=blob) and uploads it as a build artifact; validate-tests-merge downloads all 4, merges them via vitest's own --mergeReports, and re-runs the global threshold check (without COVERAGE_NO_THRESHOLDS) against the combined whole-suite result -- restoring the exact backstop the old single unsharded job had.
1 parent 12b76ec commit 71646d3

2 files changed

Lines changed: 77 additions & 5 deletions

File tree

.github/workflows/ci.yml

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,11 @@ jobs:
438438
- name: Test with coverage (shard ${{ matrix.shard }}/4)
439439
id: coverage
440440
env:
441-
VITEST_JUNIT_PATH: reports/junit/vitest.xml
441+
# Disables vitest.config.ts's own global 90% threshold check for THIS per-shard invocation -- a
442+
# single shard only exercises part of the tree, so it would always false-fail. The global-threshold
443+
# "catastrophe net" this disables is NOT dropped from CI, though: validate-tests-merge (below) merges
444+
# all 4 shards' coverage via vitest's own --mergeReports and re-checks the threshold against that
445+
# merged (whole-suite) total, without COVERAGE_NO_THRESHOLDS set.
442446
COVERAGE_NO_THRESHOLDS: "true"
443447
# Same self-contained-test-file narrowing as the pre-sharding job had -- see the `mcpCliHarness`/
444448
# `minerTestHarness` filter comments in the `changes` job above for the full rationale.
@@ -460,7 +464,7 @@ jobs:
460464
--exclude "test/unit/miner-calibration-types.test.ts"
461465
)
462466
fi
463-
npm run test:coverage -- --maxWorkers=4 --shard=${{ matrix.shard }}/4 "${EXCLUDE_ARGS[@]}"
467+
npm run test:coverage -- --maxWorkers=4 --shard=${{ matrix.shard }}/4 --reporter=default --reporter=blob --reporter=junit --outputFile.blob=blob-report/report.blob --outputFile.junit=reports/junit/vitest.xml "${EXCLUDE_ARGS[@]}"
464468
- name: Test failure guidance
465469
if: ${{ failure() && steps.coverage.conclusion == 'failure' }}
466470
run: |
@@ -474,6 +478,15 @@ jobs:
474478
echo "::error title=Coverage::coverage/lcov.info is missing or empty"
475479
exit 1
476480
fi
481+
# Consumed by validate-tests-merge to re-check the global coverage threshold against all 4 shards
482+
# combined -- see this job's own header comment.
483+
- name: Upload coverage blob report
484+
if: ${{ success() }}
485+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
486+
with:
487+
name: coverage-blob-shard-${{ matrix.shard }}
488+
path: blob-report/report.blob
489+
retention-days: 1
477490
# Direct upload for trusted contexts (push + same-repo PRs). Multiple shards' uploads for the same
478491
# commit/PR are additive in Codecov -- see this job's own header comment.
479492
- name: Upload coverage to Codecov
@@ -534,6 +547,61 @@ jobs:
534547
override_pr: ${{ github.event.pull_request.number }}
535548
fail_ci_if_error: false
536549

550+
# Re-checks vitest.config.ts's global 90% coverage threshold against all 4 shards MERGED -- each shard
551+
# above deliberately disables that check for itself (COVERAGE_NO_THRESHOLDS=true), since a single shard's
552+
# partial view would always false-fail it. This is the "loose catastrophe net" (e.g. a deleted test file)
553+
# vitest.config.ts's own comment describes; Codecov's patch gate (changed-lines only) doesn't cover a
554+
# whole-repo regression outside the diff, so this is what actually restores that backstop for CI, using
555+
# vitest's own --mergeReports against each shard's uploaded blob report.
556+
validate-tests-merge:
557+
name: validate-tests-merge
558+
needs: [changes, validate-tests]
559+
if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }}
560+
runs-on: ubuntu-latest
561+
timeout-minutes: 10
562+
steps:
563+
- name: Checkout
564+
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
565+
with:
566+
ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}
567+
- name: Neutralize untrusted npm config
568+
run: rm -f .npmrc
569+
- name: Setup Node
570+
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
571+
with:
572+
node-version-file: .nvmrc
573+
cache: npm
574+
# Same cache key as validate-code/validate-tests -- a cache hit here is the common case since those
575+
# jobs run concurrently and one of them usually wins the race to populate it first.
576+
- name: Restore node_modules cache
577+
id: node-modules-cache
578+
uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
579+
with:
580+
path: |
581+
node_modules
582+
apps/gittensory-ui/node_modules
583+
key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json') }}
584+
- name: Install dependencies (retry on transient failures)
585+
if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }}
586+
run: |
587+
for attempt in 1 2 3; do
588+
if npm ci --prefer-offline --no-audit --no-fund; then
589+
exit 0
590+
fi
591+
echo "::warning::npm ci failed (attempt ${attempt}/3); retrying in 10s"
592+
sleep 10
593+
done
594+
echo "::error::npm ci failed after 3 attempts"
595+
exit 1
596+
- name: Download all shards' blob reports
597+
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
598+
with:
599+
pattern: coverage-blob-shard-*
600+
path: all-blob-reports
601+
merge-multiple: true
602+
- name: Merge shard coverage and check the global threshold
603+
run: npx vitest run --coverage --mergeReports=all-blob-reports
604+
537605
# Diff-scoped security gate: fails only on vulnerabilities this PR introduces.
538606
# Ambient advisories in untouched deps are handled by Renovate + the scheduled
539607
# audit workflow, so one upstream CVE never blocks unrelated PRs.
@@ -563,7 +631,7 @@ jobs:
563631
# Path-filtered jobs report "skipped", which is treated as success.
564632
validate:
565633
name: validate
566-
needs: [changes, validate-code, validate-tests, security]
634+
needs: [changes, validate-code, validate-tests, validate-tests-merge, security]
567635
if: ${{ always() }}
568636
# Pure result-aggregation (reads needs.*.result, echoes pass/fail) -- no build/test work, so it never
569637
# needed the self-hosted pool's cached toolchain (#2507).

test/unit/workflow-runner-labels.test.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ describe("workflow runner labels", () => {
1717
expect(workflow).not.toContain("|| 'self-hosted'");
1818
expect(workflow).not.toContain('"fork-ci"');
1919
expect(workflow).toContain("validate-code:");
20-
expect(workflow).toContain("needs: [changes, validate-code, validate-tests, security]");
20+
expect(workflow).toContain("needs: [changes, validate-code, validate-tests, validate-tests-merge, security]");
2121
expect(workflow).not.toContain("\n lint:\n");
2222
expect(workflow).not.toContain("\n test:\n");
2323
expect(workflow).not.toContain("\n workers:\n");
@@ -31,8 +31,12 @@ describe("workflow runner labels", () => {
3131
expect(validateCodeJob).toContain("runs-on: ubuntu-latest");
3232
// validate-tests (#ci-shard-coverage) is the matrix-sharded full-suite coverage run, split out of
3333
// validate-code so the dominant ~9-10min step no longer serializes with the much-faster checks.
34-
const validateTestsJob = workflow.slice(workflow.indexOf("\n validate-tests:\n"), workflow.indexOf("\n security:\n"));
34+
const validateTestsJob = workflow.slice(workflow.indexOf("\n validate-tests:\n"), workflow.indexOf("\n validate-tests-merge:\n"));
3535
expect(validateTestsJob).toContain("runs-on: ubuntu-latest");
36+
// validate-tests-merge re-checks the global coverage threshold against all 4 shards merged -- see its
37+
// own header comment in ci.yml.
38+
const validateTestsMergeJob = workflow.slice(workflow.indexOf("\n validate-tests-merge:\n"), workflow.indexOf("\n security:\n"));
39+
expect(validateTestsMergeJob).toContain("runs-on: ubuntu-latest");
3640
const securityJob = workflow.slice(workflow.indexOf("\n security:\n"), workflow.indexOf("\n validate:\n"));
3741
expect(securityJob).toContain("runs-on: ubuntu-latest");
3842
const validateJob = workflow.slice(workflow.indexOf("\n validate:\n"));

0 commit comments

Comments
 (0)