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

test_runner: support source mapped test locations by cjihrig · Pull Request #52010 · nodejs/node · GitHub

/ node Public

test_runner: support source mapped test locations - #52010

Closed
cjihrig wants to merge 2 commits into
nodejs:mainfrom
cjihrig:test-location
ConversationCommits (2)ChecksFiles changed
Closed

test_runner: support source mapped test locations #52010
cjihrig wants to merge 2 commits into
nodejs:mainfrom
cjihrig:test-location

Conversation

cjihrig commented Mar 8, 2024

Copy link
Copy Markdown
Contributor
test_runner: support source mapped test locations

This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

Fixes: #51392

test_runner: use paths for test locations

This commit transforms test locations to paths when V8 provides
file URLs (which seems to be for ESM files).

Fixes: #51610

cjihrig added 2 commits March 7, 2024 21:19
test_runner: support source mapped test locations
3e39c37 
This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

Fixes: nodejs#51392
test_runner: use paths for test locations
0f4d2e0 
This commit transforms test locations to paths when V8 provides
file URLs (which seems to be for ESM files).

Fixes: nodejs#51610
cjihrig requested review from MoLow and atlowChemi March 8, 2024 03:13

nodejs-github-bot commented Mar 8, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

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 8, 2024
benjamingr approved these changes Mar 8, 2024
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

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

welcome back!

rluvaton commented Mar 8, 2024

Copy link
Copy Markdown
Member

Can you add a test for inline source map as well?

legendecas commented Mar 8, 2024

Copy link
Copy Markdown
Member

Can you add a test for inline source map as well?

I think the existing source map implementation and tests could cover this sufficiently.

theoludwig reviewed Mar 9, 2024
View reviewed changes

theoludwig left a comment •
edited
Loading

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

Thanks, that will be useful, for TypeScript users, that build their files to JavaScript to then execute the Node.js built-in test runner.

Does this support the --experimental-test-coverage? So that the reported uncovered lines, are the ones from the source TypeScript file?

Comment thread lib/internal/test_runner/test.js
file: loc[2],
};

if (sourceMaps === true) {

theoludwig Mar 9, 2024

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

Why checking === true and not using directly sourceMaps?

if (sourceMaps) {

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2024

nodejs-github-bot commented Mar 9, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 9, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 9, 2024

Copy link
Copy Markdown
Collaborator

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

cjihrig commented Mar 9, 2024

Copy link
Copy Markdown
Contributor Author

Does this support the --experimental-test-coverage?

@theoludwig no, not yet. That is a separate change.

nodejs-github-bot commented Mar 10, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 10, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 10, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

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

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Mar 11, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 11, 2024

nodejs-github-bot commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

Landed in 592c690...328642b

nodejs-github-bot pushed a commit that referenced this pull request Mar 11, 2024
test_runner: support source mapped test locations
6d625fe 
This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

Fixes: #51392
PR-URL: #52010
Fixes: #51610
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
nodejs-github-bot pushed a commit that referenced this pull request Mar 11, 2024
test_runner: use paths for test locations
328642b 
This commit transforms test locations to paths when V8 provides
file URLs (which seems to be for ESM files).

Fixes: #51610
PR-URL: #52010
Fixes: #51392
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
cjihrig deleted the test-location branch March 11, 2024 13:21
RafaelGSS mentioned this pull request Apr 15, 2024
Merged
gjuchault mentioned this pull request Apr 24, 2024
Closed
6 tasks
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
test_runner: support source mapped test locations
089d2ae 
This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

Fixes: #51392
PR-URL: #52010
Fixes: #51610
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito mentioned this pull request May 2, 2024
Merged

marco-ippolito commented May 3, 2024

Copy link
Copy Markdown
Member

Can you create a backport for v20, its not landing clean

MoLow pushed a commit to MoLow/node that referenced this pull request May 7, 2024
test_runner: support source mapped test locations
bdeaafd 
This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

Fixes: nodejs#51392
PR-URL: nodejs#52010
Fixes: nodejs#51610
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
MoLow pushed a commit to MoLow/node that referenced this pull request May 7, 2024
test_runner: use paths for test locations
2eac413 
This commit transforms test locations to paths when V8 provides
file URLs (which seems to be for ESM files).

Fixes: nodejs#51610
PR-URL: nodejs#52010
Fixes: nodejs#51392
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
MoLow mentioned this pull request May 7, 2024
Closed
marco-ippolito pushed a commit that referenced this pull request May 17, 2024
test_runner: support source mapped test locations
743281a 
This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

Fixes: #51392
PR-URL: #52010
Backport-PR-URL: #52872
Fixes: #51610
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 17, 2024
test_runner: use paths for test locations
be587e3 
This commit transforms test locations to paths when V8 provides
file URLs (which seems to be for ESM files).

Fixes: #51610
PR-URL: #52010
Backport-PR-URL: #52872
Fixes: #51392
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito mentioned this pull request May 23, 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

benjamingr benjamingr approved these changes
MoLow MoLow approved these changes
legendecas legendecas approved these changes
marco-ippolito marco-ippolito approved these changes
atlowChemi atlowChemi approved these changes
+1 more reviewer
theoludwig theoludwig left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. 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.

test_runner: always use paths to identify files Test runner location output not source-mapped

9 participants

Back | FazBrowse Home | New Git URL