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.

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

test_runner: support source mapped test locations #52010

Closed
wants to merge 2 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 8, 2024

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
This commit adds support for source mapping test locations
when the --enable-source-maps flag is present.

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

Fixes: nodejs#51610
@cjihrig cjihrig requested review from MoLow and atlowChemi March 8, 2024 03:13
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

welcome back!

@rluvaton
Copy link
Member

rluvaton commented Mar 8, 2024

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

@legendecas
Copy link
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.

Copy link
Contributor

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

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?

@@ -359,6 +370,21 @@ class Test extends AsyncResource {
column: loc[1],
file: loc[2],
};

if (sourceMaps === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (sourceMaps) {

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 9, 2024
@github-actions 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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 9, 2024

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

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow 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 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
Copy link
Collaborator

Landed in 592c690...328642b

nodejs-github-bot pushed a commit that referenced this pull request Mar 11, 2024
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
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 cjihrig deleted the test-location branch March 11, 2024 13:21
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
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>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
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>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
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 marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label May 3, 2024
@marco-ippolito
Copy link
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
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
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>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
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>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
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>
marco-ippolito pushed a commit that referenced this pull request May 17, 2024
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
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 marco-ippolito added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v20.x PRs backported to the v20.x-staging branch. 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
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