Blitzy: test_runner: bind built-in coverage exclusions to cwd-relative paths (fixes nodejs/node#58654)

lib/internal/test_runner/coverage.js

@@ -27,7 +27,7 @@ const {
 } = require('fs');
 const { setupCoverageHooks } = require('internal/util');
 const { tmpdir } = require('os');
-const { join, resolve, relative } = require('path');
+const { isAbsolute, join, resolve, relative, sep } = require('path');
 const { fileURLToPath, URL } = require('internal/url');
 const { kMappings, SourceMap } = require('internal/source_map/source_map');
 const {
@@ -38,12 +38,15 @@ const {
 } = require('internal/errors');
 const { matchGlobPattern } = require('internal/fs/glob');
 const { constants: { kMockSearchParam } } = require('internal/test_runner/mock/loader');
+const { kDefaultPattern } = require('internal/test_runner/utils');
 
 const kCoverageFileRegex = /^coverage-(\d+)-(\d{13})-(\d+)\.json$/;
 const kIgnoreRegex = /\/\* node:coverage ignore next (?<count>\d+ )?\*\//;
 const kLineEndingRegex = /\r?\n$/u;
 const kLineSplitRegex = /(?<=\r?\n)/u;
 const kStatusRegex = /\/\* node:coverage (?<status>enable|disable) \*\//;
+const kNodeModulesPrefix = `node_modules${sep}`;
+const kNodeModulesSegment = `${sep}node_modules${sep}`;
 
 class CoverageLine {
   constructor(line, startOffset, src, length = src?.length) {
@@ -472,18 +475,41 @@ class TestCoverage {
 
     const absolutePath = fileURLToPath(url);
     const relativePath = relative(this.options.cwd, absolutePath);
+    // relative() cannot express a file that lives on another Windows volume as a
+    // working directory relative path, so it returns an absolute path such as
+    // 'D:\test\project\src.js' instead. The built-in exclusions below only ever
+    // decide on a genuinely relative path, because matching them against an
+    // absolute one would let the ancestor directory names of that other volume
+    // empty the coverage report again. User supplied globs keep matching absolute
+    // paths, which is the documented behaviour of --test-coverage-exclude.
+    const isRelativeToCwd = !isAbsolute(relativePath);
     const {
       coverageExcludeGlobs: excludeGlobs,
       coverageIncludeGlobs: includeGlobs,
     } = this.options;
 
-    // This check filters out files that match the exclude globs.
+    // This check filters out files that match the exclude globs. User supplied
+    // globs are documented to match both absolute and relative paths, but the
+    // built-in default pattern is not user supplied: it is the very same pattern
+    // that test discovery resolves relative to the working directory (see
+    // createTestFileList() in internal/test_runner/runner.js). Matching its
+    // 'test/**/*' alternative against the absolute path made every file of a
+    // project stored below a directory named 'test' (for example
+    // /home/test/my-project) look like a test file, which silently emptied the
+    // coverage report. Refs: https://github.com/nodejs/node/issues/58654
     if (excludeGlobs?.length > 0) {
       for (let i = 0; i < excludeGlobs.length; ++i) {
-        if (
-          matchGlobPattern(relativePath, excludeGlobs[i]) ||
-          matchGlobPattern(absolutePath, excludeGlobs[i])
-        ) return true;
+        const excludeGlob = excludeGlobs[i];
+
+        // Skip the built-in default for files that have no working directory
+        // relative path at all, so that it can never match an absolute one.
+        if (excludeGlob === kDefaultPattern && !isRelativeToCwd) continue;
+
+        if (matchGlobPattern(relativePath, excludeGlob) ||
+            (excludeGlob !== kDefaultPattern &&
+             matchGlobPattern(absolutePath, excludeGlob))) {
+          return true;
+        }
       }
     }
 
@@ -503,8 +529,15 @@ class TestCoverage {
       return true;
     }
 
-    // This check filters out the node_modules/ directory, unless it is explicitly included.
-    return StringPrototypeIncludes(url, '/node_modules/');
+    // This check filters out the node_modules/ directory, unless it is explicitly
+    // included. Like the default exclusion above, the check runs on the working
+    // directory relative path so that a project stored below a directory named
+    // node_modules is not excluded in its entirety. This mirrors test discovery,
+    // which skips node_modules by directory name. A file that has no working
+    // directory relative path cannot be classified this way, so it is reported.
+    return isRelativeToCwd &&
+           (StringPrototypeStartsWith(relativePath, kNodeModulesPrefix) ||
+            StringPrototypeIncludes(relativePath, kNodeModulesSegment));
   }
 }
 

test/fixtures/test-runner/coverage-relative-exclusion/file.test.mjs

@@ -0,0 +1,9 @@
+import test from 'node:test';
+import assert from 'node:assert';
+import { foo } from './logic-file.js';
+import dependency from './node_modules/dependency.js';
+
+test('foo returns 1', () => {
+    assert.strictEqual(foo(), 1);
+    assert.strictEqual(dependency.helper(), 'helper');
+});

test/fixtures/test-runner/coverage-relative-exclusion/logic-file.js

@@ -0,0 +1,9 @@
+function foo() {
+    return 1;
+}
+
+function bar() {
+    return 'bar';
+}
+
+module.exports = { foo, bar };

test/parallel/test-runner-coverage-default-exclusion.mjs

@@ -14,6 +14,13 @@ tmpdir.refresh();
 async function setupFixtures() {
   const fixtureDir = fixtures.path('test-runner', 'coverage-default-exclusion');
   await cp(fixtureDir, tmpdir.path, { recursive: true });
+
+  // Both copies live below a directory named node_modules on purpose: it keeps them
+  // invisible to the test discovery of the cases that run from tmpdir.path, and it
+  // reproduces a project whose absolute path contains a node_modules segment.
+  const relativeFixtureDir = fixtures.path('test-runner', 'coverage-relative-exclusion');
+  await cp(relativeFixtureDir, tmpdir.resolve('node_modules', 'project'), { recursive: true });
+  await cp(relativeFixtureDir, tmpdir.resolve('node_modules', 'absolute-glob-project'), { recursive: true });
 }
 
 describe('test runner coverage default exclusion', skipIfNoInspector, () => {
@@ -114,4 +121,104 @@ describe('test runner coverage default exclusion', skipIfNoInspector, () => {
     assert(result.stdout.toString().includes(report));
     assert.strictEqual(result.status, 0);
   });
+
+  // Regression test for https://github.com/nodejs/node/issues/58654: the default
+  // exclusion must be matched against working directory relative paths only, so a
+  // directory named test above the project must not exclude the whole project.
+  it('should not exclude files because an ancestor directory is named test', async () => {
+    const report = [
+      '# start of coverage report',
+      '# --------------------------------------------------------------',
+      '# file          | line % | branch % | funcs % | uncovered lines',
+      '# --------------------------------------------------------------',
+      '# logic-file.js |  66.67 |   100.00 |   50.00 | 5-7',
+      '# --------------------------------------------------------------',
+      '# all files     |  66.67 |   100.00 |   50.00 | ',
+      '# --------------------------------------------------------------',
+      '# end of coverage report',
+    ].join('\n');
+
+    const args = [
+      '--no-experimental-strip-types',
+      '--test',
+      '--experimental-test-coverage',
+      '--test-reporter=tap',
+    ];
+    const result = spawnSync(process.execPath, args, {
+      env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
+      cwd: fixtures.path('test-runner', 'coverage-relative-exclusion')
+    });
+
+    assert.strictEqual(result.stderr.toString(), '');
+    assert(result.stdout.toString().includes(report));
+    assert.strictEqual(result.status, 0);
+  });
+
+  // The node_modules exclusion must likewise be decided from the working directory
+  // relative path, so an ancestor node_modules directory cannot exclude the project.
+  // The run starts one level above the staged project so that the project's own
+  // dependency is relative as project/node_modules/dependency.js: that keeps the
+  // interior node_modules segment covered, which a top level only check would miss.
+  it('should not exclude files because an ancestor directory is named node_modules', async () => {
+    const report = [
+      '# start of coverage report',
+      '# ---------------------------------------------------------------',
+      '# file           | line % | branch % | funcs % | uncovered lines',
+      '# ---------------------------------------------------------------',
+      '# project        |        |          |         | ',
+      '#  logic-file.js |  66.67 |   100.00 |   50.00 | 5-7',
+      '# ---------------------------------------------------------------',
+      '# all files      |  66.67 |   100.00 |   50.00 | ',
+      '# ---------------------------------------------------------------',
+      '# end of coverage report',
+    ].join('\n');
+
+    const args = [
+      '--no-experimental-strip-types',
+      '--test',
+      '--experimental-test-coverage',
+      '--test-reporter=tap',
+      'project/file.test.mjs',
+    ];
+    const result = spawnSync(process.execPath, args, {
+      env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
+      cwd: tmpdir.resolve('node_modules')
+    });
+
+    assert.strictEqual(result.stderr.toString(), '');
+    assert(result.stdout.toString().includes(report));
+    assert.strictEqual(result.status, 0);
+  });
+
+  // User supplied globs are documented to match absolute paths as well, and that
+  // contract must survive the change above.
+  it('should still exclude files matching a user supplied absolute glob', async () => {
+    const report = [
+      '# start of coverage report',
+      '# --------------------------------------------------------------',
+      '# file          | line % | branch % | funcs % | uncovered lines',
+      '# --------------------------------------------------------------',
+      '# file.test.mjs | 100.00 |   100.00 |  100.00 | ',
+      '# --------------------------------------------------------------',
+      '# all files     | 100.00 |   100.00 |  100.00 | ',
+      '# --------------------------------------------------------------',
+      '# end of coverage report',
+    ].join('\n');
+
+    const args = [
+      '--no-experimental-strip-types',
+      '--test',
+      '--experimental-test-coverage',
+      `--test-coverage-exclude=${tmpdir.resolve('node_modules', 'absolute-glob-project', 'logic-file.js')}`,
+      '--test-reporter=tap',
+    ];
+    const result = spawnSync(process.execPath, args, {
+      env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
+      cwd: tmpdir.resolve('node_modules', 'absolute-glob-project')
+    });
+
+    assert.strictEqual(result.stderr.toString(), '');
+    assert(result.stdout.toString().includes(report));
+    assert.strictEqual(result.status, 0);
+  });
 });