Skip to content

Bring js/tokenizer to 100% line/branch coverage: 97.54% → 100% branch - #1564

Merged
sergey-shandar merged 7 commits into
mainfrom
claude/js-tokenizer-coverage-improvement
Aug 14, 2026
Merged

Bring js/tokenizer to 100% line/branch coverage: 97.54% → 100% branch#1564
sergey-shandar merged 7 commits into
mainfrom
claude/js-tokenizer-coverage-improvement

Conversation

@sergey-shandar

Copy link
Copy Markdown
Contributor

Summary

fjs/js/tokenizer/module.f.mjs was at 97.54% branch / 97.40% func coverage (6 uncovered branches, 4 uncalled functions). This brings line and branch coverage to 100%:

  • Three previously-untested "invalid number" transitions, each in a different number-parsing handler: a leading zero followed directly by another digit (digit19ToToken's own '0' arm — distinct from the existing '00' case, which goes through digit0ToToken instead), 'e' right after a bare '.' with no fractional digits yet, and '+' after the exponent's digits have already started.
  • Removed tokenizeOp, a thin (input, state) => input === null ? tokenizeEofOp(state) : tokenizeCharCodeOp(input, state) wrapper. Every one of its 16 call sites is a _CreateToToken handler, whose own type ((input: number) => ...) guarantees a non-null input, so the input === null branch could never be taken. All call sites now call tokenizeCharCodeOp directly, and the ternary — along with its permanently-dead branch — is gone.
  • Added direct-call proof cases for tokenizeCharCodeOp/tokenizeEofOp's own 'eof'-state arms. tokenize() appends exactly one trailing null after its input, so the scan reaches { kind: 'eof' } only on that final step — nothing ever runs either function again afterward with that state, and proving this through the type system would require threading the invariant through the whole stateScan-based composition (out of scope here). This matches the file's own existing convention (getOperatorTokenInvalid, unionConflict) of covering such unreachable-through-tokenize() branches with a direct call rather than leaving them uncovered.

funcs stays at 98.08%: the three still-uncalled closures (def/a/b) are unionConflict's own test fixtures, deliberately never invoked since that test asserts union() throws before calling either handler — covering them would defeat the test's purpose (same shape as #1535's proof.throw).

Test plan

  • npx tsc --noEmit
  • node --test --experimental-test-coverage --test-coverage-include='fjs/js/tokenizer/module.f.mjs' fjs/emergent_testing/all.test.mjs → 100.00% line/branch, 98.08% func (see above)
  • node ./fjs/module.mjs t → 2712 pass, 0 fail

🤖 Generated with Claude Code


Generated by Claude Code

- Add proof cases for three previously-untested "invalid number"
  transitions: a leading zero followed directly by another digit
  (digit19ToToken's own '0' arm, distinct from the existing '00'
  case), 'e' right after a bare '.' with no fractional digits yet,
  and '+' after the exponent's digits have already started.
- Remove tokenizeOp, a thin (input, state) => input === null ?
  tokenizeEofOp(state) : tokenizeCharCodeOp(input, state) wrapper.
  Every one of its 16 call sites is a _CreateToToken handler, whose
  own type guarantees a non-null number input, so the input === null
  branch could never be taken; all call sites now call
  tokenizeCharCodeOp directly.
- Add direct-call proof cases for tokenizeCharCodeOp/tokenizeEofOp's
  own 'eof'-state arms. tokenize() appends exactly one trailing null
  after its input, so the scan reaches { kind: 'eof' } only on that
  final step — nothing ever runs either function again afterward with
  that state — matching the file's existing convention
  (getOperatorTokenInvalid, unionConflict) of covering such
  unreachable-through-tokenize() branches with a direct call.

funcs stays at 98.08%: the three uncalled closures (def/a/b) are
unionConflict's own test fixtures, deliberately never invoked since
the test asserts union() throws before calling either handler —
covering them would defeat the test's purpose.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 14, 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 d85573d Commit Preview URL

Branch Preview URL
Aug 14 2026, 11:01 PM

@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.

Reviewed at 6b191e1, baseline origin/main = c3af487f ("Add task to split AGENTS.md into scoped documents (#1560)").

What I verified

  • npx tsc --noEmit → exit 0. npm testpass: 2713, fail: 0 (main: pass: 2708, fail: 0, +5 = 2 new module.f.mjs proofs + 3 new proof.f.mjs cases).

  • The title's claim, measured with npm run cov (Node v23.11.0, both trees clean). Row fjs/js/tokenizer/module.f.mjs:

    line branch funcs
    origin/main 100.00 97.14 97.40
    this PR 100.00 100.00 98.08

    So "100% line/branch" holds. The before-figure in the title is 97.54; I measure 97.14 — probably a Node-version difference, not worth changing. Note funcs is 98.08, not 100, so "100% line/branch coverage" is the right way to phrase it (as the title does).

  • Removing tokenizeOp is sound. Every call site is inside a _CreateToToken<…> handler, and _ToToken = (input: number) => … (types.ts:204), so input === null is statically unreachable. Instrumented on origin/main rather than arguing from types alone: replacing the input === null arm with a throw leaves the suite at pass: 2708, fail: 0; the negative control (throw in the other arm) fails 209 tests. Dead on main, not merely dead at this head.

  • Mutation-tested each new case two-sided (mutate → run full suite at this head; same mutation on origin/main → confirm it kills nothing):

    • '01'digit19ToToken's case '0' arm: collapsing it into the '.'/'fractional' arm ⇒ 1 fail here, 0 on main. Good.
    • '1.e5'expToToken's default: merging it into the accepting arm ⇒ 1 fail here, 0 on main. Good.
    • tokenizeCharCodeOpAfterEofeofStateOp returning [[], state]1 fail here, 0 on main. Good.
    • tokenizeEofOpAfterEoftokenizeEofOp's 'eof' arm dropping the error token ⇒ 1 fail here, 0 on main. Good.

Two findings

1. '1e5+' raises branch coverage but does not prove the branch

Deleting plusSignToToken's default branch — i.e. making the function unconditional by giving default the same body as case 'e':

const plusSignToToken = state => input => {
    switch (state.numberKind) {
        case 'e': return [empty, {, numberKind: 'e+' }]
        default: return [empty, {, numberKind: 'e+' }]   // was: tokenizeCharCodeOp(input, { kind: 'invalidNumber' })
    }
}

leaves the entire suite green: pass: 2713, fail: 0. The reason is that the assertion pins the string '[{"kind":"error","message":"invalid number"},{"kind":"eof"}]', and tokenizeEofOp's 'e+' arm emits exactly that same invalid number error at EOF — so both the real code and the mutant print an identical result for '1e5+':

1e5+  original => [{"kind":"error","message":"invalid number"},{"kind":"eof"}]
1e5+  mutant   => [{"kind":"error","message":"invalid number"},{"kind":"eof"}]

This is the shape from #1518: pinning an error message let a branch-changing mutation through. The branch is executed (hence 100% branch coverage) but nothing observes that taking it matters.

Adding one character fixes it — '1e5+3' makes the two diverge, because under the mutant the '+' is accepted as e+ and the following digit turns the whole thing into a valid number token:

1e5+3 original => [{"kind":"error","message":"invalid number"},{"kind":"eof"}]
1e5+3 mutant   => a number token (its value is a BigInt)

I confirmed '1e5+3' still produces [{"kind":"error","message":"invalid number"},{"kind":"eof"}] on the unmutated PR head, so it is a drop-in replacement for (or addition to) the '1e5+' case.

2. The two new module.f.mjs proofs pin only the token half of the returned pair

Both destructure const [tokens] = … and drop next[1]. Both targeted arms are state passthroughs (eofStateOp's state, and tokenizeEofOp's case 'eof': … , state]), and clobbering that passthrough survives the whole suite:

  • eofStateOp[[{ kind: 'error', message: 'eof' }], { kind: 'initial' }]pass: 2713, fail: 0
  • tokenizeEofOp 'eof' arm → […, { kind: 'initial' }]pass: 2713, fail: 0

Same shape as #1525/#1528, fixed on #1559 by asserting the state survives. Since these arms are (by the comment's own argument) unreachable through tokenize, the co-located proof is the only thing that can observe them — nothing else in the suite will ever catch a state regression here. Asserting the second element too, e.g.

const [tokens, state] = tokenizeEofOp({ kind: 'eof' })
assertStructurallySame(toArray(tokens), [{ kind: 'error', message: 'eof' }, { kind: 'eof' }])
assertStructurallySame(state, { kind: 'eof' })

closes it. (Unlike the proof.throw case below it, which correctly asserts only that it throws, these are value-returning proofs, so the full return value is fair game.)

Minor

changelog/unreleased/1564.md ends with [#1564](…/pull/1564), but AGENTS.md §8.3 now says to write entries "with no PR number or link inside the file — the file name already carries the number, and a renderer derives the link from it." Three of the four existing changelog/unreleased/ files still carry links, so this follows the current de-facto shape rather than the written rule; flagging it only so the drift gets noticed somewhere.

Not an issue

  • The getOperatorTokenInvalid and throw.unionConflict proofs are unchanged and out of scope here.
  • _CharCodeOrEof is still used by tokenizeWithPositionOp, so removing tokenizeOp does not orphan it.
  • Entry length (~230 chars) is within the house shape.

- '1e5+' didn't distinguish the mutant that merges plusSignToToken's
  default arm into its 'e' arm (both eof-cut a still-invalid token to
  the same message). Replaced with '1e5+3': under the mutant, the '+'
  restarts the exponent and the trailing '3' completes a valid number
  instead, which the pinned error message catches.
- tokenizeCharCodeOpAfterEof/tokenizeEofOpAfterEof only asserted the
  returned tokens, not the passthrough state — since these arms are
  unreachable through tokenize(), the co-located proof is the only
  thing that can ever catch a regression in the state half. Assert it
  too (same shape as #1559's fix for the same weakness).
- changelog/unreleased/1564.md still had a PR link; AGENTS.md §8.3
  now says entries carry no PR number or link (the file name already
  carries it).

Copy link
Copy Markdown
Contributor Author

Fixed in 3613475:

  • '1e5+''1e5+3': the trailing digit distinguishes the branch from the mutant that merges plusSignToToken's default arm into its 'e' arm (both eof-cut to the same "invalid number" message; the mutant instead completes a valid number when a digit follows).
  • tokenizeCharCodeOpAfterEof/tokenizeEofOpAfterEof now also assert the returned (passthrough) state, closing the Add defensive branch coverage for endObject non-object guard #1525/Add test case for writeFile over JsModule entries #1528-shaped gap — since these arms are unreachable through tokenize(), the co-located proof is the only thing that could ever catch a state regression here.
  • changelog/unreleased/1564.md no longer carries a PR link, matching the current AGENTS.md §8.3 wording.

npx tsc --noEmit, targeted coverage (still 100.00% line/branch), and the full suite (2712 pass, 0 fail) all still pass.


Generated by Claude Code

@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. All three items from the previous round at 6b191e14 are resolved, and I re-verified each by building the mutant rather than reading the case names.

Baseline for everything below: origin/main at 4a3b6877 (js/keywords: one source of truth for JavaScript keywords (#1562)).

The three open findings

1. '1e5+''1e5+3' — now discriminating. I rebuilt the collapsing mutant, folding plusSignToToken's default into its case 'e' arm:

const plusSignToToken = state => input => {
    return [empty, { kind: 'number', value: appendChar(state.value)(input), b: state.b, numberKind: 'e+' }]
}

It dies, killed by exactly the new case:

FAIL js/tokenizer/proof.f.mjs .djs[39]
  thrown: [{"bf":[1n,53],"kind":"number","value":"1e5+3"},{"kind":"eof"}]

The trailing 3 is what does it — with the mutant the + restarts the exponent and 1e5+3 tokenizes as a valid number, which 'invalid number'-on-eof alone could not have told apart. The comment in the proof says this explicitly, which is the right place for it.

2. The two module.f.mjs proofs now pin the state, not just the tokens. Both state-clobbering mutants die:

  • eofStateOp: (state => () => [[{ kind: 'error', message: 'eof' }], state])…, { kind: 'initial' }])
    → kills proof.tokenizeCharCodeOpAfterEof
  • tokenizeEofOp's 'eof' arm: return [[…], state]return [[…], { kind: 'initial' }]
    → kills proof.tokenizeEofOpAfterEof

That closes the #1525/#1528/#1559 shape here. For completeness: a mutant returning the literal { kind: 'eof' } in place of state would still survive, but _EofState has exactly one inhabitant, so that mutant is semantically identical — there is no non-trivial state to start from in this case, and the assertion is as strong as the type allows.

3. Changelog format — fixed. changelog/unreleased/1564.md is now three wrapped list-item lines, no PR number or link inside, no heading, ~200 characters. Matches §8.3 and the shape of its neighbours (1562.md, 1563.md).

The two other new cases, also mutation-tested

I did not take the case names on trust for these either (§3.2):

  • '01' — dropping case '0': from digit19ToToken (so '0' falls through to the int arm) dies, killed by '01'.
  • '1.e5' — collapsing expToToken's default into its accepting arm dies, killed by '1.e5'.

All four mutants I built are killed by exactly the case that was added for them, and by nothing else in the tokenizer proofs.

Battery

  • npx tsc --noEmit — exit 0.
  • npm run prepack from a clean tree — exit 0.
  • npm test2714 pass / 0 fail, against 2709 on 4a3b6877. The +5 is exactly the five added proofs (three cases in proof.f.mjs, two named ones in module.f.mjs); nothing was lost.
  • Emitted declarations, whole tree, main vs PR: the only diff is js/tokenizer/module.f.d.mts gaining the two new proof members. No other public surface moves, nothing widens to any, no elided.
  • bin/linkcheck.mjs — broken-link sets byte-identical to main (147 lines both sides). Deleting nothing, so this was expected, but checked rather than assumed.

Carried over from 6b191e14, not re-verified (untouched since)

Removing tokenizeOp is sound: its input === null arm was proven dead on origin/main by instrumentation — a throw in that arm killed 0 tests, while the negative control killed 209. Measured coverage for js/tokenizer/module.f.mjs went branch 97.14 → 100.00, line 100 → 100, funcs 97.40 → 98.08. The title's "97.54" before-figure is off by a little; not worth another push, but if you retitle on merge, 97.14 is the number I measured.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit ac47cb7 Aug 14, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the claude/js-tokenizer-coverage-improvement branch August 14, 2026 23:09
sergey-shandar pushed a commit that referenced this pull request Aug 15, 2026
registerEmptyModuleMap/registerEmptyModuleMapInlineBun raised the
branch counter for register's inlineTestContext/engine ternaries but
discarded the state, so a mutant swapping either ternary's arms (or
deleting a branch outright) survived undetected — same shape as #1564.

Replaces the ctx/star part with registerSelectsContextAndStar, which
interprets register's effect through a synthetic runner (discovery
faked via readdir/import, since the virtual harness's own test op is
todo and would throw on a non-empty root) and asserts which
TestContext object and which registered name each test call actually
received.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BmWCv5YGRXToq26xPoSXjX
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