test_runner: exclude test files from code coverage by default

Is not usual to test the code coverage of test files. Fixes: nodejs#53508
Llorx · Nov 2, 2024 · c5c8ea7 · c5c8ea7
1 parent d37214b
commit c5c8ea7
Show file tree

Hide file tree

Showing 9 changed files with 129 additions and 25 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -2352,6 +2352,9 @@ This option may be specified multiple times to include 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, the files being tested are excluded from code coverage. They can be explicitly
+included via this flag.
+
 ### `--test-coverage-lines=threshold`
 
 <!-- YAML

diff --git a/doc/api/test.md b/doc/api/test.md
@@ -1359,6 +1359,8 @@ changes:
     This property is only applicable when `coverage` was set to `true`.
     If both `coverageExcludeGlobs` and `coverageIncludeGlobs` are provided,
     files must meet **both** criteria to be included in the coverage report.
+    By default, the files being tested are excluded from code coverage. They can be explicitly
+	included via this parameter.
     **Default:** `undefined`.
   * `lineCoverage` {number} Require a minimum percent of covered lines. If code
     coverage does not reach the threshold specified, the process will exit with code `1`.

diff --git a/lib/internal/test_runner/coverage.js b/lib/internal/test_runner/coverage.js
@@ -1,6 +1,7 @@
 'use strict';
 const {
   ArrayFrom,
+  ArrayPrototypeIncludes,
   ArrayPrototypeMap,
   ArrayPrototypePush,
   JSONParse,
@@ -463,6 +464,7 @@ class TestCoverage {
     const {
       coverageExcludeGlobs: excludeGlobs,
       coverageIncludeGlobs: includeGlobs,
+      testFiles: testFiles,
     } = this.options;
     // This check filters out files that match the exclude globs.
     if (excludeGlobs?.length > 0) {
@@ -481,8 +483,8 @@ 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 and the test files, unless they are explicitly included.
+    return StringPrototypeIncludes(url, '/node_modules/') || ArrayPrototypeIncludes(testFiles, absolutePath);
   }
 }
 

diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -13,7 +13,7 @@ const {
   createHook,
   executionAsyncId,
 } = require('async_hooks');
-const { relative } = require('path');
+const { relative, resolve } = require('path');
 const {
   codes: {
     ERR_TEST_FAILURE,
@@ -261,6 +261,7 @@ function lazyBootstrapRoot() {
     };
     const globalOptions = parseCommandLine();
     globalOptions.cwd = process.cwd();
+    globalOptions.testFiles = entryFile ? [resolve(globalOptions.cwd, entryFile)] : [];
     createTestTree(rootTestOptions, globalOptions);
     globalRoot.reporter.on('test:summary', (data) => {
       if (!data.success) {

diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
@@ -666,6 +666,8 @@ function run(options = kEmptyObject) {
   validateStringArray(execArgv, 'options.execArgv');
 
   const rootTestOptions = { __proto__: null, concurrency, timeout, signal };
+  let testFiles = files ?? createTestFileList(globPatterns, cwd);
+  const absoluteTestFiles = ArrayPrototypeMap(testFiles, (file) => resolve(cwd, file));
   const globalOptions = {
     __proto__: null,
     // parseCommandLine() should not be used here. However, The existing run()
@@ -675,13 +677,13 @@ function run(options = kEmptyObject) {
     coverage,
     coverageExcludeGlobs,
     coverageIncludeGlobs,
+    testFiles: absoluteTestFiles,
     lineCoverage: lineCoverage,
     branchCoverage: branchCoverage,
     functionCoverage: functionCoverage,
     cwd,
   };
   const root = createTestTree(rootTestOptions, globalOptions);
-  let testFiles = files ?? createTestFileList(globPatterns, cwd);
 
   if (shard) {
     testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1);
@@ -733,7 +735,6 @@ function run(options = kEmptyObject) {
     };
   } else if (isolation === 'none') {
     if (watch) {
-      const absoluteTestFiles = ArrayPrototypeMap(testFiles, (file) => (isAbsolute(file) ? file : resolve(cwd, file)));
       filesWatcher = watchFiles(absoluteTestFiles, opts);
       runFiles = async () => {
         root.harness.bootstrapPromise = null;

diff --git a/test/parallel/test-runner-coverage-source-map.js b/test/parallel/test-runner-coverage-source-map.js
@@ -20,6 +20,7 @@ function generateReport(report) {
 
 const flags = [
   '--enable-source-maps',
+  '--no-warnings', '--test-coverage-include=**',
   '--test', '--experimental-test-coverage', '--test-reporter', 'tap',
 ];
 

diff --git a/test/parallel/test-runner-coverage-thresholds.js b/test/parallel/test-runner-coverage-thresholds.js
@@ -61,6 +61,8 @@ for (const coverage of coverages) {
     const result = spawnSync(process.execPath, [
       '--test',
       '--experimental-test-coverage',
+      '--no-warnings',
+      '--test-coverage-include=**',
       `${coverage.flag}=25`,
       '--test-reporter', 'tap',
       fixture,
@@ -77,6 +79,8 @@ for (const coverage of coverages) {
     const result = spawnSync(process.execPath, [
       '--test',
       '--experimental-test-coverage',
+      '--no-warnings',
+      '--test-coverage-include=**',
       `${coverage.flag}=25`,
       '--test-reporter', reporter,
       fixture,
@@ -92,6 +96,8 @@ for (const coverage of coverages) {
     const result = spawnSync(process.execPath, [
       '--test',
       '--experimental-test-coverage',
+      '--no-warnings',
+      '--test-coverage-include=**',
       `${coverage.flag}=99`,
       '--test-reporter', 'tap',
       fixture,
@@ -108,6 +114,8 @@ for (const coverage of coverages) {
     const result = spawnSync(process.execPath, [
       '--test',
       '--experimental-test-coverage',
+      '--no-warnings',
+      '--test-coverage-include=**',
       `${coverage.flag}=99`,
       '--test-reporter', reporter,
       fixture,
@@ -123,6 +131,8 @@ for (const coverage of coverages) {
     const result = spawnSync(process.execPath, [
       '--test',
       '--experimental-test-coverage',
+      '--no-warnings',
+      '--test-coverage-include=**',
       `${coverage.flag}=101`,
       fixture,
     ]);
@@ -136,6 +146,8 @@ for (const coverage of coverages) {
     const result = spawnSync(process.execPath, [
       '--test',
       '--experimental-test-coverage',
+      '--no-warnings',
+      '--test-coverage-include=**',
       `${coverage.flag}=-1`,
       fixture,
     ]);

diff --git a/test/parallel/test-runner-coverage.js b/test/parallel/test-runner-coverage.js
@@ -97,7 +97,11 @@ test('test coverage report', async (t) => {
 test('test tap coverage reporter', skipIfNoInspector, async (t) => {
   await t.test('coverage is reported and dumped to NODE_V8_COVERAGE if present', (t) => {
     const fixture = fixtures.path('test-runner', 'coverage.js');
-    const args = ['--experimental-test-coverage', '--test-reporter', 'tap', fixture];
+    const args = [
+      '--experimental-test-coverage',
+      '--no-warnings', '--test-coverage-include=**',
+      '--test-reporter', 'tap', fixture,
+    ];
     const options = { env: { ...process.env, NODE_V8_COVERAGE: tmpdir.path } };
     const result = spawnSync(process.execPath, args, options);
     const report = getTapCoverageFixtureReport();
@@ -109,7 +113,11 @@ test('test tap coverage reporter', skipIfNoInspector, async (t) => {
 
   await t.test('coverage is reported without NODE_V8_COVERAGE present', (t) => {
     const fixture = fixtures.path('test-runner', 'coverage.js');
-    const args = ['--experimental-test-coverage', '--test-reporter', 'tap', fixture];
+    const args = [
+      '--experimental-test-coverage',
+      '--no-warnings', '--test-coverage-include=**',
+      '--test-reporter', 'tap', fixture,
+    ];
     const result = spawnSync(process.execPath, args);
     const report = getTapCoverageFixtureReport();
 
@@ -123,7 +131,11 @@ test('test tap coverage reporter', skipIfNoInspector, async (t) => {
 test('test spec coverage reporter', skipIfNoInspector, async (t) => {
   await t.test('coverage is reported and dumped to NODE_V8_COVERAGE if present', (t) => {
     const fixture = fixtures.path('test-runner', 'coverage.js');
-    const args = ['--experimental-test-coverage', '--test-reporter', 'spec', fixture];
+    const args = [
+      '--experimental-test-coverage',
+      '--no-warnings', '--test-coverage-include=**',
+      '--test-reporter', 'spec', fixture,
+    ];
     const options = { env: { ...process.env, NODE_V8_COVERAGE: tmpdir.path } };
     const result = spawnSync(process.execPath, args, options);
     const report = getSpecCoverageFixtureReport();
@@ -136,7 +148,11 @@ test('test spec coverage reporter', skipIfNoInspector, async (t) => {
 
   await t.test('coverage is reported without NODE_V8_COVERAGE present', (t) => {
     const fixture = fixtures.path('test-runner', 'coverage.js');
-    const args = ['--experimental-test-coverage', '--test-reporter', 'spec', fixture];
+    const args = [
+      '--experimental-test-coverage',
+      '--no-warnings', '--test-coverage-include=**',
+      '--test-reporter', 'spec', fixture,
+    ];
     const result = spawnSync(process.execPath, args);
     const report = getSpecCoverageFixtureReport();
 
@@ -150,7 +166,9 @@ test('test spec coverage reporter', skipIfNoInspector, async (t) => {
 test('single process coverage is the same with --test', skipIfNoInspector, () => {
   const fixture = fixtures.path('test-runner', 'coverage.js');
   const args = [
-    '--test', '--experimental-test-coverage', '--test-reporter', 'tap', fixture,
+    '--test', '--experimental-test-coverage',
+    '--no-warnings', '--test-coverage-include=**',
+    '--test-reporter', 'tap', fixture,
   ];
   const result = spawnSync(process.execPath, args);
   const report = getTapCoverageFixtureReport();
@@ -183,7 +201,7 @@ test('coverage is combined for multiple processes', skipIfNoInspector, () => {
 
   const fixture = fixtures.path('v8-coverage', 'combined_coverage');
   const args = [
-    '--test', '--experimental-test-coverage', '--test-reporter', 'tap',
+    '--test', '--experimental-test-coverage', '--no-warnings', '--test-coverage-include=**', '--test-reporter', 'tap',
   ];
   const result = spawnSync(process.execPath, args, {
     env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
@@ -236,7 +254,8 @@ test.skip('coverage works with isolation=none', skipIfNoInspector, () => {
 test('coverage reports on lines, functions, and branches', skipIfNoInspector, async (t) => {
   const fixture = fixtures.path('test-runner', 'coverage.js');
   const child = spawnSync(process.execPath,
-                          ['--test', '--experimental-test-coverage', '--test-reporter',
+                          ['--test', '--experimental-test-coverage',
+                           '--no-warnings', '--test-coverage-include=**', '--test-reporter',
                            fixtures.fileURL('test-runner/custom_reporters/coverage.mjs'),
                            fixture]);
   assert.strictEqual(child.stderr.toString(), '');
@@ -297,7 +316,6 @@ test('coverage with ESM hook - source irrelevant', skipIfNoInspector, () => {
     '# ------------------------------------------------------------------',
     '# hooks.mjs         | 100.00 |   100.00 |  100.00 | ',
     '# register-hooks.js | 100.00 |   100.00 |  100.00 | ',
-    '# virtual.js        | 100.00 |   100.00 |  100.00 | ',
     '# ------------------------------------------------------------------',
     '# all files         | 100.00 |   100.00 |  100.00 | ',
     '# ------------------------------------------------------------------',
@@ -327,7 +345,6 @@ test('coverage with ESM hook - source transpiled', skipIfNoInspector, () => {
     '# ------------------------------------------------------------------',
     '# hooks.mjs         | 100.00 |   100.00 |  100.00 | ',
     '# register-hooks.js | 100.00 |   100.00 |  100.00 | ',
-    '# sum.test.ts       | 100.00 |   100.00 |  100.00 | ',
     '# sum.ts            | 100.00 |   100.00 |  100.00 | ',
     '# ------------------------------------------------------------------',
     '# all files         | 100.00 |   100.00 |  100.00 | ',
@@ -384,7 +401,38 @@ test('coverage with excluded files', skipIfNoInspector, () => {
   assert.strictEqual(result.status, 0);
   assert(!findCoverageFileForPid(result.pid));
 });
+test('coverage should not include test files by default', skipIfNoInspector, () => {
+  const fixture = fixtures.path('test-runner', 'coverage.js');
+  const args = [
+    '--experimental-test-coverage', '--test-reporter', 'tap',
+    fixture,
+  ];
+  const result = spawnSync(process.execPath, args);
+  const report = [
+    '# start of coverage report',
+    '# ---------------------------------------------------------------',
+    '# file           | line % | branch % | funcs % | uncovered lines',
+    '# ---------------------------------------------------------------',
+    '# test           |        |          |         | ',
+    '#  fixtures      |        |          |         | ',
+    '#   test-runner  |        |          |         | ',
+    '#   v8-coverage  |        |          |         | ',
+    '#    throw.js    |  71.43 |    50.00 |  100.00 | 5-6',
+    '# ---------------------------------------------------------------',
+    '# all files      |  78.13 |    40.00 |   60.00 | ',
+    '# ---------------------------------------------------------------',
+    '# end of coverage report',
+  ].join('\n');
 
+
+  if (common.isWindows) {
+    return report.replaceAll('/', '\\');
+  }
+
+  assert(result.stdout.toString().includes(report));
+  assert.strictEqual(result.status, 0);
+  assert(!findCoverageFileForPid(result.pid));
+});
 test('coverage with included files', skipIfNoInspector, () => {
   const fixture = fixtures.path('test-runner', 'coverage.js');
   const args = [
@@ -458,18 +506,17 @@ test('coverage with included and excluded files', skipIfNoInspector, () => {
 test('correctly prints the coverage report of files contained in parent directories', skipIfNoInspector, () => {
   let report = [
     '# start of coverage report',
-    '# --------------------------------------------------------------------------------------------',
+    '# ------------------------------------------------------------------',
     '# file              | line % | branch % | funcs % | uncovered lines',
-    '# --------------------------------------------------------------------------------------------',
+    '# ------------------------------------------------------------------',
     '# ..                |        |          |         | ',
-    '#  coverage.js      |  78.65 |    38.46 |   60.00 | 12-13 16-22 27 39 43-44 61-62 66-67 71-72',
     '#  invalid-tap.js   | 100.00 |   100.00 |  100.00 | ',
     '#  ..               |        |          |         | ',
     '#   v8-coverage     |        |          |         | ',
     '#    throw.js       |  71.43 |    50.00 |  100.00 | 5-6',
-    '# --------------------------------------------------------------------------------------------',
-    '# all files         |  78.35 |    43.75 |   60.00 | ',
-    '# --------------------------------------------------------------------------------------------',
+    '# ------------------------------------------------------------------',
+    '# all files         |  75.00 |    66.67 |  100.00 | ',
+    '# ------------------------------------------------------------------',
     '# end of coverage report',
   ].join('\n');
 

diff --git a/test/parallel/test-runner-run-coverage.mjs b/test/parallel/test-runner-run-coverage.mjs
@@ -106,7 +106,21 @@ describe('require(\'node:test\').run coverage settings', { concurrency: true },
       // eslint-disable-next-line no-unused-vars
       for await (const _ of stream);
     });
-
+    await it('should not include test files with complex paths', async () => {
+      const coverageFiles = [];
+      const stream = run({ files: [fixtures.path('no-folder', '..', 'test-runner', 'coverage.js')], coverage: true });
+      stream.on('test:fail', common.mustNotCall());
+      stream.on('test:pass', common.mustCall());
+      stream.on('test:coverage', msg => {
+        coverageFiles.push(...msg.summary.files.map(file => file.path));
+      });
+      // eslint-disable-next-line no-unused-vars
+      for await (const _ of stream);
+      assert.deepStrictEqual(coverageFiles.sort(), [
+        fixtures.path('test-runner', 'invalid-tap.js'),
+        fixtures.path('v8-coverage', 'throw.js')
+      ]);
+    });
     await it('should run with coverage and exclude by glob', async () => {
       const stream = run({ files, coverage: true, coverageExcludeGlobs: ['test/*/test-runner/invalid-tap.js'] });
       stream.on('test:fail', common.mustNotCall());
@@ -137,13 +151,13 @@ describe('require(\'node:test\').run coverage settings', { concurrency: true },
 
     await it('should run while including and excluding globs', async () => {
       const stream = run({
-        files: [...files, fixtures.path('test-runner/invalid-tap.js')],
+        files: files,
         coverage: true,
         coverageIncludeGlobs: ['test/fixtures/test-runner/*.js'],
         coverageExcludeGlobs: ['test/fixtures/test-runner/*-tap.js']
       });
       stream.on('test:fail', common.mustNotCall());
-      stream.on('test:pass', common.mustCall(2));
+      stream.on('test:pass', common.mustCall(1));
       stream.on('test:coverage', common.mustCall(({ summary: { files } }) => {
         const filesPaths = files.map(({ path }) => path);
         assert.strictEqual(filesPaths.every((path) => !path.includes(`test-runner${sep}invalid-tap.js`)), true);
@@ -153,11 +167,12 @@ describe('require(\'node:test\').run coverage settings', { concurrency: true },
       for await (const _ of stream);
     });
 
-    await it('should run with coverage and fail when below line threshold', async () => {
+    await it('should run with coverage and coverageIncludeGlobs and fail when below thresholds', async () => {
       const thresholdErrors = [];
       const originalExitCode = process.exitCode;
       assert.notStrictEqual(originalExitCode, 1);
-      const stream = run({ files, coverage: true, lineCoverage: 99, branchCoverage: 99, functionCoverage: 99 });
+      const stream = run({ files, coverageIncludeGlobs: '**', coverage: true,
+                           lineCoverage: 99, branchCoverage: 99, functionCoverage: 99 });
       stream.on('test:fail', common.mustNotCall());
       stream.on('test:pass', common.mustCall(1));
       stream.on('test:diagnostic', ({ message }) => {
@@ -172,6 +187,26 @@ describe('require(\'node:test\').run coverage settings', { concurrency: true },
       assert.strictEqual(process.exitCode, 1);
       process.exitCode = originalExitCode;
     });
+    await it('should run with coverage and fail when below thresholds', async () => {
+      const thresholdErrors = [];
+      const originalExitCode = process.exitCode;
+      assert.notStrictEqual(originalExitCode, 1);
+      const stream = run({ files, coverage: true,
+                           lineCoverage: 99, branchCoverage: 99, functionCoverage: 99 });
+      stream.on('test:fail', common.mustNotCall());
+      stream.on('test:pass', common.mustCall(1));
+      stream.on('test:diagnostic', ({ message }) => {
+        const match = message.match(/Error: \d{2}\.\d{2}% (line|branch|function) coverage does not meet threshold of 99%/);
+        if (match) {
+          thresholdErrors.push(match[1]);
+        }
+      });
+      // eslint-disable-next-line no-unused-vars
+      for await (const _ of stream);
+      assert.deepStrictEqual(thresholdErrors.sort(), ['branch', 'line']);
+      assert.strictEqual(process.exitCode, 1);
+      process.exitCode = originalExitCode;
+    });
   });
 });