FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: report error on missing sourcemap source by avivkeller · Pull Request #55037 · nodejs/node · GitHub

/ node Public

test_runner: report error on missing sourcemap source - #55037

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
avivkeller:fix-coverage-sourcemap-missing-source
Sep 23, 2024
ConversationCommits (1)ChecksFiles changed
Merged

test_runner: report error on missing sourcemap source#55037
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
avivkeller:fix-coverage-sourcemap-missing-source

Conversation

avivkeller commented Sep 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Sep 21, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

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
avivkeller commented Sep 21, 2024
View reviewed changes
Comment thread test/parallel/test-runner-coverage.js Outdated
avivkeller added coverage Issues and PRs related to native coverage support. source maps Issues and PRs related to source map support. labels Sep 21, 2024
avivkeller mentioned this pull request Sep 21, 2024
Closed

codecov Bot commented Sep 21, 2024 •
edited
Loading

Copy link
Copy Markdown

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 445 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 reviewed Sep 21, 2024
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

avivkeller commented Sep 21, 2024

Copy link
Copy Markdown
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 commented Sep 21, 2024

Copy link
Copy Markdown
Contributor

I can undo it if you want

Please do.

avivkeller requested a review from cjihrig September 21, 2024 18:15
cjihrig approved these changes Sep 21, 2024
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM thanks

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
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>
avivkeller removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024

avivkeller commented Sep 21, 2024

Copy link
Copy Markdown
Member Author

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

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
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 commented Sep 21, 2024

Copy link
Copy Markdown
Collaborator

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

avivkeller mentioned this pull request Sep 21, 2024
Closed
3 tasks
avivkeller added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 21, 2024

avivkeller commented Sep 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Sep 22, 2024

Copy link
Copy Markdown
Collaborator

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

atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2024
avivkeller mentioned this pull request Sep 22, 2024
Merged
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 merged commit 4f88179 into nodejs:main Sep 23, 2024

nodejs-github-bot commented Sep 23, 2024

Copy link
Copy Markdown
Collaborator

Landed in 4f88179

targos pushed a commit that referenced this pull request Oct 4, 2024
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 mentioned this pull request Oct 9, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
atlowChemi atlowChemi approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

5 participants

Back | FazBrowse Home | New Git URL