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

test_runner: consider --enable-source-maps option in coverage report #55146

Closed

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Sep 28, 2024

Ref: #54753

@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_runner Issues and PRs related to the test runner subsystem. labels Sep 28, 2024
@geeksilva97 geeksilva97 marked this pull request as draft September 28, 2024 03:10
@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 28, 2024

Just FYI there's some logic issues that need to resolved for this (see #55106 and #55039)

@RedYetiDev RedYetiDev added wip Issues and PRs that are still a work in progress. coverage Issues and PRs related to native coverage support. source maps Issues and PRs related to source map support. labels Sep 28, 2024
const { result } = coverage;
const sourceMapCache = coverage['source-map-cache'];
if (!sourceMapCache) {
if (!sourceMapEnabled || !sourceMapCache) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use getOptionValue. Instead, check whether the test runners sourceMaps is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RedYetiDev , do you have any clue on how I could do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous implementation for more information

@RedYetiDev
Copy link
Member

This was tried in #55039, however it couldn't land until a regression was fixed.

IMHO this PR will have the same result. Maybe looking into-and fixing?-the regression will help this move along?

@geeksilva97
Copy link
Contributor Author

This was tried in #55039, however it couldn't land until a regression was fixed.

IMHO this PR will have the same result. Maybe looking into-and fixing?-the regression will help this move along?

Thanks for letting me know. Let me close this then and look into this regression stuff.

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (17fd327) to head (ec3a30a).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/coverage.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55146      +/-   ##
==========================================
- Coverage   88.25%   88.24%   -0.01%     
==========================================
  Files         651      651              
  Lines      183915   183900      -15     
  Branches    35867    35857      -10     
==========================================
- Hits       162307   162276      -31     
- Misses      14895    14910      +15     
- Partials     6713     6714       +1     
Files with missing lines Coverage Δ
lib/internal/test_runner/coverage.js 64.70% <66.66%> (-0.05%) ⬇️

... and 47 files with indirect coverage changes

@geeksilva97
Copy link
Contributor Author

@RedYetiDev I had started some implementation related to non-mapped lines in that same issue.

Following @jaydenseric's suggestion it worked.

ℹ -----------------------------------------------------------
ℹ file       | line % | branch % | funcs % | uncovered lines
ℹ -----------------------------------------------------------
ℹ a.mts      | 100.00 |   100.00 |  100.00 |
ℹ test.mjs   | 100.00 |   100.00 |  100.00 |
ℹ -----------------------------------------------------------
ℹ all files  | 100.00 |   100.00 |  100.00 |
ℹ -----------------------------------------------------------

Is it worth opening a PR or does it also depend on regression?

@geeksilva97 geeksilva97 deleted the consider-source-map-flag branch September 28, 2024 05:26
@RedYetiDev
Copy link
Member

Feel free to open a PR. Only --enable-source-maps has the regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage Issues and PRs related to native coverage support. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support. test_runner Issues and PRs related to the test runner subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants