test_runner: exclude test files from coverage by default

pmarchini · aduh95 · commit ea9a675f56a2 · 2024-12-18T22:31:05.000+01:00
PR-URL: #56060 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -2267,6 +2267,9 @@ This option may be specified multiple times to exclude multiple glob patterns.
 If both `--test-coverage-exclude` and `--test-coverage-include` are provided,
 files must meet **both** criteria to be included in the coverage report.
 
+By default all the matching test files are excluded from the coverage report.
+Specifying this option will override the default behavior.
+
 ### `--test-coverage-functions=threshold`
 
 <!-- YAML
diff --git a/doc/api/test.md b/doc/api/test.md
@@ -476,8 +476,10 @@ all tests have completed. If the [`NODE_V8_COVERAGE`][] environment variable is
 used to specify a code coverage directory, the generated V8 coverage files are
 written to that directory. Node.js core modules and files within
 `node_modules/` directories are, by default, not included in the coverage report.
-However, they can be explicitly included via the [`--test-coverage-include`][] flag. If
-coverage is enabled, the coverage report is sent to any [test reporters][] via
+However, they can be explicitly included via the [`--test-coverage-include`][] flag.
+By default all the matching test files are excluded from the coverage report.
+Exclusions can be overridden by using the [`--test-coverage-exclude`][] flag.
+If coverage is enabled, the coverage report is sent to any [test reporters][] via
 the `'test:coverage'` event.
 
 Coverage can be disabled on a series of lines using the following
@@ -3592,6 +3594,7 @@ Can be used to abort test subtasks when the test has been aborted.
 [`--experimental-test-module-mocks`]: cli.md#--experimental-test-module-mocks
 [`--import`]: cli.md#--importmodule
 [`--test-concurrency`]: cli.md#--test-concurrency
+[`--test-coverage-exclude`]: cli.md#--test-coverage-exclude
 [`--test-coverage-include`]: cli.md#--test-coverage-include
 [`--test-name-pattern`]: cli.md#--test-name-pattern
 [`--test-only`]: cli.md#--test-only
diff --git a/lib/internal/fs/glob.js b/lib/internal/fs/glob.js
@@ -650,7 +650,30 @@ class Glob {
   }
 }
 
+/**
+ * Check if a path matches a glob pattern
+ * @param {string} path the path to check
+ * @param {string} pattern the glob pattern to match
+ * @param {boolean} windows whether the path is on a Windows system, defaults to `isWindows`
+ * @returns {boolean}
+ */
+function matchGlobPattern(path, pattern, windows = isWindows) {
+  validateString(path, 'path');
+  validateString(pattern, 'pattern');
+  return lazyMinimatch().minimatch(path, pattern, {
+    kEmptyObject,
+    nocase: isMacOS || isWindows,
+    windowsPathsNoEscape: true,
+    nonegate: true,
+    nocomment: true,
+    optimizationLevel: 2,
+    platform: windows ? 'win32' : 'posix',
+    nocaseMagicOnly: true,
+  });
+}
+
 module.exports = {
   __proto__: null,
   Glob,
+  matchGlobPattern,
 };
diff --git a/lib/internal/test_runner/coverage.js b/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, matchesGlob } = require('path');
+const { join, resolve, relative } = require('path');
 const { fileURLToPath } = require('internal/url');
 const { kMappings, SourceMap } = require('internal/source_map/source_map');
 const {
@@ -36,6 +36,8 @@ const {
     ERR_SOURCE_MAP_MISSING_SOURCE,
   },
 } = require('internal/errors');
+const { matchGlobPattern } = require('internal/fs/glob');
+
 const kCoverageFileRegex = /^coverage-(\d+)-(\d{13})-(\d+)\.json$/;
 const kIgnoreRegex = /\/\* node:coverage ignore next (?<count>\d+ )?\*\//;
 const kLineEndingRegex = /\r?\n$/u;
@@ -464,19 +466,24 @@ class TestCoverage {
       coverageExcludeGlobs: excludeGlobs,
       coverageIncludeGlobs: includeGlobs,
     } = this.options;
+
     // This check filters out files that match the exclude globs.
     if (excludeGlobs?.length > 0) {
       for (let i = 0; i < excludeGlobs.length; ++i) {
-        if (matchesGlob(relativePath, excludeGlobs[i]) ||
-            matchesGlob(absolutePath, excludeGlobs[i])) return true;
+        if (
+          matchGlobPattern(relativePath, excludeGlobs[i]) ||
+          matchGlobPattern(absolutePath, excludeGlobs[i])
+        ) return true;
       }
     }
 
     // This check filters out files that do not match the include globs.
     if (includeGlobs?.length > 0) {
       for (let i = 0; i < includeGlobs.length; ++i) {
-        if (matchesGlob(relativePath, includeGlobs[i]) ||
-            matchesGlob(absolutePath, includeGlobs[i])) return false;
+        if (
+          matchGlobPattern(relativePath, includeGlobs[i]) ||
+          matchGlobPattern(absolutePath, includeGlobs[i])
+        ) return false;
       }
       return true;
     }
diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js
@@ -287,6 +287,11 @@ function parseCommandLine() {
 
   if (coverage) {
     coverageExcludeGlobs = getOptionValue('--test-coverage-exclude');
+    if (!coverageExcludeGlobs || coverageExcludeGlobs.length === 0) {
+      // TODO(pmarchini): this default should follow something similar to c8 defaults
+      // Default exclusions should be also exported to be used by other tools / users
+      coverageExcludeGlobs = [kDefaultPattern];
+    }
     coverageIncludeGlobs = getOptionValue('--test-coverage-include');
 
     branchCoverage = getOptionValue('--test-coverage-branches');
diff --git a/lib/path.js b/lib/path.js
@@ -52,13 +52,12 @@ const {
 } = require('internal/validators');
 
 const {
-  getLazy,
   emitExperimentalWarning,
   isWindows,
-  isMacOS,
+  getLazy,
 } = require('internal/util');
 
-const lazyMinimatch = getLazy(() => require('internal/deps/minimatch/index'));
+const lazyMatchGlobPattern = getLazy(() => require('internal/fs/glob').matchGlobPattern);
 
 function isPathSeparator(code) {
   return code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH;
@@ -164,22 +163,6 @@ function _format(sep, pathObject) {
   return dir === pathObject.root ? `${dir}${base}` : `${dir}${sep}${base}`;
 }
 
-function glob(path, pattern, windows) {
-  emitExperimentalWarning('glob');
-  validateString(path, 'path');
-  validateString(pattern, 'pattern');
-  return lazyMinimatch().minimatch(path, pattern, {
-    __proto__: null,
-    nocase: isMacOS || isWindows,
-    windowsPathsNoEscape: true,
-    nonegate: true,
-    nocomment: true,
-    optimizationLevel: 2,
-    platform: windows ? 'win32' : 'posix',
-    nocaseMagicOnly: true,
-  });
-}
-
 const win32 = {
   /**
    * path.resolve([from ...], to)
@@ -1140,7 +1123,8 @@ const win32 = {
   },
 
   matchesGlob(path, pattern) {
-    return glob(path, pattern, true);
+    emitExperimentalWarning('glob');
+    return lazyMatchGlobPattern()(path, pattern, true);
   },
 
   sep: '\\',
@@ -1616,7 +1600,8 @@ const posix = {
   },
 
   matchesGlob(path, pattern) {
-    return glob(path, pattern, false);
+    emitExperimentalWarning('glob');
+    return lazyMatchGlobPattern()(path, pattern, false);
   },
 
   sep: '/',
diff --git a/test/fixtures/test-runner/coverage-default-exclusion/file-test.js b/test/fixtures/test-runner/coverage-default-exclusion/file-test.js
@@ -0,0 +1,7 @@
+const test = require('node:test');
+const assert = require('node:assert');
+const { foo } = require('./logic-file');
+
+test('foo returns 1', () => {
+    assert.strictEqual(foo(), 1);
+});
diff --git a/test/fixtures/test-runner/coverage-default-exclusion/file.test.mjs b/test/fixtures/test-runner/coverage-default-exclusion/file.test.mjs
@@ -0,0 +1,7 @@
+import test from 'node:test';
+import assert from 'node:assert';
+import { foo } from './logic-file.js';
+
+test('foo returns 1', () => {
+    assert.strictEqual(foo(), 1);
+});
diff --git a/test/fixtures/test-runner/coverage-default-exclusion/file.test.ts b/test/fixtures/test-runner/coverage-default-exclusion/file.test.ts
@@ -0,0 +1,7 @@
+import test from 'node:test';
+import assert from 'node:assert';
+import { foo } from './logic-file.js';
+
+test('foo returns 1', () => {
+    assert.strictEqual(foo(), 1);
+});
diff --git a/test/fixtures/test-runner/coverage-default-exclusion/logic-file.js b/test/fixtures/test-runner/coverage-default-exclusion/logic-file.js
@@ -0,0 +1,9 @@
+function foo() {
+    return 1;
+}
+
+function bar() {
+    return 'bar';
+}
+
+module.exports = { foo, bar };
diff --git a/test/fixtures/test-runner/coverage-default-exclusion/test.cjs b/test/fixtures/test-runner/coverage-default-exclusion/test.cjs
@@ -0,0 +1,7 @@
+const test = require('node:test');
+const assert = require('node:assert');
+const { foo } = require('./logic-file.js');
+
+test('foo returns 1', () => {
+    assert.strictEqual(foo(), 1);
+});
diff --git a/test/fixtures/test-runner/coverage-default-exclusion/test/not-matching-test-name.js b/test/fixtures/test-runner/coverage-default-exclusion/test/not-matching-test-name.js
@@ -0,0 +1,7 @@
+const test = require('node:test');
+const assert = require('node:assert');
+const { foo } = require('../logic-file.js');
+
+test('foo returns 1', () => {
+    assert.strictEqual(foo(), 1);
+});
diff --git a/test/fixtures/test-runner/output/lcov_reporter.js b/test/fixtures/test-runner/output/lcov_reporter.js
@@ -4,4 +4,13 @@ const fixtures = require('../../../common/fixtures');
 const spawn = require('node:child_process').spawn;
 
 spawn(process.execPath,
-      ['--no-warnings', '--experimental-test-coverage', '--test-reporter', 'lcov', fixtures.path('test-runner/output/output.js')], { stdio: 'inherit' });
+      [
+            '--no-warnings',
+            '--experimental-test-coverage',
+            '--test-coverage-exclude=!test/**',
+            '--test-reporter',
+            'lcov',
+            fixtures.path('test-runner/output/output.js')
+      ],
+      { stdio: 'inherit' }
+);
diff --git a/test/parallel/test-runner-coverage-default-exclusion.mjs b/test/parallel/test-runner-coverage-default-exclusion.mjs
@@ -0,0 +1,116 @@
+import '../common/index.mjs';
+import { before, describe, it } from 'node:test';
+import assert from 'node:assert';
+import { spawnSync } from 'node:child_process';
+import { cp } from 'node:fs/promises';
+import tmpdir from '../common/tmpdir.js';
+import fixtures from '../common/fixtures.js';
+const skipIfNoInspector = {
+  skip: !process.features.inspector ? 'inspector disabled' : false
+};
+
+tmpdir.refresh();
+
+async function setupFixtures() {
+  const fixtureDir = fixtures.path('test-runner', 'coverage-default-exclusion');
+  await cp(fixtureDir, tmpdir.path, { recursive: true });
+}
+
+describe('test runner coverage default exclusion', skipIfNoInspector, () => {
+  before(async () => {
+    await setupFixtures();
+  });
+
+  it('should override default exclusion setting --test-coverage-exclude', async () => {
+    const report = [
+      '# start of coverage report',
+      '# ---------------------------------------------------------------------------',
+      '# file                       | line % | branch % | funcs % | uncovered lines',
+      '# ---------------------------------------------------------------------------',
+      '# file-test.js               | 100.00 |   100.00 |  100.00 | ',
+      '# file.test.mjs              | 100.00 |   100.00 |  100.00 | ',
+      '# logic-file.js              |  66.67 |   100.00 |   50.00 | 5-7',
+      '# test.cjs                   | 100.00 |   100.00 |  100.00 | ',
+      '# test                       |        |          |         | ',
+      '#  not-matching-test-name.js | 100.00 |   100.00 |  100.00 | ',
+      '# ---------------------------------------------------------------------------',
+      '# all files                  |  91.89 |   100.00 |   83.33 | ',
+      '# ---------------------------------------------------------------------------',
+      '# end of coverage report',
+    ].join('\n');
+
+
+    const args = [
+      '--test',
+      '--experimental-test-coverage',
+      '--test-coverage-exclude=!test/**',
+      '--test-reporter=tap',
+    ];
+    const result = spawnSync(process.execPath, args, {
+      env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
+      cwd: tmpdir.path
+    });
+
+    assert.strictEqual(result.stderr.toString(), '');
+    assert(result.stdout.toString().includes(report));
+    assert.strictEqual(result.status, 0);
+  });
+
+  it('should exclude test files from coverage by default', 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 = [
+      '--test',
+      '--experimental-test-coverage',
+      '--test-reporter=tap',
+    ];
+    const result = spawnSync(process.execPath, args, {
+      env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
+      cwd: tmpdir.path
+    });
+
+    assert.strictEqual(result.stderr.toString(), '');
+    assert(result.stdout.toString().includes(report));
+    assert.strictEqual(result.status, 0);
+  });
+
+  it('should exclude ts test files when using --experimental-strip-types', 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 = [
+      '--test',
+      '--experimental-test-coverage',
+      '--experimental-strip-types',
+      '--disable-warning=ExperimentalWarning',
+      '--test-reporter=tap',
+    ];
+    const result = spawnSync(process.execPath, args, {
+      env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
+      cwd: tmpdir.path
+    });
+
+    assert.strictEqual(result.stderr.toString(), '');
+    assert(result.stdout.toString().includes(report));
+    assert.strictEqual(result.status, 0);
+  });
+});
diff --git a/test/parallel/test-runner-coverage-source-map.js b/test/parallel/test-runner-coverage-source-map.js
@@ -20,7 +20,11 @@ function generateReport(report) {
 
 const flags = [
   '--enable-source-maps',
-  '--test', '--experimental-test-coverage', '--test-reporter', 'tap',
+  '--test',
+  '--experimental-test-coverage',
+  '--test-coverage-exclude=!test/**',
+  '--test-reporter',
+  'tap',
 ];
 
 describe('Coverage with source maps', async () => {
diff --git a/test/parallel/test-runner-coverage-thresholds.js b/test/parallel/test-runner-coverage-thresholds.js
diff --git a/test/parallel/test-runner-coverage.js b/test/parallel/test-runner-coverage.js
diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs
