Repository navigation
test(html): cover the tables no corpus reaches - #22058
Conversation
Measured every lookup table in lib/html/data.js against configCases, wpt and the framework corpus: 21 held names nothing spelled. Seven cases take 24 tables further and 15 to full, EVENT_HANDLER_ATTRIBUTES from 5/94 among them, each handler on an element the table allows so a wrong binding fails rather than declines quietly. Two pin what must not fold: autocomplete names autofill on an input where it enumerates on a form.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesThe PR adds HTML minification coverage for attribute normalization, event handlers, foreign names, marquee scope, quirks-mode doctypes, and ruby annotations. It also updates token-list equivalence handling for duplicate tokens. HTML Minification Cases
Suggested reviewers: Priority: ⬇️ Low Change: Other Merge Risk: ⚪ Minimal · up to The PR adds test coverage without changing library behavior, and no merge-blocking issue remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Full details: Title checkExplanation The title uses valid Conventional Commit syntax with the allowed type Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22058 +/- ##
=======================================
Coverage 94.97% 94.97%
=======================================
Files 726 726
Lines 97275 97275
Branches 29629 29629
=======================================
+ Hits 92386 92389 +3
+ Misses 4889 4886 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The new attribute case put a repeat and extra whitespace in `accesskey` and `itemref`, and the equivalence check called the minified page a different DOM: it read neither as a list, and it sorted the lists it did read without dropping a repeat. Both are sets of unique tokens in the spec, so a repeat names nothing the first did not. `ping` stays as written, where the spec lists URLs rather than a set.
Merging this PR will improve performance by 88.29%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | benchmark "side-effects-reexport", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
454.1 KB | 167.8 KB | ×2.7 |
| ⚡ | Memory | benchmark "concatenate-modules", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
2.6 MB | 1 MB | ×2.6 |
| ⚡ | Memory | benchmark "no-minimize", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
902.7 KB | 625.9 KB | +44.21% |
| ⚡ | Memory | unit benchmark "js-parser-unit", source 'react.development.js' |
740.1 KB | 600.2 KB | +23.3% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing aryanraj45:test/html-table-coverage (1ae8c74) with main (3a69d29)
Footnotes
-
6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
…e reads The set the harness folds a repeat out of was written from the spec calling a list unique, which is a wider line than the printer's: it drops a repeat only where the list reflects as a DOMTokenList, leaving `headers` and `accesskey` as the strings they reflect as. Folding those here hid the difference. The case now carries a repeat in all six lists, where it drove two.
|
This PR is packaged and the instant preview is available (e142eea). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@e142eea
yarn add -D webpack@https://pkg.pr.new/webpack@e142eea
pnpm add -D webpack@https://pkg.pr.new/webpack@e142eea |
Summary
Measured every lookup table in
lib/html/data.jsagainstconfigCases, wpt and the framework corpus the tool comparisons install: 21 tables held names nothing spelled, so those branches were reached by no fixture. Seven cases take 24 tables further and 15 to full.EVENT_HANDLER_ATTRIBUTESgoes from 5/94 to full, each handler written on an element the table allows it on, so a wrong binding fails rather than declining quietly. The rest close the tree-construction tablesmarqueeand the ruby elements are the only names in (SPECIAL,HTML_SCOPE,IMPLIED,SCOPE_ENDED_BY, the optional-end-tag pair), the attribute rewrite tables, both quirks-mode public identifiers, and the reserved SVG names.Two cases pin what must not change:
autocompletenames autofill on aninput, where it enumerates on aform, so only the second folds.What kind of change does this PR introduce?
test — it adds
configCases/only, nolib/change.Did you add tests for your changes?
This is the tests. Seven cases under
test/configCases/html/:marquee-scope,ruby-annotations,event-handler-attributes,attribute-tables,quirky-doctype-w3o,quirky-doctype-w3c,foreign-reserved-names.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Claude Code was used, driven by me, for three things: walking the data tables against each corpus to find which names no fixture spells, writing the fixtures and cases, and running the suites. Every finding it reported was re-checked against the spec before it went in — three suspected defects (ruby end-tag omission, enumerated folding,
autocomplete) turned out to be correct behaviour with the fixture at fault, and theautocompleteone is now pinned as a case. Nolib/code was changed.Summary by CodeRabbit