test_runner: check test coverage threshold

[marco-ippolito]

Refs: #48739
With this PR I want to introduce a new flag --check-coverage (heavily inspired from c8).
Used in conjunction with the --experimental-test-coverage flag, enforce a specific test coverage threshold check (--lines, --branches, --functions).
Each of these flags accept a numerical value between 0 and 100, representing the percentage (e.g., 80 for 80% coverage).
If the coverage falls below the threshold, the test will exit with exitCode 1.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[targos]

There's some discussion about this on #48739

[meyfa]

I think this is already being worked on in #51182.

[marco-ippolito]

I think this is already being worked on in #51182.

I went for the c8 (CLI flags --lines, --branches, --functions) approach, which is different from the one in the pr and has not been approved yet.

[bnb]

@marco-ippolito would it be possible to add CLI/shell examples in the CLI doc? Reading the documentation, I see that the additions are connected, but I can't immediately visualize how they should be used together based off prose alone.

[marco-ippolito]

@marco-ippolito would it be possible to add CLI/shell examples in the CLI doc? Reading the documentation, I see that the additions are connected, but I can't immediately visualize how they should be used together based off prose alone.

Added a bunch of examples and expanded documentation, should be much clearer now

[mcollina]

lgtm

[benjamingr]

I am still not convinced we need/want this + as others have mentioned there is already another PR open for this prior at #51182

The optics of a collaborator opening a PR for the same feature (with a similar impl) as a non-collaborator and it getting approved quickly also isn't great.

[marco-ippolito]

@benjamingr I did not notice there was another PR opened before being mentioned in the PR, and surely I haven't opened it because I wanted it to approve it quickly.
If you want I can close the pull request but note this is a different implementation, inspired from https://github.com/bcoe/c8.
It is very common to check coverage, can you please elaborate on why this feature is not wanted?

[benjamingr]

I haven't opened it because I wanted it to approve it quickly.

To be 100% clear, I am absolutely confident you've acted in good faith and opened this PR to implement a feature you want in Node.js. I am grateful for the effort.

As for the feature - the problem is that this feature is very incomplete and doubtly useful without source map support in the test runner first - so we're probably giving people a footgun here?

As for the other PR - I would personally prefer to see the author of that to at least share credit if we land this one.

[marco-ippolito]

@benjamingr thanks for clarifying.
Tagging the author of the other PR @pulkit-30 for visibility.

I might have missed conversation about sourcemaps in the test runner, can you be more specific on why it is a footgun?
The feature is based on data from the coverage, if data is not accurate the coverage itself its useless.

If we have shipped coverage I dont see why not ship also checks, as user will need to install an external dependency for coverage (like C8) just for the checks.
Also the feature is experiemental.

[tniessen]

As mentioned in the other PR, I think it's fine for Node.js to produce coverage information in a standardized format instead that can be consumed by external tools (such as codecov or so).

In any case, such thresholds tend to be fragile metrics. For example, 100% coverage does not at all guarantee that there are no bugs, yet often requires a ton of effort to achieve.

Also, Node.js will need to provide stability guarantees for any of these metrics. (I assume that's already the case, but I want to mention it regardless.)

[tniessen]

I still think that, if folks keep adding more and more flags for the test runner, they should at least be scoped, i.e., the name should be somewhat specific (e.g., --test-coverage-line-threshold) or the option should only be parsed in the context of the test runner (similar to python's -m, for example). Otherwise, we are just bloating the CLI with ambiguously named options.

[marco-ippolito]

I still think that, if folks keep adding more and more flags for the test runner, they should at least be scoped, i.e., the name should be somewhat specific (e.g., --test-coverage-line-threshold) or the option should only be parsed in the context of the test runner (similar to python's -m, for example). Otherwise, we are just bloating the CLI with ambiguously named options.

I do agree that flags need to be scoped but also not too long as they become hard to read when lined. Can you elaborate on how the option should be parsed in test runner context?

[meyfa]

One idea may be to let --check-coverage take a string argument with the additional options, for example:

--check-coverage=lines=80,branches=100

# or
--check-coverage=lines:80,branches:100

# or support passing multiple times, one aspect each
--check-coverage lines=80 --check-coverage branches=100

I know this increases parsing complexity and may not be aligned with other CLI flags in Node.js (though I haven't checked them all). There is, however, precedence in other CLIs such as the Docker CLI:

$ docker run --env VAR1=value1 --env VAR2=value2 ubuntu

[tniessen]

I assume some projects actually want this for one reason or another, but is it something we want to promote?

[marco-ippolito]

I assume some projects actually want this for one reason or another, but is it something we want to promote?

I think that not having it means people using an external library for code coverage instead of the native one, rather than something built on top

[marco-ippolito]

100% Coverage does not mean no bug, and that applies regardless if the check is in place or not.
This is more a general misconception.
For reference Bun ship the threshold check while Deno does not.

[ljharb]

This feature should not be added until it has all 4 metrics that every other similar tool does - it’s missing “statements”.

[marco-ippolito]

Closing as general consensus it is to not ship this feature