Sitelet https://web.archive.org/web/20221106105320/https://github.com/nodejs/node/pull/44520
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

Jump to bottom

test_runner: support using --inspect with --test #44520

Merged
merged 15 commits into from Sep 10, 2022
Merged

test_runner: support using --inspect with --test #44520

merged 15 commits into from Sep 10, 2022

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Sep 5, 2022 •
edited

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. test_runner labels Sep 5, 2022
@MoLow
Copy link
Member Author

MoLow commented Sep 5, 2022

CC @nodejs/test_runner

benjamingr
benjamingr reviewed Sep 5, 2022
View changes
doc/api/test.md Show resolved Hide resolved
benjamingr
benjamingr reviewed Sep 5, 2022
View changes
Copy link
Member

@benjamingr benjamingr left a comment

👍

@MoLow MoLow added the dont-land-on-v14.x PRs that should not land on the v14.x-staging branch and should not be released in v14.x. label Sep 5, 2022
aduh95
aduh95 reviewed Sep 6, 2022
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment

We want to throw if inspectPort is not of the expected type.

doc/api/test.md Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 6, 2022
MoLow
MoLow commented Sep 6, 2022
View changes
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Sep 6, 2022
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment

I find it kinda hard to follow what this file is doing, I wonder if it should be split into several files for better readability.

test/sequential/test-runner-run-inspect.js Outdated Show resolved Hide resolved
test/sequential/test-runner-run-inspect.js Outdated Show resolved Hide resolved
@MoLow
Copy link
Member Author

MoLow commented Sep 6, 2022

I find it kinda hard to follow what this file is doing, I wonder if it should be split into several files for better readability.

I mainly copied from test-inspector-port-cluster.js

ljharb
ljharb reviewed Sep 6, 2022
View changes
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated
function isUsingInspector() {
return ArrayPrototypeSome(process.execArgv, (arg) => RegExpPrototypeExec(kInspectArgRegex, arg) !== null) ||
RegExpPrototypeExec(kInspectArgRegex, process.env.NODE_OPTIONS) !== null;
}
Copy link
Member

@ljharb ljharb Sep 6, 2022

Choose a reason for hiding this comment

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

this kind of seems like it should just be a boolean property available on process or something

benjamingr
benjamingr approved these changes Sep 6, 2022
View changes
Copy link
Member

@benjamingr benjamingr left a comment

Not sure about everything here but this is fine to land and then iterate and a bit improvement

aduh95
aduh95 reviewed Sep 7, 2022
View changes
test/sequential/test-runner-run-inspect.js Outdated Show resolved Hide resolved
test/sequential/test-runner-run-inspect.js Outdated Show resolved Hide resolved
test/sequential/test-runner-run-inspect.js Outdated Show resolved Hide resolved
@MoLow MoLow requested a review from aduh95 Sep 8, 2022
aduh95
aduh95 reviewed Sep 8, 2022
View changes
test/fixtures/test-runner/run_inspect.js Show resolved Hide resolved
test/fixtures/test-runner/run_inspect.js Outdated Show resolved Hide resolved
test/fixtures/test-runner/run_inspect_assert.js Show resolved Hide resolved
test/fixtures/test-runner/run_inspect_assert.js Outdated Show resolved Hide resolved
test/fixtures/test-runner/run_inspect.js Outdated Show resolved Hide resolved
@MoLow @aduh95
Apply suggestions from code review
dea4784 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
aduh95 approved these changes Sep 8, 2022
View changes
lib/internal/cluster/primary.js Outdated Show resolved Hide resolved
@MoLow @aduh95
Update lib/internal/cluster/primary.js
85b5716 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@MoLow @RafaelGSS
test_runner: support using --inspect with --test
603eb54 
PR-URL: #44520
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@MoLow @RafaelGSS
test_runner: support using --inspect with --test
689a2ee 
PR-URL: #44520
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 27, 2022
@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 27, 2022

As mentioned in: #44799 (comment). Could you please create a manual backport?

MoLow added a commit to MoLow/node that referenced this pull request Sep 29, 2022
@MoLow
test_runner: support using --inspect with --test
1cffc6c 
PR-URL: nodejs#44520
Backport-PR-URL: TBD
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow mentioned this pull request Sep 29, 2022
Closed
MoLow added a commit to MoLow/node that referenced this pull request Sep 29, 2022
@MoLow
test_runner: support using --inspect with --test
4e3c2ca 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44813
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Sep 29, 2022
MoLow added a commit to MoLow/node that referenced this pull request Sep 29, 2022
@MoLow
test_runner: support using --inspect with --test
6b59510 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44813
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Sep 29, 2022
@MoLow
test_runner: support using --inspect with --test
7f2f502 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44813
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Oct 2, 2022
@MoLow
test_runner: support using --inspect with --test
faa8f8a 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44813
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@juanarbol
Copy link
Member

juanarbol commented Oct 3, 2022

This seems to need to be backported for the v16.x release branch as well. Would you mind backporting this to the v16.x release branch?

@juanarbol juanarbol added the backport-requested-v16.x PRs awaiting manual backport to the v16.x-staging branch. label Oct 3, 2022
@MoLow MoLow mentioned this pull request Oct 3, 2022
Open
MoLow added a commit to MoLow/node that referenced this pull request Oct 3, 2022
@MoLow
test_runner: support using --inspect with --test
e3c3e60 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow added backport-open-v16.x Indicate that the PR has an open backport. and removed backport-requested-v16.x PRs awaiting manual backport to the v16.x-staging branch. labels Oct 3, 2022
MoLow added a commit to MoLow/node that referenced this pull request Oct 3, 2022
@MoLow
test_runner: support using --inspect with --test
e2fe4f1 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Oct 3, 2022
@MoLow
test_runner: support using --inspect with --test
c5db7f0 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 4, 2022
@MoLow @danielleadams
test_runner: support using --inspect with --test
3c1e9d4 
PR-URL: #44520
Backport-PR-URL: #44813
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Oct 4, 2022
@danielleadams danielleadams mentioned this pull request Oct 11, 2022
Merged
MoLow added a commit to MoLow/node that referenced this pull request Oct 12, 2022
@MoLow
test_runner: support using --inspect with --test
269d702 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Oct 12, 2022
@MoLow
test_runner: support using --inspect with --test
a62692b 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Oct 12, 2022
@MoLow
test_runner: support using --inspect with --test
b16f87c 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Oct 12, 2022
@MoLow
test_runner: support using --inspect with --test
87a3a3f 
PR-URL: nodejs#44520
Backport-PR-URL: nodejs#44873
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow mentioned this pull request Oct 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb

@benjamingr benjamingr

@aduh95 aduh95

Assignees
No one assigned
Labels
backport-open-v16.x Indicate that the PR has an open backport. backported-to-v18.x PRs backported to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v14.x PRs that should not land on the v14.x-staging branch and should not be released in v14.x. needs-ci PRs that need a full CI run. test_runner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@MoLow @nodejs-github-bot @tniessen @RafaelGSS @juanarbol @ljharb @benjamingr @aduh95 @danielleadams