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: support forced exit #52038

Merged
merged 1 commit into from
Mar 13, 2024
Merged

test_runner: support forced exit #52038

merged 1 commit into from
Mar 13, 2024

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 10, 2024

This commit updates the test runner to allow a forced exit once all known tests have finished running.

Fixes: #49925

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 10, 2024
@cjihrig cjihrig mentioned this pull request Mar 10, 2024
Closed
rluvaton
rluvaton reviewed Mar 11, 2024
View reviewed changes
lib/internal/test_runner/test.js
// possible that a ref'ed handle could asynchronously create more tests,
// but the user opted into this behavior.
this.reporter.once('close', () => {
process.exit();
Copy link
Member

Choose a reason for hiding this comment

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

  1. what if the reporter is already closed, should we check if the reporter is already closed?
  2. I think we should check if there are active handles before exiting so maybe the force exit is not needed
  3. maybe print a warning that some handles are refed and it's best to clean them up

Copy link
Member

Choose a reason for hiding this comment

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

what if the reporter is already closed

in this case, there is no need for forced exit. the reporter closes on a call to root.postRun

MoLow
MoLow approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

LGTM. +1 for some @rluvaton comments but not blocking (fire a warning /w count & description of open handles, test exit code not effected when using this)

@cjihrig
test_runner: support forced exit
d898cf8 
This commit updates the test runner to allow a forced exit once
all known tests have finished running.

Fixes: nodejs#49925
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 11, 2024

I've made the requested changes. I did not print any information about the handles because:

  1. It can be trivially added in a follow up if someone wants.
  2. Some users may not care to see that output since they don't even want to be bothered to make their tests exit correctly.

atlowChemi
atlowChemi approved these changes Mar 12, 2024
View reviewed changes
src/node_options.cc
Comment on lines +182 to +184
} else if (test_runner_force_exit) {
errors->push_back("either --watch or --test-force-exit "
"can be used, not both");
Copy link
Member

Choose a reason for hiding this comment

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

This is becoming a repetitive check (if AAA -> push_back("either --watch or AAA can be used, not both")),
Might be worth extracting to some function 🤔

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

rluvaton
rluvaton approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

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

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 13, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 13, 2024
@nodejs-github-bot nodejs-github-bot merged commit 84de97a into nodejs:main Mar 13, 2024
52 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 84de97a

@MoLow MoLow linked an issue Mar 13, 2024 that may be closed by this pull request
node:test hangs when BroadcastChannel is left open #52073
Closed
@cjihrig cjihrig deleted the force-exit branch March 13, 2024 13:36
@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 14, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@cjihrig @rdw-msft
test_runner: support forced exit
58e44b8 
This commit updates the test runner to allow a forced exit once
all known tests have finished running.

Fixes: nodejs#49925
PR-URL: nodejs#52038
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@cjihrig @marco-ippolito
test_runner: support forced exit
e054dc8 
This commit updates the test runner to allow a forced exit once
all known tests have finished running.

Fixes: #49925
PR-URL: #52038
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
marco-ippolito added a commit that referenced this pull request May 2, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
7a683cc 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: TODO
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
Merged
marco-ippolito added a commit that referenced this pull request May 2, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
6db6838 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: TODO
marco-ippolito added a commit that referenced this pull request May 2, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
3db5f4c 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: #52793
marco-ippolito added a commit that referenced this pull request May 2, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
2e68800 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: #52793
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
@cjihrig @jcbhmr
test_runner: support forced exit
c660655 
This commit updates the test runner to allow a forced exit once
all known tests have finished running.

Fixes: nodejs#49925
PR-URL: nodejs#52038
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 23, 2024
@cjihrig @marco-ippolito
test_runner: support forced exit
77e2bf0 
This commit updates the test runner to allow a forced exit once
all known tests have finished running.

Fixes: #49925
PR-URL: #52038
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
marco-ippolito added a commit that referenced this pull request May 23, 2024
@marco-ippolito
2024-05-28, Version 20.14.0 'Iron' (LTS)
83e7936 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) #52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038

PR-URL: TODO
@marco-ippolito marco-ippolito mentioned this pull request May 23, 2024
Merged
marco-ippolito added a commit that referenced this pull request May 23, 2024
@marco-ippolito
2024-05-28, Version 20.14.0 'Iron' (LTS)
b4386a2 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) #52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038

PR-URL: #53120
marco-ippolito added a commit that referenced this pull request May 23, 2024
@marco-ippolito
2024-05-28, Version 20.14.0 'Iron' (LTS)
911a9a7 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) #52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038

PR-URL: #53120
marco-ippolito added a commit that referenced this pull request May 27, 2024
@marco-ippolito
2024-05-28, Version 20.14.0 'Iron' (LTS)
4b2d1a0 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) #52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038

PR-URL: #53120
marco-ippolito added a commit that referenced this pull request May 27, 2024
@marco-ippolito
2024-05-28, Version 20.14.0 'Iron' (LTS)
fe0f08a 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) #52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038

PR-URL: #53120
marco-ippolito added a commit that referenced this pull request May 28, 2024
@marco-ippolito
2024-05-28, Version 20.14.0 'Iron' (LTS)
8c6d43c 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) #52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038

PR-URL: #53120
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
@marco-ippolito @EliphazBouye
2024-05-28, Version 20.14.0 'Iron' (LTS)
ddb2326 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) nodejs#52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) nodejs#52038

PR-URL: nodejs#53120
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@marco-ippolito @bmeck
2024-05-28, Version 20.14.0 'Iron' (LTS)
b3ed657 
Notable changes:

src,permission:
  * throw async errors on async APIs (Rafael Gonzaga) nodejs#52730
test_runner:
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) nodejs#52038

PR-URL: nodejs#53120
@samuelmurray samuelmurray mentioned this pull request Aug 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi approved these changes

@rluvaton rluvaton rluvaton approved these changes

@MoLow MoLow MoLow approved these changes

@benjamingr benjamingr benjamingr approved these changes

@juliangruber juliangruber juliangruber approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

node:test hangs when BroadcastChannel is left open test_runner: allow to force exit after all tests finished
7 participants
@cjihrig @nodejs-github-bot @juliangruber @benjamingr @MoLow @rluvaton @atlowChemi