Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: exclude test files from coverage by default #56060

Merged
merged 5 commits into from
Dec 17, 2024
Merged

test_runner: exclude test files from coverage by default #56060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f5885e
test_runner: exclude test files from coverage by default
pmarchini Dec 8, 2024
4e2bdf6
test: add default coverage default exclusions test
pmarchini Dec 11, 2024
1e5a29c
lib: move glob from path to internal/fs/glob
pmarchini Dec 12, 2024
2692e5f
test: skip test if no inspector
pmarchini Dec 13, 2024
70be970
lib: rename glob to matchGlobPattern
pmarchini Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2291,6 +2291,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
Expand Down
7 changes: 5 additions & 2 deletions doc/api/test.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
pmarchini marked this conversation as resolved.
Show resolved Hide resolved
the `'test:coverage'` event.

Coverage can be disabled on a series of lines using the following
Expand Down Expand Up @@ -3594,6 +3596,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
Expand Down
36 changes: 30 additions & 6 deletions lib/internal/test_runner/coverage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ const {
readFileSync,
rmSync,
} = require('fs');
const { setupCoverageHooks } = require('internal/util');
const { setupCoverageHooks, isWindows, isMacOS } = 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 {
Expand All @@ -42,6 +42,25 @@ const kLineEndingRegex = /\r?\n$/u;
const kLineSplitRegex = /(?<=\r?\n)/u;
const kStatusRegex = /\/\* node:coverage (?<status>enable|disable) \*\//;

let minimatch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all of this to avoid the experimental warning? If so, maybe we should create a flag in the internal glob() function to skip the warning instead of duplicating the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it is 😞
I tried without luck to add a flag or something to prevent the warning in case of internal usage but I didn't find a decent solution.
Do you know if we have something similar around in the codebase?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like matchesGlob() calls an internal glob() function. I would move glob() to internal/ and add a flag to that function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks, I'll take a look ASAP 🚀

Copy link
Member Author

@pmarchini pmarchini Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just realised that in another part of the runner, we're using a different approach:

#47653

This includes a lean implementation of https://github.com/isaacs/node-glob on top of minimatch (added at #47499).
This is marked as [semver-major]( semver-major PRs that contain breaking changes and should be released in the next major version. ) since a couple of edge cases will produce a different list of tests, such as file names containing *.

I'm just going to replace matchesGlob with lib/internal/fs/glob.js, as we are doing in the test files glob.

Edit: I think it's better to go with

It looks like matchesGlob() calls an internal glob() function. I would move glob() to internal/ and add a flag to that function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved glob to internal/fs/glob but I'm not entirely convinced about the naming:

module.exports = {
  __proto__: null,
  Glob,
  glob,
};

function lazyMinimatch() {
minimatch ??= require('internal/deps/minimatch/index');
return minimatch;
}

function glob(path, pattern, windows) {
return lazyMinimatch().minimatch(path, pattern, {
__proto__: null,
nocase: isMacOS || isWindows,
windowsPathsNoEscape: true,
nonegate: true,
nocomment: true,
optimizationLevel: 2,
platform: windows ? 'win32' : 'posix',
nocaseMagicOnly: true,
});
}

class CoverageLine {
constructor(line, startOffset, src, length = src?.length) {
const newlineLength = src == null ? 0 :
Expand Down Expand Up @@ -464,19 +483,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 (
glob(relativePath, excludeGlobs[i]) ||
glob(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 (
glob(relativePath, includeGlobs[i]) ||
glob(absolutePath, includeGlobs[i])
) return false;
}
return true;
}
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/test_runner/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
11 changes: 10 additions & 1 deletion test/fixtures/test-runner/output/lcov_reporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
);
6 changes: 5 additions & 1 deletion test/parallel/test-runner-coverage-source-map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-runner-coverage-thresholds.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ for (const coverage of coverages) {
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
`${coverage.flag}=25`,
'--test-reporter', 'tap',
fixture,
Expand All @@ -77,6 +78,7 @@ for (const coverage of coverages) {
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
`${coverage.flag}=25`,
'--test-reporter', reporter,
fixture,
Expand All @@ -92,6 +94,7 @@ for (const coverage of coverages) {
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
`${coverage.flag}=99`,
'--test-reporter', 'tap',
fixture,
Expand All @@ -108,6 +111,7 @@ for (const coverage of coverages) {
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
`${coverage.flag}=99`,
'--test-reporter', reporter,
fixture,
Expand All @@ -123,6 +127,7 @@ for (const coverage of coverages) {
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
`${coverage.flag}=101`,
fixture,
]);
Expand All @@ -136,6 +141,7 @@ for (const coverage of coverages) {
const result = spawnSync(process.execPath, [
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
`${coverage.flag}=-1`,
fixture,
]);
Expand Down
89 changes: 75 additions & 14 deletions test/parallel/test-runner-coverage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@ test('test coverage report', async (t) => {
}

const fixture = fixtures.path('test-runner', 'coverage.js');
const args = ['--experimental-test-coverage', fixture];
const args = [
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
fixture,
];
const result = spawnSync(process.execPath, args);

assert(!result.stdout.toString().includes('# start of coverage report'));
Expand All @@ -97,7 +101,13 @@ 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',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'tap',
fixture,
];
const options = { env: { ...process.env, NODE_V8_COVERAGE: tmpdir.path } };
const result = spawnSync(process.execPath, args, options);
const report = getTapCoverageFixtureReport();
Expand All @@ -109,7 +119,13 @@ 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',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'tap',
fixture,
];
const result = spawnSync(process.execPath, args);
const report = getTapCoverageFixtureReport();

Expand All @@ -123,7 +139,12 @@ 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',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'spec',
fixture];
const options = { env: { ...process.env, NODE_V8_COVERAGE: tmpdir.path } };
const result = spawnSync(process.execPath, args, options);
const report = getSpecCoverageFixtureReport();
Expand All @@ -136,7 +157,12 @@ 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',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'spec',
fixture];
const result = spawnSync(process.execPath, args);
const report = getSpecCoverageFixtureReport();

Expand All @@ -150,7 +176,12 @@ 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',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'tap',
fixture,
];
const result = spawnSync(process.execPath, args);
const report = getTapCoverageFixtureReport();
Expand Down Expand Up @@ -183,7 +214,11 @@ 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',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'tap',
];
const result = spawnSync(process.execPath, args, {
env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
Expand Down Expand Up @@ -221,7 +256,11 @@ test.skip('coverage works with isolation=none', skipIfNoInspector, () => {

const fixture = fixtures.path('v8-coverage', 'combined_coverage');
const args = [
'--test', '--experimental-test-coverage', '--test-reporter', 'tap', '--experimental-test-isolation=none',
'--test',
'--experimental-test-coverage',
'--test-reporter',
'tap',
'--experimental-test-isolation=none',
];
const result = spawnSync(process.execPath, args, {
env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
Expand All @@ -236,9 +275,14 @@ 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',
fixtures.fileURL('test-runner/custom_reporters/coverage.mjs'),
fixture]);
[
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
'--test-reporter',
fixtures.fileURL('test-runner/custom_reporters/coverage.mjs'),
fixture,
]);
assert.strictEqual(child.stderr.toString(), '');
const stdout = child.stdout.toString();
const coverage = JSON.parse(stdout);
Expand Down Expand Up @@ -310,7 +354,14 @@ test('coverage with ESM hook - source irrelevant', skipIfNoInspector, () => {

const fixture = fixtures.path('test-runner', 'coverage-loader');
const args = [
'--import', './register-hooks.js', '--test', '--experimental-test-coverage', '--test-reporter', 'tap', 'virtual.js',
'--import',
'./register-hooks.js',
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'tap',
'virtual.js',
];
const result = spawnSync(process.execPath, args, { cwd: fixture });

Expand Down Expand Up @@ -341,7 +392,10 @@ test('coverage with ESM hook - source transpiled', skipIfNoInspector, () => {

const fixture = fixtures.path('test-runner', 'coverage-loader');
const args = [
'--import', './register-hooks.js', '--test', '--experimental-test-coverage',
'--import', './register-hooks.js',
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
'--test-reporter', 'tap', 'sum.test.ts',
];
const result = spawnSync(process.execPath, args, { cwd: fixture });
Expand All @@ -356,6 +410,7 @@ test('coverage with excluded files', skipIfNoInspector, () => {
const args = [
'--experimental-test-coverage', '--test-reporter', 'tap',
'--test-coverage-exclude=test/*/test-runner/invalid-tap.js',
'--test-coverage-exclude=!test/**',
fixture];
const result = spawnSync(process.execPath, args);
const report = [
Expand Down Expand Up @@ -391,6 +446,7 @@ test('coverage with included files', skipIfNoInspector, () => {
'--experimental-test-coverage', '--test-reporter', 'tap',
'--test-coverage-include=test/fixtures/test-runner/coverage.js',
'--test-coverage-include=test/fixtures/v8-coverage/throw.js',
'--test-coverage-exclude=!test/**',
fixture,
];
const result = spawnSync(process.execPath, args);
Expand Down Expand Up @@ -478,7 +534,12 @@ test('correctly prints the coverage report of files contained in parent director
}
const fixture = fixtures.path('test-runner', 'coverage.js');
const args = [
'--test', '--experimental-test-coverage', '--test-reporter', 'tap', fixture,
'--test',
'--experimental-test-coverage',
'--test-coverage-exclude=!test/**',
'--test-reporter',
'tap',
fixture,
];
const result = spawnSync(process.execPath, args, {
env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path },
Expand Down
Loading
Loading