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: report error on missing sourcemap source #55037

Merged
merged 1 commit into from
Sep 23, 2024
Merged

test_runner: report error on missing sourcemap source #55037

merged 1 commit into from
Sep 23, 2024

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Sep 21, 2024 •
edited
Loading

Fixes #54756

ERR_SOURCE_MAP_MISSING_SOURCE is now thrown when a sourcemap contains a reference to a non-existent file.

@jaydenseric provided the test fixture, and @cjihrig provided the initial patch.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. 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
@RedYetiDev RedYetiDev force-pushed the fix-coverage-sourcemap-missing-source branch from 3723a8d to 4badc52 Compare September 21, 2024 00:32
@RedYetiDev RedYetiDev mentioned this pull request Sep 21, 2024
Open
@codecov Codecov
Copy link

codecov bot commented Sep 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (8b8fc53) to head (462fb41).
Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/coverage.js 62.50% 3 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55037   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files         652      652           
  Lines      183855   183921   +66     
  Branches    35856    35862    +6     
=======================================
+ Hits       162227   162287   +60     
- Misses      14909    14914    +5     
- Partials     6719     6720    +1
Files with missing lines Coverage Δ
lib/internal/errors.js 96.98% <100.00%> (ø)
lib/internal/test_runner/coverage.js 64.74% <62.50%> (-0.03%) ⬇️

... and 47 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.

The changes LGTM to me but I'd rather not make a bunch of unrelated changes in this PR.

@RedYetiDev
Copy link
Member Author

IMO they are semi-related, because I moved source-map-line-lengths to source-map/line-lengths so that I could add source-map/missing-sources, but I can undo it if you want.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 21, 2024

I can undo it if you want

Please do.

@RedYetiDev RedYetiDev force-pushed the fix-coverage-sourcemap-missing-source branch from 4badc52 to 31ed46d Compare September 21, 2024 18:15
cjihrig
cjihrig approved these changes 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.

LGTM thanks

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@RedYetiDev @jaydenseric @cjihrig
test_runner: report error on missing sourcemap source
462fb41 
Co-Authored-By: Jayden Seric <me@jaydenseric.com>
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
@RedYetiDev RedYetiDev force-pushed the fix-coverage-sourcemap-missing-source branch from 31ed46d to 462fb41 Compare September 21, 2024 18:32
@RedYetiDev RedYetiDev removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@RedYetiDev
Copy link
Member Author

@atlowChemi I force-pushed to include the co-authors, so you'll need to re-approve before starting a CI. (Sorry!)

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev RedYetiDev mentioned this pull request Sep 21, 2024
Closed
3 tasks
@RedYetiDev RedYetiDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 21, 2024
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 21, 2024 •
edited
Loading

I'm worried this might also catch #55054, so I'm gonna look into that I checked, it's not gonna interfere.

@nodejs-github-bot
Copy link
Collaborator

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

@atlowChemi atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2024
@RedYetiDev RedYetiDev mentioned this pull request Sep 22, 2024
Merged
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2024
@nodejs-github-bot nodejs-github-bot merged commit 4f88179 into nodejs:main Sep 23, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4f88179

targos pushed a commit that referenced this pull request Oct 4, 2024
@RedYetiDev @jaydenseric
@cjihrig @targos
test_runner: report error on missing sourcemap source
8818c6c 
Co-Authored-By: Jayden Seric <me@jaydenseric.com>
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #55037
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@RedYetiDev @jaydenseric
@cjihrig @louwers
test_runner: report error on missing sourcemap source
a3901bc 
Co-Authored-By: Jayden Seric <me@jaydenseric.com>
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#55037
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. coverage Issues and PRs related to native coverage support. errors Issues and PRs related to JavaScript errors originated in Node.js core. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bad error message in --experimental-test-coverage output when a source maps source module is missing
4 participants
@RedYetiDev @nodejs-github-bot @cjihrig @atlowChemi