test_runner: added coverage threshold support for tests by pulkit-30 · Pull Request #51182 · nodejs/node · GitHub

Added support for coverage threshold comparison.

pass coverage threshold value via cli.
if threshold didn;t match then log it.

example:

code:

import { test } from 'node:test';
import assert from 'node:assert';

test('running test', {}, () => {
    assert.strictEqual(1, 1);
});

output:

cmd: ./node --experimental-test-coverage --experimental-minimal-test-coverage 80 index.mjs

✔ running test (0.998708ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 8.136042
ℹ start of coverage report
ℹ ---------------------------------------------------------------
ℹ file           | line % | branch % | funcs % | uncovered lines
ℹ ---------------------------------------------------------------
ℹ index.test.mjs |  77.78 |    66.67 |  100.00 | 8-9
ℹ ---------------------------------------------------------------
ℹ all files      |  77.78 |    66.67 |  100.00 |
ℹ ---------------------------------------------------------------
ℹ minimum coverage failed for line. expected: 80% | actual: 77.78%
ℹ minimum coverage failed for branch. expected: 80% | actual: 66.67%
ℹ end of coverage report

code:

import { spec } from 'node:test/reporters';
import { run } from 'node:test';
import process from 'node:process';
import path from 'node:path';

run({
  files: [path.resolve('./index.test.mjs')],
  coverage: true,
  minimumCoverage: {
    branch: 80,
    function: 80,
    line: 80,
  },
})
  .compose(spec)
  .pipe(process.stdout);

output:

✔ running test (1.034708ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 90.49975
ℹ start of coverage report
ℹ ---------------------------------------------------------------
ℹ file           | line % | branch % | funcs % | uncovered lines
ℹ ---------------------------------------------------------------
ℹ index.test.mjs |  77.78 |    66.67 |  100.00 | 8-9
ℹ ---------------------------------------------------------------
ℹ all files      |  77.78 |    66.67 |  100.00 |
ℹ ---------------------------------------------------------------
ℹ minimum coverage failed for line. expected: 80% | actual: 77.78%
ℹ minimum coverage failed for branch. expected: 80% | actual: 66.67%
ℹ end of coverage report

Review requested:

@nodejs/test_runner

I think this should also set the exit code to a nonzero value such that CI systems will mark the job as failed.

I agree on the exit code; also, coverage always has 4 metrics, lines, functions, branches, and statements.

I think this should also set the exit code to a nonzero value such that CI systems will mark the job as failed.
I agree on the exit code;

working on it 👍🏼

It’s still missing “statements”.

It’s still missing “statements”.

I think we don't collect statement coverage summary.
https://github.com/nodejs/node/blob/main/lib/internal/test_runner/coverage.js#L241-L263

@MoLow please confirm about statement summary here and how can we support that.

PTAL @nodejs/test_runner

Overall LGTM, should add documentation though

Overall LGTM, should add documentation though

Thanks for review 🙌🏼 . will add documentation.

It’s still missing “statements”.

I think we don't collect statement coverage summary. https://github.com/nodejs/node/blob/main/lib/internal/test_runner/coverage.js#L241-L263

please confirm about statement summary here and how can we support that.

@atlowChemi Any idea about this?

There's a reason we don't use such a threshold in this repository -- it tends to be a fragile indicator.

On a side note, the first commit message does not adhere to our requirements and must be amended.

The documentation doesn't say anything about what coverage metric is meant here.

Indeed; typically in code coverage there are 4 metrics (statements remains missing), and all 4 are independently able to be set to a threshold.

Really good work, I'm not convinced we need/want the feature but the code and everyting else looks solid <3

Really good work, I'm not convinced we need/want the feature but the code and everyting else looks solid <3

Thanks @benjamingr,
I see this feature request. I don't want to force the inclusion of this feature, but I'm curious if there's an alternative method to ensure 100% code coverage.

Additionally, it might be beneficial to consider adding a coverage option within the test_runner/run() function. WDYT @nodejs/test_runner

I'm curious if there's an alternative method to ensure 100% code coverage.

FWIW, Node.js could always emit coverage data in some standardized format that could then be consumed by a different tool (e.g., codecov or so).

Also, "100% code coverage" is a somewhat odd metric. It by no means guarantees the absence of bugs yet is almost impossible to achieve in many cases.

FWIW, Node.js could always emit coverage data in some standardized format that could then be consumed by a different tool (e.g., codecov or so).

node already does that https://nodejs.org/api/test.html#event-testcoverage

This appears to have stalled out and now has conflicts. I'm going to close this for now.

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Dec 16, 2023

pulkit-30 force-pushed the fix-48739 branch from 7aeb2bd to 17d653c Compare December 17, 2023 07:34

pulkit-30 marked this pull request as ready for review December 17, 2023 07:34

ljharb requested changes Dec 17, 2023

View reviewed changes

Comment thread lib/internal/test_runner/harness.js Outdated

pulkit-30 force-pushed the fix-48739 branch 2 times, most recently from bfcfe72 to 52100c7 Compare December 19, 2023 13:07

pulkit-30 commented Dec 19, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

pulkit-30 force-pushed the fix-48739 branch from 52100c7 to bb211c1 Compare December 20, 2023 10:41

rluvaton reviewed Dec 20, 2023

View reviewed changes

Comment thread test/parallel/test-runner-run.mjs Outdated

pulkit-30 force-pushed the fix-48739 branch from b2e1fe4 to a928527 Compare December 25, 2023 13:55

atlowChemi reviewed Dec 26, 2023

View reviewed changes

Comment thread src/node_options.cc Outdated

tniessen reviewed Dec 27, 2023

View reviewed changes

test_runner: add initial support for coverage threshold

379c080

pulkit-30 force-pushed the fix-48739 branch from e0ecb36 to 379c080 Compare January 1, 2024 11:11

pulkit-30 requested a review from atlowChemi January 1, 2024 14:56

benjamingr reviewed Jan 1, 2024

View reviewed changes

meyfa mentioned this pull request Jan 4, 2024

test_runner: check test coverage threshold #51370

Closed

cjihrig closed this Aug 13, 2024

tniessen mentioned this pull request Aug 19, 2024

test_runner: add support for coverage thresholds #54429

Merged

test_runner: added coverage threshold support for tests by pulkit-30 · Pull Request #51182 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: added coverage threshold support for tests - #51182

test_runner: added coverage threshold support for tests#51182pulkit-30 wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom pulkit-30:fix-48739pulkit-30/node:fix-48739Copy head branch name to clipboard

Conversation

pulkit-30 commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 16, 2023

Uh oh!

meyfa commented Dec 17, 2023

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pulkit-30 commented Dec 18, 2023

Uh oh!

Uh oh!

ljharb commented Dec 19, 2023

Uh oh!

pulkit-30 commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pulkit-30 commented Dec 25, 2023

Uh oh!

atlowChemi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pulkit-30 commented Dec 26, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tniessen Dec 27, 2023

Choose a reason for hiding this comment

Uh oh!

ljharb Dec 27, 2023

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

pulkit-30 commented Jan 2, 2024

Uh oh!

tniessen commented Jan 3, 2024

Uh oh!

MoLow commented Jan 4, 2024

Uh oh!

cjihrig commented Aug 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

test_runner: added coverage threshold support for tests#51182
pulkit-30 wants to merge 1 commit into
nodejs:mainfrom
pulkit-30:fix-48739

pulkit-30 commented Dec 16, 2023 •
edited
Loading

pulkit-30 commented Dec 19, 2023 •
edited
Loading