From eeb63759449c488086cb88ffb00d43a0542ccb63 Mon Sep 17 00:00:00 2001 From: Junaid Khan Date: Tue, 20 Oct 2026 17:56:57 +0530 Subject: [PATCH] test_runner: keep never-run blocks when merging coverage V8 omits a nested block when its count equals its parent. The old set-based merge dropped those uncovered ranges across processes, so coverage gates could pass. Merge with a range tree instead, matching V8/c8 semantics. Fixes: https://github.com/nodejs/node/issues/66860 Signed-off-by: Junaid Khan --- lib/internal/test_runner/coverage.js | 66 +--- .../test_runner/coverage_range_tree.js | 344 ++++++++++++++++++ .../coverage-merge-nested/a.test.mjs | 8 + .../coverage-merge-nested/b.test.mjs | 7 + .../test-runner/coverage-merge-nested/mod.mjs | 9 + .../test-runner-coverage-merge-nested.js | 49 +++ test/parallel/test-runner-coverage.js | 15 +- 7 files changed, 431 insertions(+), 67 deletions(-) create mode 100644 lib/internal/test_runner/coverage_range_tree.js create mode 100644 test/fixtures/test-runner/coverage-merge-nested/a.test.mjs create mode 100644 test/fixtures/test-runner/coverage-merge-nested/b.test.mjs create mode 100644 test/fixtures/test-runner/coverage-merge-nested/mod.mjs create mode 100644 test/parallel/test-runner-coverage-merge-nested.js diff --git a/lib/internal/test_runner/coverage.js b/lib/internal/test_runner/coverage.js index 3d0fcde700a..0e3616c77f2 100644 --- a/lib/internal/test_runner/coverage.js +++ b/lib/internal/test_runner/coverage.js @@ -41,6 +41,8 @@ const { } = require('internal/errors'); const { matchGlobPattern } = require('internal/fs/glob'); const { constants: { kMockSearchParam } } = require('internal/test_runner/mock/loader'); +const { mergeFunctionRanges } = + require('internal/test_runner/coverage_range_tree'); const kCoverageFileRegex = /^coverage-(\d+)-(\d{13})-(\d+)\.json$/; const kIgnoreRegex = /\/\* node:coverage ignore next (?\d+ )?\*\//; @@ -787,65 +789,11 @@ function mergeCoverageScripts(oldScript, newScript) { } function mergeCoverageRanges(oldFn, newFn) { - const mergedRanges = new SafeSet(); - - // Keep all of the existing covered ranges. - for (let i = 0; i < oldFn.ranges.length; ++i) { - const oldRange = oldFn.ranges[i]; - - if (oldRange.count > 0) { - mergedRanges.add(oldRange); - } - } - - // Merge in the new ranges where appropriate. - for (let i = 0; i < newFn.ranges.length; ++i) { - const newRange = newFn.ranges[i]; - let exactMatch = false; - - for (let j = 0; j < oldFn.ranges.length; ++j) { - const oldRange = oldFn.ranges[j]; - - if (doesRangeEqualOtherRange(newRange, oldRange)) { - // These are the same ranges, so keep the existing one. - oldRange.count += newRange.count; - mergedRanges.add(oldRange); - exactMatch = true; - break; - } - - // Look at ranges representing missing coverage and add ranges that - // represent the intersection. - if (oldRange.count === 0 && newRange.count === 0) { - if (doesRangeContainOtherRange(oldRange, newRange)) { - // The new range is completely within the old range. Discard the - // larger (old) range, and keep the smaller (new) range. - mergedRanges.add(newRange); - } else if (doesRangeContainOtherRange(newRange, oldRange)) { - // The old range is completely within the new range. Discard the - // larger (new) range, and keep the smaller (old) range. - mergedRanges.add(oldRange); - } - } - } - - // Add new ranges that do not represent missing coverage. - if (newRange.count > 0 && !exactMatch) { - mergedRanges.add(newRange); - } - } - - oldFn.ranges = ArrayFrom(mergedRanges); -} - -function doesRangeEqualOtherRange(range, otherRange) { - return range.startOffset === otherRange.startOffset && - range.endOffset === otherRange.endOffset; -} - -function doesRangeContainOtherRange(range, otherRange) { - return range.startOffset <= otherRange.startOffset && - range.endOffset >= otherRange.endOffset; + // Sum innermost counts per source position across processes, matching V8's + // own coverage semantics (as used by c8 via @bcoe/v8-coverage). The previous + // set-based merge dropped never-run nested blocks when one process omitted + // them because their count equaled the parent. See nodejs/node#66860. + oldFn.ranges = mergeFunctionRanges(oldFn.ranges, newFn.ranges); } module.exports = { setupCoverage, TestCoverage }; diff --git a/lib/internal/test_runner/coverage_range_tree.js b/lib/internal/test_runner/coverage_range_tree.js new file mode 100644 index 00000000000..20061a1913a --- /dev/null +++ b/lib/internal/test_runner/coverage_range_tree.js @@ -0,0 +1,344 @@ +'use strict'; + +// Range-tree merge for V8 function coverage, based on @bcoe/v8-coverage (MIT): +// https://github.com/demurgos/v8-coverage +// +// V8 omits a nested block when its count equals its parent's. Merging two +// process coverages must recover those omitted uncovered blocks by summing, +// for each source position, the count of the innermost range that contains it +// in each process. See https://github.com/nodejs/node/issues/66860. + +const { + ArrayPrototypePush, + ArrayPrototypeSort, + ArrayPrototypeSplice, + ArrayPrototypeUnshift, + MathMax, + SafeMap, +} = primordials; + +class RangeTree { + constructor(start, end, delta, children) { + this.start = start; + this.end = end; + this.delta = delta; + this.children = children; + } + + /** + * @precondition `ranges` are well-formed and pre-order sorted + */ + static fromSortedRanges(ranges) { + let root; + // Stack of parent trees and parent counts. + const stack = []; + for (let i = 0; i < ranges.length; ++i) { + const range = ranges[i]; + const node = new RangeTree( + range.startOffset, range.endOffset, range.count, [], + ); + if (root === undefined) { + root = node; + ArrayPrototypePush(stack, [node, range.count]); + continue; + } + let parent; + let parentCount; + while (true) { + [parent, parentCount] = stack[stack.length - 1]; + if (range.startOffset < parent.end) { + break; + } + stack.pop(); + } + node.delta -= parentCount; + ArrayPrototypePush(parent.children, node); + ArrayPrototypePush(stack, [node, range.count]); + } + return root; + } + + normalize() { + const children = []; + let curEnd; + let head; + const tail = []; + for (let i = 0; i < this.children.length; ++i) { + const child = this.children[i]; + if (head === undefined) { + head = child; + } else if (child.delta === head.delta && child.start === curEnd) { + ArrayPrototypePush(tail, child); + } else { + endChain(); + head = child; + } + curEnd = child.end; + } + if (head !== undefined) { + endChain(); + } + + if (children.length === 1) { + const child = children[0]; + if (child.start === this.start && child.end === this.end) { + this.delta += child.delta; + this.children = child.children; + return; + } + } + this.children = children; + + function endChain() { + if (tail.length !== 0) { + head.end = tail[tail.length - 1].end; + for (let i = 0; i < tail.length; ++i) { + const tailTree = tail[i]; + for (let j = 0; j < tailTree.children.length; ++j) { + const subChild = tailTree.children[j]; + subChild.delta += tailTree.delta - head.delta; + ArrayPrototypePush(head.children, subChild); + } + } + tail.length = 0; + } + head.normalize(); + ArrayPrototypePush(children, head); + } + } + + /** + * @precondition `tree.start < value && value < tree.end` + * @return {RangeTree} Right part + */ + split(value) { + let leftChildLen = this.children.length; + let mid; + for (let i = 0; i < this.children.length; ++i) { + const child = this.children[i]; + if (child.start < value && value < child.end) { + mid = child.split(value); + leftChildLen = i + 1; + break; + } else if (child.start >= value) { + leftChildLen = i; + break; + } + } + const rightLen = this.children.length - leftChildLen; + const rightChildren = + ArrayPrototypeSplice(this.children, leftChildLen, rightLen); + if (mid !== undefined) { + ArrayPrototypeUnshift(rightChildren, mid); + } + const result = new RangeTree(value, this.end, this.delta, rightChildren); + this.end = value; + return result; + } + + /** + * Get the range coverages corresponding to the tree. + * The ranges are pre-order sorted. + */ + toRanges() { + const ranges = []; + const stack = [[this, 0]]; + while (stack.length > 0) { + const [cur, parentCount] = stack.pop(); + const count = parentCount + cur.delta; + ArrayPrototypePush(ranges, { + __proto__: null, + startOffset: cur.start, + endOffset: cur.end, + count, + }); + for (let i = cur.children.length - 1; i >= 0; --i) { + ArrayPrototypePush(stack, [cur.children[i], count]); + } + } + return ranges; + } +} + +class RangeTreeWithParent { + constructor(parentIndex, tree) { + this.parentIndex = parentIndex; + this.tree = tree; + } +} + +class StartEvent { + constructor(offset, trees) { + this.offset = offset; + this.trees = trees; + } + + static compare(a, b) { + return a.offset - b.offset; + } +} + +class StartEventQueue { + constructor(queue) { + this.queue = queue; + this.nextIndex = 0; + this.pendingOffset = 0; + this.pendingTrees = undefined; + } + + static fromParentTrees(parentTrees) { + const startToTrees = new SafeMap(); + for (let parentIndex = 0; parentIndex < parentTrees.length; ++parentIndex) { + const parentTree = parentTrees[parentIndex]; + for (let i = 0; i < parentTree.children.length; ++i) { + const child = parentTree.children[i]; + let trees = startToTrees.get(child.start); + if (trees === undefined) { + trees = []; + startToTrees.set(child.start, trees); + } + ArrayPrototypePush( + trees, new RangeTreeWithParent(parentIndex, child), + ); + } + } + const queue = []; + for (const { 0: startOffset, 1: trees } of startToTrees) { + ArrayPrototypePush(queue, new StartEvent(startOffset, trees)); + } + ArrayPrototypeSort(queue, StartEvent.compare); + return new StartEventQueue(queue); + } + + setPendingOffset(offset) { + this.pendingOffset = offset; + } + + pushPendingTree(tree) { + if (this.pendingTrees === undefined) { + this.pendingTrees = []; + } + ArrayPrototypePush(this.pendingTrees, tree); + } + + next() { + const pendingTrees = this.pendingTrees; + const nextEvent = this.queue[this.nextIndex]; + if (pendingTrees === undefined) { + this.nextIndex++; + return nextEvent; + } else if (nextEvent === undefined) { + this.pendingTrees = undefined; + return new StartEvent(this.pendingOffset, pendingTrees); + } else if (this.pendingOffset < nextEvent.offset) { + this.pendingTrees = undefined; + return new StartEvent(this.pendingOffset, pendingTrees); + } + if (this.pendingOffset === nextEvent.offset) { + this.pendingTrees = undefined; + for (let i = 0; i < pendingTrees.length; ++i) { + ArrayPrototypePush(nextEvent.trees, pendingTrees[i]); + } + } + this.nextIndex++; + return nextEvent; + } +} + +/** + * @precondition Same `start` and `end` for all the trees + */ +function mergeRangeTrees(trees) { + if (trees.length <= 1) { + return trees[0]; + } + const first = trees[0]; + let delta = 0; + for (let i = 0; i < trees.length; ++i) { + delta += trees[i].delta; + } + const children = mergeRangeTreeChildren(trees); + return new RangeTree(first.start, first.end, delta, children); +} + +function mergeRangeTreeChildren(parentTrees) { + const result = []; + const startEventQueue = StartEventQueue.fromParentTrees(parentTrees); + const parentToNested = new SafeMap(); + let openRange; + while (true) { + const event = startEventQueue.next(); + if (event === undefined) { + break; + } + + if (openRange !== undefined && openRange.end <= event.offset) { + ArrayPrototypePush(result, nextChild(openRange, parentToNested)); + openRange = undefined; + } + + if (openRange === undefined) { + let openRangeEnd = event.offset + 1; + for (let i = 0; i < event.trees.length; ++i) { + const { parentIndex, tree } = event.trees[i]; + openRangeEnd = MathMax(openRangeEnd, tree.end); + insertChild(parentToNested, parentIndex, tree); + } + startEventQueue.setPendingOffset(openRangeEnd); + openRange = { __proto__: null, start: event.offset, end: openRangeEnd }; + } else { + for (let i = 0; i < event.trees.length; ++i) { + const { parentIndex, tree } = event.trees[i]; + if (tree.end > openRange.end) { + const right = tree.split(openRange.end); + startEventQueue.pushPendingTree( + new RangeTreeWithParent(parentIndex, right), + ); + } + insertChild(parentToNested, parentIndex, tree); + } + } + } + if (openRange !== undefined) { + ArrayPrototypePush(result, nextChild(openRange, parentToNested)); + } + return result; +} + +function insertChild(parentToNested, parentIndex, tree) { + let nested = parentToNested.get(parentIndex); + if (nested === undefined) { + nested = []; + parentToNested.set(parentIndex, nested); + } + ArrayPrototypePush(nested, tree); +} + +function nextChild(openRange, parentToNested) { + const matchingTrees = []; + for (const nested of parentToNested.values()) { + if (nested.length === 1 && + nested[0].start === openRange.start && + nested[0].end === openRange.end) { + ArrayPrototypePush(matchingTrees, nested[0]); + } else { + ArrayPrototypePush( + matchingTrees, + new RangeTree(openRange.start, openRange.end, 0, nested), + ); + } + } + parentToNested.clear(); + return mergeRangeTrees(matchingTrees); +} + +function mergeFunctionRanges(oldRanges, newRanges) { + const mergedTree = mergeRangeTrees([ + RangeTree.fromSortedRanges(oldRanges), + RangeTree.fromSortedRanges(newRanges), + ]); + mergedTree.normalize(); + return mergedTree.toRanges(); +} + +module.exports = { mergeFunctionRanges }; diff --git a/test/fixtures/test-runner/coverage-merge-nested/a.test.mjs b/test/fixtures/test-runner/coverage-merge-nested/a.test.mjs new file mode 100644 index 00000000000..455d6e0a175 --- /dev/null +++ b/test/fixtures/test-runner/coverage-merge-nested/a.test.mjs @@ -0,0 +1,8 @@ +import assert from 'node:assert'; +import { test } from 'node:test'; +import { f } from './mod.mjs'; + +test('a', () => { + assert.strictEqual(f(true, false), 'a'); + assert.strictEqual(f(false, false), 'other'); +}); diff --git a/test/fixtures/test-runner/coverage-merge-nested/b.test.mjs b/test/fixtures/test-runner/coverage-merge-nested/b.test.mjs new file mode 100644 index 00000000000..bee271d8e5e --- /dev/null +++ b/test/fixtures/test-runner/coverage-merge-nested/b.test.mjs @@ -0,0 +1,7 @@ +import assert from 'node:assert'; +import { test } from 'node:test'; +import { f } from './mod.mjs'; + +test('b', () => { + assert.strictEqual(f(false, false), 'other'); +}); diff --git a/test/fixtures/test-runner/coverage-merge-nested/mod.mjs b/test/fixtures/test-runner/coverage-merge-nested/mod.mjs new file mode 100644 index 00000000000..2ca1bfa2b8e --- /dev/null +++ b/test/fixtures/test-runner/coverage-merge-nested/mod.mjs @@ -0,0 +1,9 @@ +export const f = (a, b) => { + if (a) { + if (b) { + return 'never'; + } + return 'a'; + } + return 'other'; +}; diff --git a/test/parallel/test-runner-coverage-merge-nested.js b/test/parallel/test-runner-coverage-merge-nested.js new file mode 100644 index 00000000000..245ff26c388 --- /dev/null +++ b/test/parallel/test-runner-coverage-merge-nested.js @@ -0,0 +1,49 @@ +'use strict'; +// Validates that merging V8 coverage across test processes keeps never-run +// nested blocks. Without the range-tree merge, adding a second process that +// covers less can drop an uncovered nested block and make a coverage gate pass. +// See https://github.com/nodejs/node/issues/66860. +const common = require('../common'); +const assert = require('node:assert'); +const { spawnSync } = require('node:child_process'); +const { test } = require('node:test'); +const fixtures = require('../common/fixtures'); +const tmpdir = require('../common/tmpdir'); + +common.skipIfInspectorDisabled(); +tmpdir.refresh(); + +const fixture = fixtures.path('test-runner', 'coverage-merge-nested'); + +function runCoverage(...files) { + return spawnSync(process.execPath, [ + '--test', + '--experimental-test-coverage', + // Clear default excludes so fixture cwd files are reported, then include + // only mod.mjs (the shared module under test). + '--test-coverage-exclude=!**', + '--test-coverage-include=mod.mjs', + '--test-coverage-branches=100', + '--test-reporter', + 'tap', + ...files, + ], { + cwd: fixture, + env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path }, + encoding: 'utf8', + }); +} + +test('single process keeps the never-run nested block uncovered', () => { + const result = runCoverage('a.test.mjs'); + assert.match(result.stdout, /mod\.mjs\s+\|\s+[\d.]+\s+\|\s+80\.00/); + assert.match(result.stdout, /80\.00% branch coverage does not meet threshold/); + assert.strictEqual(result.status, 1); +}); + +test('merging a less-covered process must not clear the never-run nested block', () => { + const result = runCoverage('a.test.mjs', 'b.test.mjs'); + assert.match(result.stdout, /mod\.mjs\s+\|\s+[\d.]+\s+\|\s+80\.00/); + assert.match(result.stdout, /80\.00% branch coverage does not meet threshold/); + assert.strictEqual(result.status, 1); +}); diff --git a/test/parallel/test-runner-coverage.js b/test/parallel/test-runner-coverage.js index fa56c5af3e8..05243572f51 100644 --- a/test/parallel/test-runner-coverage.js +++ b/test/parallel/test-runner-coverage.js @@ -213,17 +213,19 @@ test('single process coverage is the same with --test', skipIfNoInspector, () => }); test('coverage is combined for multiple processes', skipIfNoInspector, () => { + // Branch % must match isolation=none: the range-tree merge keeps never-run + // nested blocks that V8 omits from some processes (nodejs/node#66860). let report = [ '# start of coverage report', '# -------------------------------------------------------------------', '# file | line % | branch % | funcs % | uncovered lines', '# -------------------------------------------------------------------', - '# common.js | 89.86 | 62.50 | 100.00 | 8 13-14 18 34-35 53', + '# common.js | 89.86 | 68.42 | 100.00 | 8 13-14 18 34-35 53', '# first.test.js | 83.33 | 100.00 | 50.00 | 5-6', '# second.test.js | 100.00 | 100.00 | 100.00 | ', '# third.test.js | 100.00 | 100.00 | 100.00 | ', '# -------------------------------------------------------------------', - '# all files | 92.11 | 72.73 | 88.89 | ', + '# all files | 92.11 | 76.00 | 88.89 | ', '# -------------------------------------------------------------------', '# end of coverage report', ].join('\n'); @@ -250,11 +252,7 @@ test('coverage is combined for multiple processes', skipIfNoInspector, () => { assert.strictEqual(result.status, 0); }); -test.skip('coverage works with isolation=none', skipIfNoInspector, common.mustCallAtLeast(() => { - // There is a bug in coverage calculation. The branch % in the common.js - // fixture is different depending on the test isolation mode. The 'none' mode - // is closer to what c8 reports here, so the bug is likely in the code that - // merges reports from different processes. +test('coverage works with isolation=none', skipIfNoInspector, () => { let report = [ '# start of coverage report', '# -------------------------------------------------------------------', @@ -278,6 +276,7 @@ test.skip('coverage works with isolation=none', skipIfNoInspector, common.mustCa const args = [ '--test', '--experimental-test-coverage', + '--test-coverage-exclude=!test/**', '--test-reporter', 'tap', '--test-isolation=none', @@ -290,7 +289,7 @@ test.skip('coverage works with isolation=none', skipIfNoInspector, common.mustCa assert.strictEqual(result.stderr.toString(), ''); assertIncludesReport(result, report); assert.strictEqual(result.status, 0); -}, 0)); +}); test('coverage reports on lines, functions, and branches', skipIfNoInspector, async (t) => { const fixture = fixtures.path('test-runner', 'coverage.js');