Skip to content

Bring djs/parser to 100% coverage: 94.05% → 100% branch - #1556

Merged
sergey-shandar merged 4 commits into
mainfrom
claude/djs-parser-coverage-improvement
Aug 14, 2026
Merged

Bring djs/parser to 100% coverage: 94.05% → 100% branch#1556
sergey-shandar merged 4 commits into
mainfrom
claude/djs-parser-coverage-improvement

Conversation

@sergey-shandar

Copy link
Copy Markdown
Contributor

Summary

fjs/djs/parser/module.f.mjs was at 94.05% branch coverage (16 uncovered branches). This brings it to 100%:

  • tokenToValue's default arm — all three call sites already guard with isValueToken first, so the fallback was unreachable, same shape as media/json/parser (Remove unreachable throw in djs serializer's constSerialize #1544). Narrowed tokenToValue's parameter to a new _ValueToken type (derived from isValueToken as a type guard) so the switch is exhaustive with no defensive arm.
  • 14 missing eof/unexpected-token tests — most of the file's per-state parse functions (parseExportOp, parseConstOp, parseConstNameOp, parseImportOp, parseImportNameOp, parseImportFromOp, parseArrayStartOp, parseObjectStartOp, parseObjectKeyOp, parseObjectColonOp, parseObjectNextOp, parseObjectCommaOp) had an eof-mid-construct or unexpected-token branch nobody had written a test for, e.g. bare 'export'/'const'/'import a from', an unterminated 'export default [', an object key with no ':'. Added one test per branch, following the file's existing invalid test style.
  • parseFromTokens's own default fallback — the tokenizer never produces an empty token list (it always emits at least eof), so this was untested; added a direct parseFromTokens(null) case exercising the exported function's own contract for that input.

Test plan

  • npx tsc --noEmit
  • node --test --experimental-test-coverage --test-coverage-include='fjs/djs/parser/module.f.mjs' fjs/emergent_testing/all.test.mjs → 100.00% line/branch/func
  • node ./fjs/module.mjs t → 2691 pass, 0 fail

🤖 Generated with Claude Code


Generated by Claude Code

- tokenToValue: narrow its parameter to a new _ValueToken type (the
  union isValueToken already establishes as a type guard) so the
  switch is exhaustive without a defensive default arm, matching the
  fix already applied to media/json/parser (#1544).
- Add proof cases for every parser state's unhandled eof/unexpected-
  token branch that had no test: bare 'export'/'const'/'const x'/
  'import'/'import a'/'import a from', an unterminated array/object,
  an object key with no ':', a ':' with no value, a ',' with no next
  member, and a stray token after an object value.
- Add a direct parseFromTokens(null) case: the tokenizer never
  produces an empty token list (it always emits at least eof), but
  the exported function's own contract still needs to handle one —
  the initial state falls through to the same "unexpected end".

fjs/djs/parser/module.f.mjs reaches 100% line/branch/function
coverage.
@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 80a77ee Commit Preview URL

Branch Preview URL
Aug 14 2026, 06:40 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.

Approving. Baseline origin/main = 4fbf0b71 (merge-base of this head).

The title's number is right. npm run cov in both trees, fjs/djs/parser/module.f.mjs row:

main  4fbf0b71 : 100.00 lines | 94.05 branch | 100.00 funcs
PR    d353d4cf : 100.00 lines | 100.00 branch | 100.00 funcs

all-files branch moves 98.71 → 99.04 alongside. These are real numbers, not the vacuous 100.00 over 0 tests this environment sometimes produces — both trees reported a populated table and they differ. npm test: 2676 → 2691, exactly +15 for the 15 new invalid cases.

Two-sided mutation of every new case. I drove proof.f.mjs directly (all 12 groups, 77 functions on main / 92 here) and, for each new case, broke the branch it targets — deleting the case 'eof': arm so the token falls through to the 'unexpected token' path, or corrupting the default: arm's payload — then applied the identical mutation at the corresponding line on origin/main:

mutation kills here kills on main
parseExportOp eof invalid[23] none
parseConstOp eof invalid[24] none
parseConstNameOp eof invalid[25] none
parseConstNameOp default invalid[26] none
parseImportOp eof invalid[27] none
parseImportOp default invalid[28] none
parseImportNameOp eof invalid[29] none
parseImportFromOp eof invalid[30] none
parseArrayStartOp eof invalid[31] none
parseObjectStartOp eof invalid[32] none
parseObjectKeyOp eof invalid[33] none
parseObjectColonOp eof invalid[34] none
parseObjectNextOp default invalid[35] none
parseObjectCommaOp eof invalid[36] none
parseFromTokens default invalid[37] none

15 mutations, 15 kills, one-to-one, and every one of them is silent on origin/main. No case is redundant with existing coverage and none is a message-pin that a branch change slips past — I specifically checked for the #1518 shape and it is not present here, because each case 'eof': deletion changes 'unexpected end' to 'unexpected token', which the assertEq catches.

The deleted default: return null really is dead, on main too. Static argument first: all three call sites of tokenToValue are if (isValueToken(token)), and isValueToken's true-arm kind list, tokenToValue's case list and _ValueToken's Extract list are the same seven kinds. Then the runtime proof on origin/main, where the arm still exists — replaced default: return null with default: throw new Error('DEAD-ARM-REACHED') and ran the full suite:

pass: 2676, fail: 0, total: 2676     <- arm never reached

Negative control, same file, case 'null': instrumented with a throw instead:

pass: 2671, fail: 5, total: 2676     <- probe does detect reachability

So the branch is dead on main as well as at this head — not the #1540 situation where a branch was dead only because of an unmentioned tightening.

_ValueToken is prefixed correctly and mirrors an already-established instance of exactly this pattern in fjs/media/json/parser/types.ts + module.f.mjs, down to the comment wording. §6.2 satisfied. Worth flagging for the record: bin/extract.mjs reports no surface change here, because it keys type aliases by bare name and _ValueToken already exists in fjs/media/json/parser, so the addition is masked — I read the surface off the emitted declarations instead. fjs/djs/parser/types.d.ts gains only export type _ValueToken, module.f.d.mts differs from main only in the @import comment line, and @module survives declaration emit (count 1). No signature widened to any.

Two small notes, neither blocking:

  1. The exhaustiveness is maintained by hand, not by the compiler. noImplicitReturns is off in tsconfig.json and AstConst includes undefined, so a missing case makes tokenToValue fall off the end and return undefined — which type-checks. Removing case 'bigint': return token.value from the new switch gives tsc --noEmit exit 0, 0 errors (positive control: returning a Symbol() from case 'null' gives exit 1, 1 error). This is not a regression — main's default: return null was equally silent, just with a different wrong value — but the changelog's "the switch is exhaustive" is a convention, not something the build enforces, and a future value-carrying token kind added to isValueToken and _ValueToken but not to tokenToValue would pass CI.
  2. The new cases pin the error message but not metadata. Rewriting a newly covered arm to metadata: null while keeping the message survives the whole proof set — for parseObjectKeyOp eof, parseExportOp eof and parseObjectNextOp default alike (negative control: changing that same arm's message to 'X' kills invalid[33]). Given the dedicated errorMetadata group and that the assertions are explicitly scoped to the message, I do not read this as the #1525/#1528 defect — nothing here returns the right tag while clobbering reachable state — but adding metadata to a couple of these would be cheap.

Also checked: changelog/unreleased/1556.md links only /pull/1556, no BREAKING prefix needed (the change is additive on the public surface); npm run prepack exit 0 in both trees from clean; bin/linkcheck.mjs broken-link sets identical to main. Very minor: the @module header on fjs/djs/parser/types.ts still describes the file as holding the ParseError shape, which is now one of two exports.

Non-blocking review note: the @module comment still described the
file as holding only the ParseError shape after this PR added
_ValueToken alongside it.

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

Re-reviewed at 8580f0db. Baseline is current origin/main = 4fbf0b71bfaa67aead3cbcada62b5eed060274a2 (confirmed by fetch; unchanged since the previous round).

The delta since the approved head d353d4cf is one commit touching two lines of the @module doc in fjs/djs/parser/types.ts — it now also names the _ValueToken subset tokenToValue accepts. git ls-tree confirms module.f.mjs (fb30a108) and proof.f.mjs (041799a8) are byte-identical blobs to d353d4cf, so no implementation or proof moved.

I re-measured the load-bearing numbers from scratch rather than carrying them forward:

origin/main 4fbf0b71 PR 8580f0db
fjs/djs/parser/module.f.mjs branch 94.05 100.00
all files branch 98.71 99.04
npm test 2676 2691

npm run cov in both trees, from freshly cleaned trees; line and funcs are 100.00/100.00 for the parser on both sides. The title claim measures exact. npx tsc --noEmit exit 0; npm run prepack exit 0 from a clean tree in both trees.

The types.ts doc edit survives declaration emit. This is exactly the §4 hazard — every import in the file is import type and can be elided, folding the header into the first import's trivia. Read the emitted fjs/djs/parser/types.d.ts directly rather than trusting bin/extract.mjs: the full /** … @module */ block is present with the new wording, both import type lines survive, and _ValueToken emits as a complete Extract<DjsToken, { readonly kind: … }> — no elided, no any, in any parser declaration file.

Public surface delta, read from the emitted declarations: exactly one addition, _ValueToken, correctly _-prefixed per §6.2 since the sibling module.f.mjs must @import it. module.f.d.mts differs from main on a single line — the @import comment gaining _ValueToken — so the parser's exported runtime signatures are otherwise unchanged.

node bin/linkcheck.mjs output is identical between main and the PR (same 146 lines, same broken-link set — the pre-existing todo/plan/vision.md entries). The changelog entry links only its own PR, with no issue or todo/ reference.

On the two notes I left open at d353d4cf:

  • Exhaustiveness is not compiler-enforced. Still true and still not this PR's job: noImplicitReturns remains commented out in tsconfig.json on both main and the PR, so it is a repo-wide setting, not a regression here. The new commit does the cheap useful half — the module doc now names _ValueToken, so a future editor of the switch has a pointer to the type that makes it exhaustive.
  • The new cases pin the error message but not metadata. Unchanged (proof.f.mjs is the same blob), so a metadata: null mutant would still survive. As before I do not think this is the #1525/#1528 defect, given the dedicated errorMetadata group covers that axis.

Both remain non-blocking. The mutation battery from the previous round (15 new cases → 15 mutations → 15 kills, zero kills on origin/main, and the deleted default: return null proven dead on main by a throw instrumentation with a negative control) stands on the same blobs and needs no re-run.

Documentation-only delta on an already-verified change, re-verified end to end. LGTM — approving.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 14, 2026
Merged via the queue into main with commit 5efbe37 Aug 14, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the claude/djs-parser-coverage-improvement branch August 14, 2026 20:46
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