Skip to content

Make npm run cov independent of node --test default discovery - #1512

Merged
sergey-shandar merged 5 commits into
mainfrom
claude/epic-fermi-70hknd
Aug 13, 2026
Merged

Make npm run cov independent of node --test default discovery#1512
sergey-shandar merged 5 commits into
mainfrom
claude/epic-fermi-70hknd

Conversation

@sergey-shandar

@sergey-shandar sergey-shandar commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Addresses the Make npm run cov report real coverage item under
Remaining after stage 1 in todo/migrate-typescript-to-mjs.md.

What was actually wrong

The diagnosis recorded in the issue was right: cov passed no path
arguments, so node --test fell back to its default file patterns
(*.test.*, test.*, *-test.*, test-*.*, *_test.*, test/**).
The repo's proofs are proof.f.mjs / module.f.mjs and match none of
them; the only file in the tree that does is
fjs/emergent_testing/all.test.ts. Since that entrypoint is TypeScript,
whether it is discovered depends on the Node version.

The "vacuous for several PRs" framing was the part that did not hold up.
Measured with the pre-fix command
(node --test --experimental-test-coverage --test-coverage-include=**/module.f.ts --test-coverage-include=**/module.f.mjs),
run against real Node tarballs:

Node tests coverage report
22.22.2 2431 yes, all files 99.93
23.11.1 0 none
24.18.1 2431 yes, all files 99.93
26.7.0 2495 yes, all files 99.93

So every version CI runs (22.23.2, 24.18.1, 26.7.0) already reported
real coverage. v23 — released, EOL, not in CI — is the version that reports
nothing, which is what the fragility looks like when it bites.

The change

  • package.json: cov names fjs/emergent_testing/all.test.ts explicitly,
    so the signal no longer depends on discovery defaults, and drops the dead
    --test-coverage-include=**/module.f.ts glob — no authored .f.ts
    remains after stage 1.
  • deno.json: the same dead .f.ts alternative leaves the Deno coverage
    filter.
  • fjs/ci/deno/module.f.mjs, its proof, fjs/ci/proof.f.mjs and the
    generated .github/workflows/ci.yml: the Deno job now runs
    deno task cov instead of inlining
    deno test … && deno coverage --include=…, so deno.json is the single
    owner of the permission set and the coverage filter — the shape the Node
    side already had via npm run cov. BREAKING CHANGES: the
    coverageInclude export is gone with it; the regression proof now pins
    the delegation, which is what can actually drift.
  • fjs/emergent_testing/module.f.mjs: the star comment said the marker is
    "non-empty only for Bun", which stopped being true when the inline
    strategy was extended to Node below the 26 baseline.
  • todo/migrate-typescript-to-mjs.md: the item is checked off, with the
    measurements above kept in place of the stale framing.

The 2431 vs 2495 gap

The issue asked for this to be understood rather than papered over. It is
not a second defect: it is exactly the 64 sub-tests reachable only through a
test function's return value. usesInlineTestContext
(fjs/effects/node/module.f.mjs) selects the flattened registration
strategy below the Node 26 baseline, where inlineContext runs sub-tests
inside their parent registration instead of declaring them to the framework,
so the framework counts the parent only. Node 26 uses native expectFailure
with nested registration and counts all 2495, matching
npm test (node ./fjs/module.mjs t).

Measured on 26.7.0: 2431 top-level plus 64 nested lines. On 24.18.1:
2431 top-level, 0 nested, of which 2376 carry the ... inline marker — the
other 55 are exactly the throw-tests, which never produce sub-tests and so
never get the marker. Same work executed either way; only the reported count
differs.

Verification

  • npx tsc — clean.
  • node ./fjs/module.mjs t — 0 fail. The count moves from 2495 to 2494 on
    this branch alone, the retired coverageInclude proof case; merging
    main (Add reduceValue2DefaultBranch test case to btree remove proof #1509) brings it back to 2495.
  • npm run cov after the change: 22.22.2 → 2431, 24.18.1 → 2431,
    26.7.0 → 2495, each all files 99.93; 23.11.1 goes from tests 0 and no
    report to tests 2431 with a full report.
  • deno task cov on the pinned CI Deno 2.9.5, after deno install --frozen
    ok | 2431 passed (64 steps) | 0 failed, same filtered per-module
    report the inlined command produced.
  • npm run ci-update is idempotent and the workflow diff is the single
    expected line.

Rust is untouched, so cargo commands were not run.

🤖 Generated with Claude Code

https://claude.ai/code/session_017QjDaCd6HwQGoU2gwGyyGE

`cov` relied on `node --test` finding `fjs/emergent_testing/all.test.ts`
through its default file patterns. Because that entrypoint is TypeScript,
whether it is discovered depends on the Node version: measured with the
previous command, v23.11.1 reports `tests 0` and prints no coverage report
at all, while 22.22.2/24.18.1 report 2431 and 26.7.0 reports 2495, each
with `all files 99.93`.

Name the entrypoint explicitly so the signal no longer depends on
discovery defaults, and drop the now-dead `**/module.f.ts` include glob
(no authored `.f.ts` remains). The same dead extension goes from the Deno
coverage filter, its regression proof, `deno.json` and the generated
workflow.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017QjDaCd6HwQGoU2gwGyyGE
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
functionalscript 9a15a99 Commit Preview URL

Branch Preview URL
Aug 13 2026, 06:12 AM

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017QjDaCd6HwQGoU2gwGyyGE

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This fixes a real, reproducible defect, and I can confirm the fix works on the exact Node version where the old command was vacuous.

The headline result

My last several reviews carried a standing caveat: npm run cov reports a meaningless 100.00 over 0 tests in my environment, so coverage is unverifiable. That caveat is now closed. Node v23.11.0, same machine, same node_modules:

tree command tests report
origin/main (3bc32ab9) npm run cov 0 vacuous
this PR (65c886a7) npm run cov 2431 / pass 2431 / fail 0 all files 99.93

So naming fjs/emergent_testing/all.test.ts as the entrypoint does exactly what the TODO says it does, including on the one released Node line where default discovery finds nothing. I will stop excluding coverage from verification.

Verification battery (at 65c886a7, clean tree)

  • npx tsc --noEmit — exit 0.
  • npm run prepack from a freshly cleaned tree — exit 0.
  • npm testpass: 2495, fail: 0, total: 2495, unchanged from main.
  • npm run ci-update then git add -A && git diff --cached --exit-code — clean, so the generated .github/workflows/ci.yml really is the output of the edited coverageInclude const. deno.json is not generated (it is only referenced in a comment), so its hand-edit is the right way to keep the third copy in sync.

Claims re-derived rather than assumed

  • Dropping **/module.f.ts and .f.ts from the Deno filter is safe. git ls-files | grep -cE '\.f\.ts$' is 0 at this head — no authored .f.ts remains, so neither filter can lose a module.
  • The unquoted glob in package.json is not shell-expanded. --test-coverage-include=**/module.f.mjs is one word; sh would need a directory literally named --test-coverage-include=* to match, so it passes through intact. (There is a depth-1 match, fjs/module.f.mjs, so this was worth checking.)
  • The 2431-vs-2495 explanation in the TODO reproduces here. Of the 2431 top-level lines on v23.11.0: 2376 carry the ... inline marker and the remaining 55 are exactly the .throw tests, with no others in that set. That is the table the doc states, measured independently on a third Node version.
  • The rewritten star comment in fjs/emergent_testing/module.f.mjs is accurate. usesInlineTestContext returns true for bun unconditionally and for node below the 26.0.0 baseline, which is what the new comment says; the old comment also described the marker as ' *' when the code has emitted ' ...' (const star = o.inlineTestContext ? ' ...' : ''), so this corrects a stale comment as well.

CHANGELOG entry is present, sits in Unreleased, and links only /pull/1512 per §8.3. The TODO rewrite is honest about which part of the earlier diagnosis was right (discovery) and which framing was wrong («vacuous for several PRs» — it was vacuous only off-CI, on v23), and keeps both rather than quietly deleting the wrong one. I like that.

Nit, non-blocking: the CHANGELOG sentence «The dead .f.ts extension leaves the Node and Deno coverage filters» parses as departs from on the second read but as leaves behind on the first. «is dropped from» would be unambiguous.

Nothing blocking.

"leaves" read as "departs from" rather than "leaves behind".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017QjDaCd6HwQGoU2gwGyyGE
Comment thread .github/workflows/ci.yml Outdated
},
{
"run": "deno test --allow-read --allow-env --allow-sys --coverage && deno coverage --include='.*module\\.f\\.(ts|mjs)'"
"run": "deno test --allow-read --allow-env --allow-sys --coverage && deno coverage --include='.*module\\.f\\.mjs'"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we run this line instead of deno task cov?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good reason — fixed in 959b765, the job now runs deno task cov.

The inlined form was pre-existing, and this PR made the cost of it visible: the coverage filter had three owners that had to be edited together — deno.json, coverageInclude in fjs/ci/deno/module.f.mjs, and the generated workflow. The Node jobs never had that problem because they run npm run cov and leave the filter to package.json; the Deno job just didn't follow the same rule.

So:

  • denoSteps emits deno task cov, and coverageInclude / the denoTest fragment are gone. deno.json is now the only owner of both the permission set and the --include regex.
  • The regression proof in fjs/ci/deno/proof.f.mjs now pins the delegation (assertEq(run, 'deno task cov')) instead of the filter string. Pinning the string was guarding the copy that could no longer drift once there is only one; what's worth guarding is that nobody re-inlines the command.
  • fjs/ci/proof.f.mjs's configuredPackageVersion assertion follows.

Verified with the pinned CI Deno (2.9.5) on a clean tree: deno install --frozen, then deno task covok | 2431 passed (64 steps) | 0 failed, with the same filtered per-module report the inlined command produced. npx tsc clean, fjs t 2494 pass / 0 fail (2495 minus the deleted coverageInclude test), and fjs/ci/deno/module.f.mjs stays at 100% coverage.

Removing the export is a public API removal, so it carries a **BREAKING CHANGES:** CHANGELOG entry.


Generated by Claude Code

claude and others added 2 commits August 13, 2026 06:08
The Node jobs run `npm run cov` and let `package.json` own the coverage
filter; the Deno job inlined `deno test … && deno coverage --include=…`
instead, so the same filter had three owners — `deno.json`, the CI
generator's `coverageInclude`, and the generated workflow.

Delegate to `deno.json`'s `cov` task and drop `coverageInclude` along with
the `denoTest` fragment. The regression proof now pins the delegation
rather than the filter string, which is what can silently drift.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017QjDaCd6HwQGoU2gwGyyGE
@sergey-shandar

Copy link
Copy Markdown
Contributor Author

@codex review

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addendum — re-verified at the new head. My approval was posted at 65c886a7; the head has since moved to 9a15a993, which is not just a merge of main: it adds fddc22ac (CHANGELOG reword) and 959b7650 (run deno task cov in CI instead of inlining the command). I re-ran the battery at 9a15a993 and it still stands — re-approving in effect, nothing new to raise.

959b7650 is a genuine improvement over what I approved. Removing coverageInclude leaves deno.json as the single owner of the Deno filter, which is the same shape the Node side already has (package.json owns npm run cov), so the "keep these two semantically equal" comment I read as a maintenance hazard is now a one-owner invariant instead. The public-surface removal is correctly marked **BREAKING CHANGES:** per §8.4 and links only /pull/1512. The CHANGELOG reword also fixes the "leaves the … filters" ambiguity I nitted — thanks.

At 9a15a993:

  • npm run ci-update then git add -A && git diff --cached --exit-code — clean, so the regenerated ci.yml step really is deno task cov.
  • grep -rn coverageInclude fjs/ outside todo/ — no hits, so the export is gone with no dangling importer (fjs/ci/proof.f.mjs dropped its import too).
  • npm testpass: 2495, fail: 0, total: 2495 against 2496 on current main (20cd6aa9). The −1 is exactly the retired coverageInclude proof case; the merge of #1509 contributes +1, so 2495 + 1 − 1 = 2495. Accounted for, not a regression.
  • npm run cov on Node v23.11.0 — tests 2431 / pass 2431 / fail 0, all files 99.93. The headline result is unchanged: still a real coverage report where main reports tests 0.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 13, 2026
Merged via the queue into main with commit b13e654 Aug 13, 2026
19 checks passed
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 9a15a993ea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants