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

[v18.x backport] test_runner various features #46360

Closed

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Jan 26, 2023

this is a backport for a batch of PRS that depend on each other, so it was convenient to backport in bulk

#45264
#45712
#45923
#46030
#46092

anonrig and others added 30 commits January 24, 2023 16:39
PR-URL: nodejs#45865
Fixes: nodejs#45535
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
If we are in an artifically created Environment that has no args set,
and uv_exepath returns an error (for instance, if /proc is not mounted
on a Linux system), then we crash with a nullptr deref attempting to
use argv[0].

Fixes: nodejs#45901
PR-URL: nodejs#45902
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#45868
PR-URL: nodejs#45882
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Tailing slash of url.href is ommited.

PR-URL: nodejs#45928
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Signed-off-by: RafaelGSS <[email protected]>
PR-URL: nodejs#45876
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: nodejs#45887
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This commit exposes uv_available_parallelism() as an alternative
to cpus().length. uv_available_parallelism() is inspired by
Rust's available_parallelism().

PR-URL: nodejs#45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
With the introduction of os.availableParallelism(), users should
no longer rely on os.cpus().length to determine the amount of
available parallelism. This commit adds a note to the os.cpus()
docs.

PR-URL: nodejs#45895
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#45914
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#45803
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#45803
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#45803
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: nodejs#45793
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
daae938 broke addons which create their own `Isolate`
instances, because enabling the shared-readonly-heap feature
of V8 requires all snapshots used for different `Isolate`s to
be identical. Usage of addons that do this has probably
decreased quite a bit since Worker threads were introduced
in Node.js, but it’s still a valid use case, and in any case
the breakage was probably not intentional (although the referenced
commit did require test changes because of this issue).

This commit addresses this issue partially by caching the
V8 snapshot parameters and ignoring ones passed in from users
in `NewIsolate()` when this feature is enabled, and makes
the `NodeMainInstance` snapshot-based isolate creation
also re-use this code.

PR-URL: nodejs#45885
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#45935
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Update the GitHub workflow action used for closing stalled issues
and PRs.

PR-URL: nodejs#45937
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mestery <[email protected]>
User can check output of example easily.

PR-URL: nodejs#45915
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#45940
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.

PR-URL: nodejs#45927
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Co-authored-by: Luigi Pinca <[email protected]>
PR-URL: nodejs#45778
Fixes: nodejs#43355
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#45957
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Qingyu Deng <[email protected]>
Error message in document is different from actual result.

PR-URL: nodejs#45920
Reviewed-By: Kohei Ueno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Starting vcbuild.bat for cross-compiling from powershell was failing the
licensertf step because it couldn't find x64_node_exe after downloading.

PR-URL: nodejs#45890
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
This reverts commit 09f33c9.

PR-URL: nodejs#45948
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#45947
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Cope with the delay(to the next function call) of
v8::Isolate::TerminateExecution()

PR-URL: nodejs#45620
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Jesse has informed me that he won't be able to contribute to this
initiative, unfortunately. If you would like to know why, feel free to
ask him or me privately. I would like to take over.

Signed-off-by: Darshan Sen <[email protected]>
PR-URL: nodejs#45956
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#45972
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#45960
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
aduh95 and others added 10 commits January 26, 2023 13:38
We already always specify a value, and failing to do so would likely be
a bug.

PR-URL: nodejs#46164
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
- add text discussed by the TSC

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#46121
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Joe Sepi <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
tls.createServer() and new tls.Server() ignore secureContext option.

PR-URL: nodejs#46224
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#45904
Fixes: nodejs#45838
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Refs: nodejs#39316
PR-URL: nodejs#46205
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46242
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
closes nodejs#45987

PR-URL: nodejs#46042
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Fixes: nodejs#45755
PR-URL: nodejs#45760
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Signed-off-by: Juan José Arboleda <[email protected]>
PR-URL: nodejs#45241
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@GeoffreyBooth
Copy link
Member

Aren’t the reporters behind --experimental-reporter? If so then I think they can land on both lines, but I defer to @nodejs/releasers

nodejs-github-bot and others added 8 commits January 26, 2023 14:21
PR-URL: nodejs#46257
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#46257
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <[email protected]>
PR-URL: nodejs#46220
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
`event.returnvalue` is described as legacy in spec.
Plus, add missed '#'(private member) of defaultPrevented
in implementation.

Refs: https://dom.spec.whatwg.org/#interface-event
Refs: https://developer.mozilla.org/en-US/docs/Web/API/Event/returnValue
PR-URL: nodejs#46175
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Fixes: nodejs#42866
PR-URL: nodejs#46226
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Arguments of some APIs are mismatched and 2 APIs are not as
described.

PR-URL: nodejs#45678
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46212
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

fossamagna and others added 5 commits January 26, 2023 23:46
PR-URL: nodejs#45712
Fixes: nodejs#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Move the logic for handling --test-reporter out of the
general module loader and into the test_runner subsystem.

PR-URL: nodejs#45923
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#46030
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This commit updates the test runner to make the built in test
reporters internal modules.

PR-URL: nodejs#46092
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@MoLow MoLow force-pushed the backport-v18-test-runner-reporters branch from 1ea2fa2 to e304cc5 Compare January 26, 2023 21:47
@ruyadorno
Copy link
Member

Personally I haven't heard of an exception to the rule for experimental APIs and I would prefer if we could wait the regular 4-week-from-landing-on-current period of time.

@MoLow
Copy link
Member Author

MoLow commented Jan 28, 2023

then let's wait for the next release 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.