ci: keep a blocking job that exercises the built bundles - #748
Conversation
|
The build fix works. The remaining failure, That is an xAI billing state, not a defect. This PR changes one cast in the Anthropic provider and touches nothing under Two things worth deciding separately, neither blocking this PR:
Generated by Claude Code |
|
Correcting my previous comment — I misidentified this PR. It is the The accurate reading:
Sorry for the noise. Generated by Claude Code |
ced2825 to
97a0b75
Compare
Resolving @workglow/* to src is what makes the coverage numbers mean anything, but the plugin is attached to every project unconditionally — not only to coverage runs — so with the default in force no vitest job resolves a specifier through `exports` at all. There is also no `bun test` job in the blocking workflow. So after the source-resolution change, nothing that can block a merge loads a built bundle: a `bun build` entry that silently dropped a re-export would reach main and surface only in the nightly Bun parity run, which is explicitly informational, runs on a cron, and excludes six sections. Adds test-vitest-dist: reuses the existing build-output artifact and runs the unit tier with WORKGLOW_TEST_TARGET=dist. It is in cleanup's needs list, since cleanup deletes the artifact it downloads. Scoping the plugin to coverage runs instead would not have worked: scripts/test.ts adds --coverage whenever CI is set, so in CI every run is a coverage run and would still resolve to src. Also skips --coverage for a dist-targeted run. The denominator names package sources, so such a run reported all ~1286 of them at 0% — not a measurement of anything, and it is what lets the new job reuse test:vitest:unit unchanged and produce no fragment for merge-vitest-coverage. The CLAUDE.md and vitest.config.ts notes claimed bundle integrity was covered by the nightly parity run; both now say what actually guards it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
The coverage-flag test spawned the runner with `{...process.env, CI: "1"}` and
let WORKGLOW_TEST_TARGET come from the ambient environment. The new
test-vitest-dist job exports that variable for its whole step, so inside that
job the source-target case inherited `dist` and became a second copy of the
dist case — asserting `--coverage` is present while the runner correctly
omitted it. It failed in the one job it was added to support.
Both cases now state the target explicitly, so the assertions hold whatever the
runner is invoked under.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
5df29d6 to
f097116
Compare
e31022f
into
claude/coverage-dist-bundle-fix-ew0vj8
Based on #741 (retarget to
mainonce that merges). Rebased ontoorigin/main(67bed681).What
#741 makes vitest resolve every
@workglow/*specifier to the package'ssrc, which is what makes the coverage numbers mean anything. Butvitest.config.tsattaches the plugin to every project unconditionally — not only to coverage runs — andtestsRunAgainstSourcedefaults tosource.Enumerating the jobs in
.github/workflows/test.yml:typecheck-budget,test-discovery,build, sixtest-vitest-*,merge-vitest-coverage,cleanup. There is nobun testjob in the blocking workflow at all (grep test:bun .github/workflows/test.yml→ nothing). So after #741, no job that can block a merge resolves a@workglow/*specifier throughexports. Abun buildentry that silently dropped a re-export reachesmainand surfaces only innightly-bun-parity.yml, which states in its own header that "Failures here are informational. They never block a merge", runs on a cron, and excludesrag,browser,provider-hft,provider-nodellama,provider-api,provider-cactus.The
.claude/CLAUDE.mdsentence and the matchingvitest.config.tsJSDoc both asserted that bundle integrity "is not left unguarded by the default" because of that nightly run. Wrong on both counts — it is not blocking, and it is not the whole suite.Why this fix
test-vitest-dist, a blocking job —needs: build, downloads the existingbuild-outputartifact, runsbun run test:vitest:unitwithWORKGLOW_TEST_TARGET: dist. It reuses the artifact the six existing jobs already share, so it costs a runner, not a build.scripts/test.tsskips--coveragewhen targetingdist. The denominator namespackages/*/srcandproviders/*/src, so a bundle-targeted run reported all ~1286 source files at 0% — not a measurement of anything, and merging such a fragment would drag the total toward zero for reasons unrelated to how well the tree is tested. Skipping it is also what lets the new job reusetest:vitest:unitverbatim and produce no fragment, somerge-vitest-coverageneeds no change..claude/CLAUDE.mdand in thevitest.config.tsJSDoc that repeats it.Explicitly rejected alternative — "scope the plugin to coverage runs only". It does not fix this.
scripts/test.tsadds--coveragewheneverCIis set, so in CI every run is a coverage run and would still resolve tosrc. The gap would be exactly as wide, with the config now implying otherwise.Beyond the original plan:
test-vitest-distwas added tocleanup'sneedslist.cleanupdeletes thebuild-outputartifact, and without that edge it could race a still-downloadingtest-vitest-dist.What the first
test-vitest-distrun actually foundThe bundles are fine. The job ran the entire unit tier against built output and reported:
5879 tests passed against
dist. The single failure was this PR's own new test, not a wiring defect:Root cause: the test spawned the runner with
{...process.env, CI: "1"}and letWORKGLOW_TEST_TARGETcome from the ambient environment. Thetest-vitest-distjob exports that variable for the whole step, so inside that job the "ordinary CI run" case inheriteddistand became a second copy of the dist case — asserting--coverageis present while the runner correctly omitted it. The test failed in the one job it was added to support.Fixed by pinning the variable explicitly in both cases (
"source"/"dist") rather than inheriting it. Verified by re-running under the job's exact ambient environment:So the job is working exactly as intended: it ran the tier against the bundles, and the only thing it caught was an environment-dependent assumption in a brand-new test. That is a better first result than a green run would have been.
The other red job on this run,
test-vitest-ai-provider-api, is a pre-existing job this PR does not touch.Tests
scripts/testRunnerArgs.test.ts(2 cases), spawning the runner in--dry-runmode, which prints the command it would have spawned:CI=1, targetsource→ command contains--coverage(the baseline)CI=1, targetdist→ command does not contain--coverageDeliberately a vitest file, not an addition to
scripts/test.test.ts— that one importsbun:test, and a regression test for "blocking CI does not cover X" that itself does not run in blocking CI would be self-defeating.scriptsis a real vitest project, so this runs intest:vitest:unit.Actually executed locally:
vitest --project scripts: 4 files, 16 passed; and 2 passed underWORKGLOW_TEST_TARGET=dist CI=1.scripts/test.tsreverted: 1 failed / 15 passed — the dist case, as expected..github/workflows/test.ymlparsed with a YAML loader: 12 jobs,test-vitest-distpresent withneeds: buildandWORKGLOW_TEST_TARGET: dist,cleanup.needsincludes it,merge-vitest-coverage.needsunchanged.prettier --checkclean.Executed in CI: the new job itself — see above.
Risk / blast radius
One extra CI job, running the full unit tier a second time. From the first run: ~3m20s wall clock (03:04:05 → 03:07:26), comparable to the existing tiers and fully parallel with them.
If that ever proves unacceptable, the cheaper follow-up is a single
dist-targeted suite that dynamically imports every runtime entry fromstubSpecsForand asserts each bundle namespace's export names are a superset of the source module's — narrower and aimed exactly at "abun buildentry silently dropped a re-export". Flagged as follow-up rather than primary because importing every entry has real side-effect and native-module hazards under Node, which the full tier does not.A local
CI=1 WORKGLOW_TEST_TARGET=distrun no longer writescoverage/. Nothing in the workflow reads it on that path.The coverage denominator itself is untouched here — that is #749, deliberately separate, so the coverage delta stays readable.
Unverified
buildfailure inherited frommain(see the note at the top) is not diagnosed or fixed here.