Skip to content

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

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
Closed
geeksilva97 wants to merge 1 commit into from
Closed

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

geeksilva97 wants to merge 1 commit into from

Conversation

@geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Sep 28, 2024 •
edited
Loading

Ref: #54753

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 28, 2024

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
@avivkeller
Copy link
Member

avivkeller commented Sep 28, 2024 •
edited
Loading

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

@avivkeller avivkeller 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
avivkeller
avivkeller reviewed Sep 28, 2024
View reviewed changes
lib/internal/test_runner/coverage.js
const { result } = coverage;
const sourceMapCache = coverage['source-map-cache'];
if (!sourceMapCache) {
if (!sourceMapEnabled || !sourceMapCache) {
Copy link
Member

@avivkeller avivkeller Sep 28, 2024

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

@geeksilva97 geeksilva97 Oct 1, 2024

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

@avivkeller avivkeller Oct 1, 2024

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

@avivkeller
Copy link
Member

avivkeller commented Sep 28, 2024

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

geeksilva97 commented Sep 28, 2024

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.

@codecov
Copy link

codecov bot commented Sep 28, 2024 •
edited
Loading

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

geeksilva97 commented Sep 28, 2024

@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
@avivkeller
Copy link
Member

avivkeller commented Sep 28, 2024

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

Reviewers

@avivkeller avivkeller avivkeller left review comments

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@geeksilva97 @nodejs-github-bot @avivkeller