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

test_runner: require --enable-source-maps for source map coverage #55039

Closed
wants to merge 2 commits into from
Closed

test_runner: require --enable-source-maps for source map coverage #55039

wants to merge 2 commits into from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Sep 21, 2024 •
edited
Loading

Ref: #54753 (comment) (CC @MoLow @cjihrig)
Ref: #55106

Coverage is currently an experimental feature, so this is non-breaking, right?

@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 21, 2024
@RedYetiDev RedYetiDev added coverage Issues and PRs related to native coverage support. source maps Issues and PRs related to source map support. labels Sep 21, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.37%. Comparing base (2cec716) to head (2b86de9).
Report is 20 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55039      +/-   ##
==========================================
+ Coverage   88.23%   88.37%   +0.13%     
==========================================
  Files         652      652              
  Lines      183911   184618     +707     
  Branches    35858    36636     +778     
==========================================
+ Hits       162271   163148     +877     
+ Misses      14916    14776     -140     
+ Partials     6724     6694      -30
Files with missing lines Coverage Δ
lib/internal/test_runner/coverage.js 70.56% <100.00%> (+5.78%) ⬆️
lib/internal/test_runner/test.js 96.94% <100.00%> (+<0.01%) ⬆️

... and 44 files with indirect coverage changes

cjihrig
cjihrig reviewed Sep 21, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Can you add a test case.

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved
cjihrig
cjihrig reviewed Sep 22, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

It looks like there are relevant CI failures.

lib/internal/test_runner/coverage.js Show resolved Hide resolved
@RedYetiDev RedYetiDev added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Sep 22, 2024
RedYetiDev
RedYetiDev commented Sep 22, 2024
View reviewed changes
lib/internal/test_runner/test.js
@@ -552,7 +552,8 @@ class Test extends AsyncResource {
file: loc[2],
};

if (this.config.sourceMaps === true) {
// TODO(RedYetiDev): This should be supported with code coverage
if (this.config.sourceMaps && !this.config.coverage) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Review Note When Code Coverage is enabled, the source map is loaded in the coverage constructor, and this statement messes up the lines. Not quite sure why, but skipping it has no negative impact.

mcollina
mcollina approved these changes Sep 22, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62676/

cjihrig
cjihrig requested changes Sep 22, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

The changes here are not correct and I am not comfortable landing this with comments like https://github.com/nodejs/node/pull/55039/files#r1770627388.

With these changes, this test is now broken if you run with --experimental-test-coverage. The failing test's location should be test/fixtures/test-runner/output/source_mapped_locations.ts:5:1, but these changes cause it to be test/fixtures/test-runner/output/source_mapped_locations.mjs:4:1 if coverage is enabled.

./node --enable-source-maps test/fixtures/test-runner/output/source_mapped_locations.mjs

@RedYetiDev RedYetiDev added wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 22, 2024
@RedYetiDev RedYetiDev closed this Sep 22, 2024
@RedYetiDev
Copy link
Member Author

As at @cjihrig mentioned above, there are a lot of kinks to work out with this change, so I'm going to close this PR while I figure out the best approach to this change.

Thank you for your reviews

@RedYetiDev RedYetiDev reopened this Sep 24, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 24, 2024 •
edited
Loading

After looking into it, the correct behavior is NOT that of --enable-source-maps See #55106.

@RedYetiDev RedYetiDev closed this Sep 24, 2024
@RedYetiDev
Copy link
Member Author

(didn't mean to re-open, just to comment)

@RedYetiDev RedYetiDev mentioned this pull request Sep 28, 2024
Closed
This was referenced Oct 1, 2024
Closed
Merged
@RedYetiDev RedYetiDev mentioned this pull request Oct 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig requested changes

@mcollina mcollina mcollina approved these changes

@MoLow MoLow Awaiting requested review from MoLow

Assignees
No one assigned
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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.
4 participants
@RedYetiDev @nodejs-github-bot @mcollina @cjihrig