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

pulkit-30 · 2023-12-16T14:54:13Z

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

nodejs-github-bot · 2023-12-16T14:54:18Z

Review requested:

@nodejs/test_runner

meyfa · 2023-12-17T09:51:31Z

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

ljharb

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

lib/internal/test_runner/harness.js

pulkit-30 · 2023-12-18T08:05:05Z

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 👍🏼

lib/internal/test_runner/test.js

ljharb · 2023-12-19T15:38:54Z

It’s still missing “statements”.

pulkit-30 · 2023-12-19T17:42:56Z

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.

test/parallel/test-runner-run.mjs

pulkit-30 · 2023-12-25T17:58:08Z

PTAL @nodejs/test_runner

atlowChemi

Overall LGTM, should add documentation though

src/node_options.cc

pulkit-30 · 2023-12-26T09:33:45Z

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?

tniessen

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.

doc/api/cli.md

tniessen · 2023-12-27T11:35:10Z

doc/api/cli.md

+To Specify the minimum percentage threshold value for code coverage.
+Used with `--experimental-test-coverage` flag. If the minimum threshold
+value is not met, the test will exit with status code 1.


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.

benjamingr

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

pulkit-30 · 2024-01-02T16:44:39Z

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

tniessen · 2024-01-03T13:42:16Z

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.

MoLow · 2024-01-04T15:03:55Z

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

cjihrig · 2024-08-13T18:50:17Z

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

lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved

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

lib/internal/test_runner/test.js Outdated Show resolved Hide resolved

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

test/parallel/test-runner-run.mjs Show resolved Hide resolved

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

src/node_options.cc Outdated Show resolved Hide resolved

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

Uh oh!

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

pulkit-30 commented Dec 16, 2023 •

edited

Loading

pulkit-30 commented Dec 19, 2023 •

edited

Loading