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.

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

New CLI flag to exit with an error status if test coverage is incomplete #48739

Open
jaydenseric opened this issue Jul 11, 2023 · 7 comments · May be fixed by #51182
Open

New CLI flag to exit with an error status if test coverage is incomplete #48739

jaydenseric opened this issue Jul 11, 2023 · 7 comments · May be fixed by #51182
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@jaydenseric
Copy link
Contributor

jaydenseric commented Jul 11, 2023

What is the problem this feature will solve?

When using the Node.js test runner in combination with the --experimental-test-coverage flag, it would be very useful to be able to also flag Node.js to exit the process with an error status (i.e. 1) if the coverage was not 100% complete, to be able to enforce complete code coverage in CI.

Until this feature exists, I won't be able to migrate projects from using coverage-node:

https://github.com/jaydenseric/coverage-node/tree/v8.0.0#command-coverage-node

Personally I only have interest in enforcing 100% code coverage or not enforcing it at all, but I can see how some teams might want to enforce a minimum percent of code coverage so they can iteratively work towards 100%, raising the minimum over time, and notice in CI if any contributions go against that goal.

What is the feature you are proposing to solve the problem?

We currently run tests like this:

node --enable-source-maps --experimental-test-coverage --test-reporter=spec --test

So I propose another flag --minimum-coverage (bikeshed the name) that can accept a percent number for the minimum coverage percent required to avoid the process exiting with an error status 1. E.g:

node --enable-source-maps --experimental-test-coverage --minimum-coverage=100 --test-reporter=spec --test

What alternatives have you considered?

  1. Changing the --experimental-test-coverage from a boolean to accepting a percent number for the minimum coverage percent required to avoid the process exiting with an error status 1.

    node --enable-source-maps --experimental-test-coverage=100 --test-reporter=spec --test
    

    I actually quite like this because it reduces the number of flags, but I'm not sure it's ok to change an existing flag already in use? Perhaps that's not a concern since it's experimental and also if the value is optional and defaults to the current behaviour it won't break projects already using --experimental-test-coverage.

  2. Enforce 100% code coverage by default, and use an environment variable to opt out of the enforcement. This is how coverage-node currently works:

    https://github.com/jaydenseric/coverage-node/tree/v8.0.0#command-coverage-node

@jaydenseric jaydenseric added the feature request Issues that request new features to be added to Node.js. label Jul 11, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Jul 11, 2023

I've been wondering when someone would open this issue 😄 .

Option 1 (changing the existing flag) would be preferable to limit the number of flags. It's fine to change its behavior because it's still experimental. We would need to support:

  • Flag not set. Do not collect coverage.
  • Flag set with no explicit value. Use a default of either 0% or 100%.
  • Flag set with an explicit value.

If coverage does not meet the threshold, we would need to set the process exit code, and ideally include some indication in the coverage output. We should also include the threshold information in the reporter coverage event.

We also need to define how the threshold is calculated. The simplest way would be to look only at the total lines vs. lines covered. However, we could also include branch taken / not taken information in the calculation.

@MoLow
Copy link
Member

MoLow commented Jul 12, 2023

I am not sure I would use this before there is sourcemaps support, but wont block it

@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Jul 13, 2023
@jasonwilliams
Copy link

I am not sure I would use this before there is sourcemaps support, but wont block it

Is there an issue open for this?

@MoLow
Copy link
Member

MoLow commented Aug 19, 2023

nope

@theScottyJam
Copy link

I like to configure my branch coverage differently from my line coverage - more specifically, I use a strict setting for the line-coverage and a looser setting for the branch coverage (I just don't find it's worth it to test every single optional parameter and what-not in the project(s) I'm working in). It would be nice if whatever config option is chosen is capable of supporting the configuration of both values independently.

I personally don't use warning thresholds, but I would imagine a good number of people would care about being able to configure those as well

And I know some tools also support configurable thresholds for the percentage of functions you have under test, though I don't know how much people care about that in practice.

All together, if this were to be a single CLI argument, I guess we could invent some sort of short-hand syntax for it. Something like:

--experimental-test-coverage='line-error:80% branch-error:50% line-warn:85% branch-warn:50%'

If warning thresholds aren't supplied, we could default to some fixed percentage above the error or something.

Pro: concise and easier for humans to read and write.
Con: Harder for programs to use this. Programmatic usage would have an easier time if each value was supplied as a separate flag.

I was also seeing somewhere in another ticket (can't remember where) some discussion about maybe adding a config file so these sorts of customizations don't get too crazy. That could be another option.

Copy link
Contributor

github-actions bot commented Jul 3, 2024

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Jul 3, 2024
@MoLow MoLow removed the stale label Jul 3, 2024
@MoLow
Copy link
Member

MoLow commented Jul 3, 2024

I think this is now relevant again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
5 participants