test_runner: use source maps when reporting coverage by MoLow · Pull Request #52060 · nodejs/node

MoLow · 2024-03-12T19:35:56Z

No description provided.

nodejs-github-bot · 2024-03-12T19:36:02Z

Review requested:

@nodejs/test_runner

test/parallel/test-runner-coverage.js

test/fixtures/test-runner/coverage/README.md

lib/internal/test_runner/coverage.js

cjihrig · 2024-03-13T00:41:05Z

lib/internal/test_runner/coverage.js

+            .findEntry(lines[lines.length - 1].line - 1, (endOffset - lines[lines.length - 1].startOffset) - 1);
+          if (!startEntry.originalSource && endEntry.originalSource &&
+            lines[0].line === 1 && startOffset === 0 && lines[0].startOffset === 0) {
+            // Edge case when the first line is not mappable


Why is the first line not mappable? It looks like there are a couple more "not mappable" comments. Do you know why they aren't mappable? If so, can it be included in the comments.

what findEntry does is a binary search for the first entry that appears in the source map after the line/column you request.
there are many cases where such an entry cannot be found (generated code doesn't always point to a source for various reasons - it might be a comment added by the build tool source maps exclude node_modules, etc)
this specific edge case is important since v8 coverage will almost always have a range starting at the beginning of generating code ({line:0,col:0}) until the end of the end of the file. since this range marks the count of for all top-level lines outside of a functions it is very important so I added a specific handling for it

node/lib/internal/source_map/source_map.js

Lines 190 to 220 in 1abff07

findEntry(lineOffset, columnOffset) {

let first = 0;

let count = this.#mappings.length;

while (count > 1) {

const step = count >> 1;

const middle = first + step;

const mapping = this.#mappings[middle];

if (lineOffset < mapping[0] ||

(lineOffset === mapping[0] && columnOffset < mapping[1])) {

count = step;

} else {

first = middle;

count -= step;

}

}

const entry = this.#mappings[first];

if (!first && entry && (lineOffset < entry[0] ||

(lineOffset === entry[0] && columnOffset < entry[1]))) {

return {};

} else if (!entry) {

return {};

}

return {

generatedLine: entry[0],

generatedColumn: entry[1],

originalSource: entry[2],

originalLine: entry[3],

originalColumn: entry[4],

name: entry[5],

};

}

nodejs-github-bot · 2024-03-13T19:32:53Z

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

nodejs-github-bot · 2024-03-13T20:30:34Z

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

nodejs-github-bot · 2024-03-13T22:31:45Z

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

nodejs-github-bot · 2024-03-14T19:41:32Z

Commit Queue failed

- Loading data for nodejs/node/pull/52060 ✔ Done loading data for nodejs/node/pull/52060 ----------------------------------- PR info ------------------------------------ Title test_runner: use source maps when reporting coverage (#52060) Author Moshe Atlow (@MoLow) Branch MoLow:test-runner-coverage-use-source-maps -> nodejs:main Labels needs-ci, commit-queue-rebase, test_runner Commits 2 - test_runner: use source maps when reporting coverage - CR Committers 1 - Moshe Atlow PR-URL: https://github.com/nodejs/node/pull/52060 Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52060 Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 12 Mar 2024 19:35:56 GMT ✔ Approvals: 3 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/52060#pullrequestreview-1932962596 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/52060#pullrequestreview-1935039694 ✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/52060#pullrequestreview-1935240850 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-03-13T22:31:45Z: https://ci.nodejs.org/job/node-test-pull-request/57738/ - Querying data for job/node-test-pull-request/57738/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52060 From https://github.com/nodejs/node * branch refs/pull/52060/merge -> FETCH_HEAD ✔ Fetched commits as b360532f1a38..9902bc5a26e5 -------------------------------------------------------------------------------- [main d74f9351de] test_runner: use source maps when reporting coverage Author: Moshe Atlow Date: Mon Mar 11 22:44:25 2024 +0200 11 files changed, 302 insertions(+), 118 deletions(-) create mode 100644 test/fixtures/test-runner/coverage/README.md create mode 100644 test/fixtures/test-runner/coverage/a.test.mjs create mode 100644 test/fixtures/test-runner/coverage/a.test.mjs.map create mode 100644 test/fixtures/test-runner/coverage/a.test.ts create mode 100644 test/fixtures/test-runner/coverage/b.test.ts create mode 100644 test/fixtures/test-runner/coverage/index.test.js create mode 100644 test/fixtures/test-runner/coverage/stdin.test.js create mode 100644 test/fixtures/test-runner/coverage/stdin.test.js.map [main 0c669f7030] CR Author: Moshe Atlow Date: Wed Mar 13 21:17:38 2024 +0200 3 files changed, 3 insertions(+), 5 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: use source maps when reporting coverage

PR-URL: #52060
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il

[detached HEAD 35522c3a19] test_runner: use source maps when reporting coverage
Author: Moshe Atlow moshe@atlow.co.il
Date: Mon Mar 11 22:44:25 2024 +0200
11 files changed, 302 insertions(+), 118 deletions(-)
create mode 100644 test/fixtures/test-runner/coverage/README.md
create mode 100644 test/fixtures/test-runner/coverage/a.test.mjs
create mode 100644 test/fixtures/test-runner/coverage/a.test.mjs.map
create mode 100644 test/fixtures/test-runner/coverage/a.test.ts
create mode 100644 test/fixtures/test-runner/coverage/b.test.ts
create mode 100644 test/fixtures/test-runner/coverage/index.test.js
create mode 100644 test/fixtures/test-runner/coverage/stdin.test.js
create mode 100644 test/fixtures/test-runner/coverage/stdin.test.js.map
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
CR

PR-URL: #52060
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il

[detached HEAD fd85539ccb] CR
Author: Moshe Atlow moshe@atlow.co.il
Date: Wed Mar 13 21:17:38 2024 +0200
3 files changed, 3 insertions(+), 5 deletions(-)

Successfully rebased and updated refs/heads/main.

✔ 35522c3a19f76b26b91d85d41b1f5779ecef0686
✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length
✖ fd85539ccb3aae21a0de6a17d42c11a62ab7f557
✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.
Please manually ammend the commit message, by running
git commit --amend
Once commit message is fixed, finish the landing command running
git node land --continue

https://github.com/nodejs/node/actions/runs/8286634396

nodejs-github-bot · 2024-03-15T06:33:43Z

Landed in 814fa1a

PR-URL: nodejs#52060 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

cjihrig · 2024-04-12T15:24:16Z

@MoLow should the source map limitation in the docs be removed after this change?

PR-URL: #52060 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

PR-URL: nodejs#52060 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

MoLow requested review from atlowChemi, benjamingr and cjihrig March 12, 2024 19:35

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 12, 2024

MoLow force-pushed the test-runner-coverage-use-source-maps branch from e0d502c to 326f7a0 Compare March 12, 2024 19:36

MoLow changed the title ~~test_runner: use report coverage with source maps~~ test_runner: use source maps when reporting coverage Mar 12, 2024

test_runner: use source maps when reporting coverage

e9dcde5

MoLow force-pushed the test-runner-coverage-use-source-maps branch from 326f7a0 to e9dcde5 Compare March 12, 2024 19:42

cjihrig approved these changes Mar 13, 2024

View reviewed changes

CR

9902bc5

MoLow added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Mar 13, 2024

benjamingr approved these changes Mar 13, 2024

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2024

atlowChemi approved these changes Mar 13, 2024

View reviewed changes

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 13, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 14, 2024

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

nodejs-github-bot merged commit 814fa1a into nodejs:main Mar 15, 2024

MoLow deleted the test-runner-coverage-use-source-maps branch March 15, 2024 06:36

MoLow mentioned this pull request Apr 13, 2024

doc: update test runner coverage limitations #52515

Merged

RafaelGSS mentioned this pull request Apr 15, 2024

2024-04-24, Version 22.0.0 (Current) #52505

Merged

marco-ippolito mentioned this pull request May 2, 2024

v20.13.0 proposal #52793

Merged

	findEntry(lineOffset, columnOffset) {
	let first = 0;
	let count = this.#mappings.length;
	while (count > 1) {
	const step = count >> 1;
	const middle = first + step;
	const mapping = this.#mappings[middle];
	if (lineOffset < mapping[0] \|\|
	(lineOffset === mapping[0] && columnOffset < mapping[1])) {
	count = step;
	} else {
	first = middle;
	count -= step;
	}
	}
	const entry = this.#mappings[first];
	if (!first && entry && (lineOffset < entry[0] \|\|
	(lineOffset === entry[0] && columnOffset < entry[1]))) {
	return {};
	} else if (!entry) {
	return {};
	}
	return {
	generatedLine: entry[0],
	generatedColumn: entry[1],
	originalSource: entry[2],
	originalLine: entry[3],
	originalColumn: entry[4],
	name: entry[5],
	};
	}

Uh oh!

Conversation

MoLow commented Mar 12, 2024

Uh oh!

nodejs-github-bot commented Mar 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjihrig Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

MoLow Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 13, 2024

Uh oh!

nodejs-github-bot commented Mar 13, 2024

Uh oh!

nodejs-github-bot commented Mar 13, 2024

Uh oh!

nodejs-github-bot commented Mar 14, 2024

PR-URL: #52060 Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Chemi Atlow chemi@atlow.co.il

PR-URL: #52060 Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Chemi Atlow chemi@atlow.co.il

Successfully rebased and updated refs/heads/main.

Uh oh!

nodejs-github-bot commented Mar 15, 2024

Uh oh!

cjihrig commented Apr 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PR-URL: #52060
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il

PR-URL: #52060
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il