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

Jump to bottom

Running tests with --test-coverage-branches (same for lines|functions) doesn't emit test:fail when threshold isn't met #54812

Closed
rozzilla opened this issue Sep 6, 2024 · 5 comments · Fixed by #54813
Closed

Running tests with --test-coverage-branches (same for lines|functions) doesn't emit test:fail when threshold isn't met #54812

rozzilla opened this issue Sep 6, 2024 · 5 comments · Fixed by #54813
Labels
coverage Issues and PRs related to native coverage support. 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

@rozzilla
Copy link

rozzilla commented Sep 6, 2024

Version

22.8.0

Platform

Darwin N4V4PGFGPT 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:16:46 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T8112 arm64

Subsystem

No response

What steps will reproduce the bug?

I'm running Node with the following options:

 --experimental-test-coverage --test-coverage-exclude='**/__tests__/**' --test-coverage-branches=95 --env-file=.env.test

and calling an executable that calls the run method as by doc.

If a test fails, a test:fail event is properly emitted.
If the coverage threshold is not met, I get a message from the test:diagnostic event like Error: 82.35% function coverage does not meet a threshold of 95%. (I get the same on the stdout). But no test:fail event is emitted, which makes it harder to detect it.

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

test:fail event should be emitted once coverage is not meet

What do you see instead?

no test:fail event emitted

Additional information

same thing apply when using --test-coverage-functions or --test-coverage-lines options
@avivkeller avivkeller self-assigned this Sep 6, 2024
@avivkeller avivkeller added coverage Issues and PRs related to native coverage support. test_runner Issues and PRs related to the test runner subsystem. repro-exists labels Sep 6, 2024
@avivkeller avivkeller removed their assignment Sep 6, 2024
@avivkeller
Copy link
Member

avivkeller commented Sep 6, 2024 •
edited
Loading

I'm able to reproduce. I'll have a look at this later today.

FWIW the process does exit with code 1, however all the tests do pass.

CC @nodejs/test_runner

@cjihrig
Copy link
Contributor

cjihrig commented Sep 6, 2024

A test:fail event should not be emitted for coverage issues. There is a test:coverage event that would be more appropriate.

@rozzilla
Copy link
Author

rozzilla commented Sep 6, 2024

A test:fail event should not be emitted for coverage issues. There is a test:coverage event that would be more appropriate.

Mmm, well, the CHANGELOG (not the docs, since they're not updated yet) says:

If the code coverage fails to meet the specified thresholds for any category, the process will exit with code 1.

Considering that, it feels more like a test:fail. Anyway, if there is a clear indication of coverage check failures somewhere, it'd be enough 👍🏼

@cjihrig
Copy link
Contributor

cjihrig commented Sep 6, 2024 •
edited
Loading

test:fail means a test failed. It's already possible to end up with a failing exit code independent of coverage. For example, if a test passes, but creates a setTimeout() or other async activity that generates an error. The test has already finished and reported itself as being successful. So the error gets surfaced through a diagnostic and the exit code is set to 1. That's basically what is happening in the code coverage case as well. I agree that it should be signaled in the coverage event though.

@avivkeller avivkeller mentioned this issue Sep 6, 2024
Merged
@avivkeller
Copy link
Member

See #54813

cjihrig added a commit to cjihrig/node that referenced this issue Sep 8, 2024
@cjihrig
test_runner: add 'test:summary' event
b5254f9 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- The test runner internals no longer change the process exit
  code. This may be important to run() users.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
@cjihrig cjihrig mentioned this issue Sep 8, 2024
Merged
@avivkeller avivkeller added feature request Issues that request new features to be added to Node.js. and removed repro-exists labels Sep 10, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Sep 10, 2024
@avivkeller avivkeller moved this from Awaiting Triage to In Progress in Node.js feature requests Sep 10, 2024
@avivkeller avivkeller moved this from In Progress to Done in Node.js feature requests Sep 13, 2024
cjihrig added a commit to cjihrig/node that referenced this issue Sep 14, 2024
@cjihrig
test_runner: add 'test:summary' event
dfcbb86 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
cjihrig added a commit to cjihrig/node that referenced this issue Sep 14, 2024
@cjihrig
test_runner: add 'test:summary' event
808d7ad 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
cjihrig added a commit to cjihrig/node that referenced this issue Sep 20, 2024
@cjihrig
test_runner: add 'test:summary' event
468d4e3 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
nodejs-github-bot pushed a commit that referenced this issue Sep 21, 2024
@cjihrig
test_runner: add 'test:summary' event
059e08b 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: #53867
Refs: #54812
PR-URL: #54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Oct 4, 2024
@cjihrig @targos
test_runner: add 'test:summary' event
b62f2f8 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: #53867
Refs: #54812
PR-URL: #54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@cjihrig @louwers
test_runner: add 'test:summary' event
0c2ab76 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
PR-URL: nodejs#54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
@cjihrig @tpoisseau
test_runner: add 'test:summary' event
c3001c2 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
PR-URL: nodejs#54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
coverage Issues and PRs related to native coverage support. feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test_runner: report coverage thresholds in test:coverage avivkeller/node
3 participants
@cjihrig @rozzilla @avivkeller