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: throw on invalid source map #55055

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

test_runner: throw on invalid source map #55055

wants to merge 4 commits into from
+110 −74

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Sep 21, 2024 •
edited
Loading

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

@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 mentioned this pull request Sep 21, 2024
Closed
3 tasks
@codecov Codecov
Copy link

codecov bot commented Sep 21, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 88.24%. Comparing base (76edde5) to head (7aad9a2).
Report is 4 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.26%   88.24%   -0.02%     
==========================================
  Files         651      651              
  Lines      183894   183880      -14     
  Branches    35858    35858              
==========================================
- Hits       162315   162266      -49     
- Misses      14882    14898      +16     
- Partials     6697     6716      +19
Files with missing lines Coverage Δ
lib/internal/errors.js 96.98% <100.00%> (+<0.01%) ⬆️
lib/internal/test_runner/coverage.js 64.70% <50.00%> (-0.05%) ⬇️

... and 31 files with indirect coverage changes

@RedYetiDev RedYetiDev force-pushed the test-runner-throw-invalid-source-map branch from 095e3e3 to a916ce4 Compare September 22, 2024 20:57
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 22, 2024 •
edited
Loading

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

@RedYetiDev RedYetiDev 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
mcollina approved these changes Sep 23, 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 force-pushed the test-runner-throw-invalid-source-map branch from a916ce4 to 51f959c Compare September 23, 2024 20:32
cjihrig
cjihrig reviewed Sep 24, 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.

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
cjihrig reviewed Sep 24, 2024
View reviewed changes
test/parallel/test-runner-coverage-source-map.js
];

describe('Coverage with source maps', async () => {
await it('should work with source maps', async (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need the awaits on the tests.

test/parallel/test-runner-coverage-source-map.js
t.assert.strictEqual(spawned.code, 1);
});
}
}).then(common.mustCall());
Copy link
Contributor

Choose a reason for hiding this comment

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

The .then() here is not needed.

lib/internal/test_runner/coverage.js Outdated
@@ -353,6 +354,7 @@ class TestCoverage {
continue;
}
const { data, lineLengths } = sourceMapCache[url];
if (!data) throw new ERR_SOURCE_MAP_CANNOT_PARSE(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 24, 2024 •
edited
Loading

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.

@RedYetiDev RedYetiDev force-pushed the test-runner-throw-invalid-source-map branch from 7aad9a2 to 1e8287f Compare September 30, 2024 19:10
@RedYetiDev RedYetiDev 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@MoLow MoLow MoLow left review comments

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. 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.
5 participants
@RedYetiDev @nodejs-github-bot @mcollina @cjihrig @MoLow