test_runner: throw on invalid source map by avivkeller · Pull Request #55055 · nodejs/node · GitHub

This PR changes the code coverage test runner to throw ERR_SOURCE_MAP_CORRUPT when a sourcemap is not a valid JSON file, or does not exist.

Review requested:

@nodejs/test_runner

Codecov Report

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

Project coverage is 88.41%. Comparing base (8de2695) to head (87af23b).
Report is 290 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/coverage.js	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55055      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         652      652              
  Lines      186576   186579       +3     
  Branches    36049    36049              
==========================================
- Hits       164960   164955       -5     
- Misses      14889    14900      +11     
+ Partials     6727     6724       -3

Files with missing lines	Coverage Δ
lib/internal/errors.js	96.98% <100.00%> (+<0.01%)	⬆️
lib/internal/test_runner/coverage.js	64.65% <50.00%> (-0.05%)	⬇️

... and 35 files with indirect coverage changes

The first commit is #55037, which will land shortly. This PR adds ERR_SOURCE_MAP_CANNOT_PARSE for when the source map does not exist / is not valid JSON

lgtm

Please stop changing unrelated things in PRs. It makes PRs more difficult to review and is more likely to introduce issues. If you feel strongly that the source map fixtures should be renamed, please do that in a separate PR.

You shouldn't need the awaits on the tests.

I'm a bit torn on this error for a couple of reasons:

The error says that the source map does not exist or cannot be parsed... but there is still an entry for it in the source map cache.
The fact that this error is about source maps, but it is only thrown from the test runner even though there is other source map functionality in core.

The error says that the source map does not exist or cannot be parsed... but there is still an entry for it in the source map cache.

There is an entry, but it's empty.

Before this change, an error was thrown because the entry was empty, and therefore didn't have any properties. Does that error get thrown anywhere else? If so, I can try and handle them as well

Please stop changing unrelated things in PRs. It makes PRs more difficult to review and is more likely to introduce issues. If you feel strongly that the source map fixtures should be renamed, please do that in a separate PR.

IMO it's related because this adds tests, and moving them into their own file is good for decluttering, but I'll move it feel it's unrelated.

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

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

Landed in 9a9409f

PR-URL: #55055 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

avivkeller requested a review from cjihrig September 21, 2024 21:00

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 mentioned this pull request Sep 21, 2024

Tracking issue: stabilization of test runner code coverage #53924

Closed

3 tasks

cjihrig reviewed Sep 22, 2024

View reviewed changes

Comment thread lib/internal/test_runner/coverage.js Outdated

avivkeller requested a review from cjihrig September 22, 2024 20:57

avivkeller added 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. source maps Issues and PRs related to source map support. labels Sep 22, 2024

mcollina approved these changes Sep 23, 2024

View reviewed changes

cjihrig reviewed Sep 24, 2024

View reviewed changes

MoLow reviewed Sep 29, 2024

View reviewed changes

Comment thread lib/internal/test_runner/coverage.js Outdated

avivkeller added blocked PRs that are blocked by other issues or PRs. and removed commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Sep 30, 2024

test_runner: throw on invalid source map

87af23b

avivkeller removed the blocked PRs that are blocked by other issues or PRs. label Oct 3, 2024

avivkeller requested review from MoLow and cjihrig October 6, 2024 18:06

cjihrig approved these changes Oct 6, 2024

View reviewed changes

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2024

cjihrig removed the needs-ci PRs that need a full CI run. label Oct 6, 2024

cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2024

nodejs-github-bot merged commit 9a9409f into nodejs:main Oct 6, 2024

aduh95 pushed a commit that referenced this pull request Oct 9, 2024

test_runner: throw on invalid source map

800f7c4

PR-URL: #55055 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

marco-ippolito added the backport-requested-v20.x label Nov 16, 2024

mcollina mentioned this pull request May 23, 2026

Could not report code coverage for file in sourcemap #55054

Closed

test_runner: throw on invalid source map by avivkeller · Pull Request #55055 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: throw on invalid source map - #55055

test_runner: throw on invalid source map#55055nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom avivkeller:test-runner-throw-invalid-source-mapCopy head branch name to clipboard

Conversation

avivkeller commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 21, 2024

Uh oh!

codecov Bot commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

avivkeller commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjihrig Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

avivkeller Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 6, 2024

Uh oh!

nodejs-github-bot commented Oct 6, 2024

Uh oh!

nodejs-github-bot commented Oct 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test_runner: throw on invalid source map#55055
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
avivkeller:test-runner-throw-invalid-source-map

avivkeller commented Sep 21, 2024 •
edited
Loading

codecov Bot commented Sep 21, 2024 •
edited
Loading

avivkeller commented Sep 22, 2024 •
edited
Loading

avivkeller commented Sep 24, 2024 •
edited
Loading