Skip to content

Conversation

mete0rfish
Copy link
Contributor

This PR adds a comprehensive test suite for the node:coverage control comments feature within the test runner.

I noticed there wasn't a specific test that checked all coverage control comments (ignore next, disable/enable).

Also I have a couple of questions to ensure I'm following the project's guidelines:

  1. I've used the term control-comments to describe the /* node:coverage */ comments. Is this the appropriate convention?
  2. For this test, I created a new fixture file named coverage-control-comments.js. I want to ensure that adding the new fixture is right choice.

I appreciate your feedback😀

refs: https://nodejs.org/api/test.html#collecting-code-coverage

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 9, 2025
Copy link

codecov bot commented Aug 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.87%. Comparing base (60a58f6) to head (6fdcdc0).
⚠️ Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59418      +/-   ##
==========================================
- Coverage   89.91%   89.87%   -0.05%     
==========================================
  Files         655      656       +1     
  Lines      192828   192962     +134     
  Branches    37805    37846      +41     
==========================================
+ Hits       173385   173419      +34     
- Misses      12029    12100      +71     
- Partials     7414     7443      +29     

see 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@daeyeon
Copy link
Member

daeyeon commented Aug 13, 2025

https://github.com/nodejs/node/actions/runs/16849137704/job/47732801343?pr=59418#step:6:13

Please follow commit message guideline #3 and wrap all lines at 72 columns (except for long URLs) in the first commit message, then update the commit.

@mete0rfish mete0rfish force-pushed the test-coverage branch 5 times, most recently from 79cbd31 to d05e777 Compare August 13, 2025 12:31
This commit adds a TC to verify the node:coverage control comments.
@mete0rfish
Copy link
Contributor Author

nodejs/node/actions/runs/16849137704/job/47732801343?pr=59418#step:6:13

Please follow commit message guideline #3 and wrap all lines at 72 columns (except for long URLs) in the first commit message, then update the commit.

I read the comment and changed the commit message!

@atlowChemi
Copy link
Member

@mete0rfish Thanks for the contribution!
The commit & PR should be prefixed with the test subsystem, as the changes are to the tests, not to the test runner itself

@mete0rfish mete0rfish changed the title test_runner: add test for coverage control comments test: add test for coverage control comments Aug 16, 2025
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 18, 2025
Copy link
Contributor

Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/17046636060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants