chore(coverage): move CommonMark conformance tests to xtask/coverage#8857
chore(coverage): move CommonMark conformance tests to xtask/coverage#8857dyc3 merged 2 commits intobiomejs:nextfrom
Conversation
|
Merging this PR will not alter performance
Comparing Footnotes
|
WalkthroughThis PR removes the CommonMark test harness file from crates/biome_markdown_parser/tests and relocates CommonMark compliance testing into the xtask/coverage framework. It adds a new markdown coverage module and a CommonMark test suite that loads an embedded spec.json, introduces a script to update spec.json, updates the justfile to run coverage for markdown, adjusts xtask/coverage Cargo manifest and wiring (modules, runner extension), and updates CLI help text. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/update-commonmark-spec.sh`:
- Around line 41-44: The script currently treats missing arguments the same as
help flags and exits 0; change the conditional so that if "$1" is "-h" or
"--help" it calls print_help and exits 0, but if there are no arguments (i.e.,
$# -lt 1) call print_help and exit with a non-zero status (e.g., exit 1 or exit
2) so missing required arguments return a failure code; update the if/elif/else
around the existing print_help invocation to reflect this in the block
containing print_help and the exit call.
🧹 Nitpick comments (3)
xtask/coverage/src/runner.rs (1)
354-356: Consider simplifying the reporting loop.Iterating solely to call
test_loaded()for each item is a bit verbose. If the reporter only needs a count, a single call with the length might suffice—though this depends on whethertest_loaded()has side effects beyond counting.♻️ Optional simplification
- for _ in &tests { - context.reporter.test_loaded(); - } + (0..tests.len()).for_each(|_| context.reporter.test_loaded());Or if the reporter can accept a batch count, that would be cleaner still.
xtask/coverage/src/markdown/commonmark.rs (1)
86-107: Edge case: same-line<pre>...</pre>handling.If a line contains both
<preand</pre>(e.g.,<pre>code</pre>), the current logic setsin_pre = truethen immediatelyin_pre = false, so it processes the line as inside a<pre>block. This happens to be correct for preserving whitespace on that line, but the logic is fragile. Consider adding a brief comment or a test case to document this behaviour.justfile (1)
237-239: Consider Windows portability for the update script.The
./scripts/update-commonmark-spec.shinvocation won't work on Windows without WSL. The justfile already handles platform differences for_touch(lines 213-219). You could apply a similar pattern here, or add a comment noting WSL/Git Bash is required.That said, this is an infrequent maintenance task, so it's not a blocker.
Move the CommonMark specification compliance tests from `crates/biome_markdown_parser/tests/` to `xtask/coverage/src/markdown/` to align with how other language conformance tests are organized. CommonMark publishes spec.json at versioned URLs (no git repo to clone), so we embed it directly (140KB, 652 tests). Add update tooling: - `scripts/update-commonmark-spec.sh` downloads spec and auto-updates the provenance comment in commonmark.rs - `just update-commonmark-spec <version>` for one-command updates
1e22275 to
c360773
Compare
Note
AI Assistance Disclosure: This PR was developed with assistance from Claude Code.
Summary
Moves CommonMark spec compliance tests from
crates/biome_markdown_parser/tests/toxtask/coverage/src/markdown/to align with how other language conformance tests are organized.load_all()for embedded test suitesjust update-commonmark-spec <version>for one-command spec updatesbiome_markdown_parsertoxtask/coverageCommonMark publishes
spec.jsonat versioned URLs so we embed the spec (140KB, 652 tests). The update script downloads the spec and auto-updates the provenance comment incommonmark.rs, providing a one-command update workflow.Follow-up to PR #8525 and is a part of #3718.
Test Plan