test_runner: add statement coverage support#62340
test_runner: add statement coverage support#62340Felipeness wants to merge 4 commits intonodejs:mainfrom
Conversation
Parse source files with acorn to extract AST statement nodes and map V8 coverage ranges to those statements, providing statement-level coverage metrics alongside existing line, branch, and function coverage. The implementation uses acorn-walk's Statement visitor to collect all non-BlockStatement nodes, then finds the most specific (smallest) V8 coverage range containing each statement to determine its execution count. Files that cannot be parsed by acorn gracefully degrade to 100% statement coverage. Adds --test-coverage-statements CLI option for setting minimum statement coverage thresholds, consistent with existing --test-coverage-lines, --test-coverage-branches, and --test-coverage-functions options. Refs: nodejs#54530
|
Review requested:
|
|
How does this affect performance? |
|
Good question. Here's the breakdown: When coverage is disabled: zero overhead. The coverage module is lazy-loaded behind When coverage IS enabled: The parse results are cached per file URL, so repeated calls don't re-parse. Files that fail to parse gracefully degrade to Relative to V8's own coverage instrumentation overhead (which is already the cost of One minor thing I noticed while looking at this: |
The acorn-walk `simple` function does not fire a generic "Statement" visitor for concrete statement node types, which meant the statements array was always empty and coveredStatementPercent was always 100%. Fix by enumerating each concrete statement type (ExpressionStatement, ReturnStatement, IfStatement, etc.) as individual visitor keys. Also fixes: - sourceType fallback: try 'module' first, catch and retry 'script' - Infinity primordial: replace bare Infinity with bestRange null pattern - double disk I/O: summary() now reads source once and passes it to both getLines() and getStatements() - tests: assert totalStatementCount > 0 to catch this regression
lib/internal/test_runner/coverage.js
Outdated
| const kStatementTypes = [ | ||
| 'ExpressionStatement', 'ReturnStatement', 'ThrowStatement', | ||
| 'IfStatement', 'WhileStatement', 'DoWhileStatement', | ||
| 'ForStatement', 'ForInStatement', 'ForOfStatement', | ||
| 'SwitchStatement', 'TryStatement', 'BreakStatement', | ||
| 'ContinueStatement', 'VariableDeclaration', 'LabeledStatement', | ||
| 'WithStatement', 'DebuggerStatement', |
There was a problem hiding this comment.
I don't like this level of hard coding, is there not a general Statement handler or something?
There was a problem hiding this comment.
Good call — acorn-walk's simple() does support a generic Statement category visitor that fires for every node dispatched in a statement position. Replaced the hardcoded array with a single visitor.Statement handler and a small deny-set for BlockStatement/EmptyStatement.
This also picks up ClassDeclaration, FunctionDeclaration, and StaticBlock which were missing from the original list, and stays forward-compatible with any future ESTree statement types.
Simplified the double parse (module→script fallback) to a single sourceType: 'script' pass with permissive flags too, since script mode + allowImportExportEverywhere handles both ESM and legacy CJS.
- Replace hardcoded kStatementTypes array with acorn-walk's generic "Statement" category visitor, which automatically covers all current and future ESTree statement types (including ClassDeclaration, FunctionDeclaration, and StaticBlock that were previously missing). BlockStatement and EmptyStatement are excluded via a small deny-set. - Simplify AST parsing to a single pass using sourceType: 'script' with permissive flags (allowReturnOutsideFunction, allowImportExportEverywhere, allowAwaitOutsideFunction). Script mode is non-strict, so it handles both ESM and legacy CJS (e.g. `with` statements) without needing a module→script fallback. - Flatten V8 coverage ranges before the statement matching loop, reducing nesting from three levels to two. - Add coverage-class.js fixture and test for ClassDeclaration and StaticBlock statement coverage.
|
Haven’t had a chance to review yet, but conceptually, this is awesome - without all 4 standard coverage metrics, a testing solution is incomplete. Will review soon :-) |
- Add allowHashBang to acorn parse options for shebang support - Reorder requires to follow ASCII convention - Reuse existing doesRangeContainOtherRange helper - Add comments to empty catch blocks for consistency - Remove else after return in findLineForOffset - Break kColumnsKeys across multiple lines (max-len) - Document statement coverage fields in test:coverage event schema - Migrate threshold tests to data-driven loop - Remove unused tmpdir import from test file - Add fixture proving statement != line coverage - Add fixture testing shebang file parsing - Add fixture testing graceful degradation for unparseable files Refs: nodejs#62340
ljharb
left a comment
There was a problem hiding this comment.
For checking statement coverage thresholds this seems great! Does this also output coverage data that includes statement info?
|
|
||
| common.skipIfInspectorDisabled(); | ||
|
|
||
| const fixture = fixtures.path('test-runner', 'coverage.js'); |
There was a problem hiding this comment.
Are we confident that this fixture covers statement edge cases well enough? I’m asking because it was built for the initial implementation
| // acorn-walk's simple() fires a generic "Statement" visitor for every | ||
| // node dispatched in a statement position (Program body, block bodies, | ||
| // if/for/while bodies, etc.). This automatically covers all current and | ||
| // future ESTree statement types without hardcoding a list. | ||
| const visitor = { __proto__: null }; | ||
| visitor.Statement = (node) => { | ||
| if (kExcludedStatementTypes.has(node.type)) return; | ||
| ArrayPrototypePush(statements, { | ||
| __proto__: null, | ||
| startOffset: node.start, | ||
| endOffset: node.end, | ||
| count: 0, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
You should create this object inline
Summary
--test-coverage-statementsCLI option for setting minimum statement coverage thresholdsImplementation
Uses
acorn-walk'sStatementvisitor to collect all non-BlockStatementnodes from the AST. For each statement, the algorithm finds the most specific (smallest) V8 coverage range that fully contains it, using that range's execution count as the statement's coverage count. This approach is similar to how@aspect-build/v8-coverage(used by Vitest) handles statement coverage.Files changed
lib/internal/test_runner/coverage.js- Core statement extraction and coverage computationlib/internal/test_runner/utils.js- Addedstmts %column to coverage report tablelib/internal/test_runner/test.js- Statement coverage threshold checkingsrc/node_options.cc/src/node_options.h---test-coverage-statementsCLI optiondoc/api/cli.md- Documentation for the new optiontest/parallel/test-runner-coverage-statements.js- TestsTest plan
--test-coverage-statementsthreshold passing and failingstmts %column appears in tap/spec reporter outputRefs: #54530