test_runner: throw on invalid source map #55055

avivkeller · 2024-09-21T21:00:40Z

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.

nodejs-github-bot · 2024-09-21T21:00:45Z

Review requested:

@nodejs/test_runner

codecov · 2024-09-21T22:10:47Z

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

lib/internal/test_runner/coverage.js

avivkeller · 2024-09-22T20:57:54Z

~~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

mcollina

lgtm

cjihrig

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.

cjihrig · 2024-09-24T14:06:21Z

test/parallel/test-runner-coverage-source-map.js

+];
+
+describe('Coverage with source maps', async () => {
+  await it('should work with source maps', async (t) => {


You shouldn't need the awaits on the tests.

test/parallel/test-runner-coverage-source-map.js

cjihrig · 2024-09-24T14:20:19Z

lib/internal/test_runner/coverage.js

@@ -353,6 +354,7 @@ class TestCoverage {
        continue;
      }
      const { data, lineLengths } = sourceMapCache[url];
+      if (!data) throw new ERR_SOURCE_MAP_CANNOT_PARSE(url);


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

avivkeller · 2024-09-24T16:05:21Z

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.

lib/internal/test_runner/coverage.js

nodejs-github-bot · 2024-10-06T18:13:38Z

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

nodejs-github-bot · 2024-10-06T19:40:38Z

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

nodejs-github-bot · 2024-10-06T20:27:33Z

Landed in 9a9409f

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

PR-URL: nodejs#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

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved

avivkeller force-pushed the test-runner-throw-invalid-source-map branch from 095e3e3 to a916ce4 Compare September 22, 2024 20:57

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

avivkeller force-pushed the test-runner-throw-invalid-source-map branch from a916ce4 to 51f959c Compare September 23, 2024 20:32

cjihrig reviewed Sep 24, 2024

View reviewed changes

MoLow reviewed Sep 29, 2024

View reviewed changes

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved

avivkeller force-pushed the test-runner-throw-invalid-source-map branch from 7aad9a2 to 1e8287f Compare September 30, 2024 19:10

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 force-pushed the test-runner-throw-invalid-source-map branch from 97d87a6 to 87af23b Compare October 3, 2024 23:21

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

avivkeller requested review from cjihrig and MoLow 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 added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels 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
59 checks passed

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

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

test_runner: throw on invalid source map

1b305f5

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

marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Nov 16, 2024

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

test_runner: throw on invalid source map

39e950c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: throw on invalid source map #55055

test_runner: throw on invalid source map #55055

avivkeller commented Sep 21, 2024 •

edited

Loading

nodejs-github-bot commented Sep 21, 2024

codecov bot commented Sep 21, 2024 •

edited

Loading

avivkeller commented Sep 22, 2024 •

edited

Loading

mcollina left a comment

cjihrig left a comment

cjihrig Sep 24, 2024

cjihrig Sep 24, 2024

avivkeller Sep 24, 2024

avivkeller commented Sep 24, 2024 •

edited

Loading

nodejs-github-bot commented Oct 6, 2024

nodejs-github-bot commented Oct 6, 2024

nodejs-github-bot commented Oct 6, 2024

test_runner: throw on invalid source map #55055

test_runner: throw on invalid source map #55055

Conversation

avivkeller commented Sep 21, 2024 • edited Loading

nodejs-github-bot commented Sep 21, 2024

codecov bot commented Sep 21, 2024 • edited Loading

Codecov Report

avivkeller commented Sep 22, 2024 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Sep 24, 2024

Choose a reason for hiding this comment

cjihrig Sep 24, 2024

Choose a reason for hiding this comment

avivkeller Sep 24, 2024

Choose a reason for hiding this comment

avivkeller commented Sep 24, 2024 • edited Loading

nodejs-github-bot commented Oct 6, 2024

nodejs-github-bot commented Oct 6, 2024

nodejs-github-bot commented Oct 6, 2024

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