test: simplify common/index.js #56712

jasnell · 2025-01-22T23:33:46Z

Multiple improvements in tests and the test harness

Eliminate single or limited use utilities from common/index.js in order to generally reduce reliance on the custom test harness
Replace uses of the older Node.js-specific global in favor of globalThis
Handful of other cleanups

See individual commit messages for more detail.

Move single or trivial and limited use things out of common/index.js for the purpose of simplifying and reducing common/index.js

There are several cleanups here that are not just style nits... 1. The `common.isMainThread` was just a passthrough to the `isMainThread` export on the worker_thread module. It's use was inconsistent and just obfuscated the fact that the test file depend on the `worker_threads` built-in. By eliminating it we simplify the test harness a bit and make it clearer which tests depend on the worker_threads check. 2. The `common.isDumbTerminal` is fairly unnecesary since that just wraps a public API check. 3. Several of the `common.skipIf....` checks were inconsistently used and really don't need to be separate utility functions. A key part of the motivation here is to work towards making more of the tests more self-contained and less reliant on the common test harness where possible.

* s/global/globalThis * clean up knownGlobals a bit, make it a Set instead of an array and condense a bit.

mcollina

lgtm

nodejs-github-bot · 2025-01-24T18:07:19Z

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

jasnell · 2025-01-24T18:10:56Z

Up to 7 flaky CI runs and counting....

jasnell · 2025-01-24T18:42:42Z

Issue for the persistently flaky test: #56753

nodejs-github-bot · 2025-01-24T23:38:07Z

Commit Queue failed

- Loading data for nodejs/node/pull/56712 ✔ Done loading data for nodejs/node/pull/56712 ----------------------------------- PR info ------------------------------------ Title test: simplify common/index.js (#56712) Author James M Snell <jasnell@gmail.com> (@jasnell) Branch jasnell:jasnell/simplify-common-test-harness -> nodejs:main Labels test, async_hooks, esm, author ready, needs-ci Commits 4 - test: simplify common/index.js - test: rely less on duplicative common test harness utilities - test: make common/index slightly less node.js specific - test: replace more uses of `global` with `globalThis` Committers 1 - James M Snell <jasnell@gmail.com> PR-URL: https://github.com/nodejs/node/pull/56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 22 Jan 2025 23:33:46 GMT ✔ Approvals: 2 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/56712#pullrequestreview-2568584470 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/56712#pullrequestreview-2572077668 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-01-24T18:07:19Z: https://ci.nodejs.org/job/node-test-pull-request/64723/ - Querying data for job/node-test-pull-request/64723/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 56712 From https://github.com/nodejs/node * branch refs/pull/56712/merge -> FETCH_HEAD ✔ Fetched commits as c752615e2bbe..ef9cb6d83fc7 -------------------------------------------------------------------------------- [main 115c795fab] test: simplify common/index.js Author: James M Snell <jasnell@gmail.com> Date: Wed Jan 22 13:05:54 2025 -0800 6 files changed, 19 insertions(+), 28 deletions(-) [main 38959857bf] test: rely less on duplicative common test harness utilities Author: James M Snell <jasnell@gmail.com> Date: Wed Jan 22 14:19:38 2025 -0800 148 files changed, 672 insertions(+), 290 deletions(-) [main da172e7fd2] test: make common/index slightly less node.js specific Author: James M Snell <jasnell@gmail.com> Date: Wed Jan 22 15:08:23 2025 -0800 1 file changed, 57 insertions(+), 83 deletions(-) Auto-merging test/parallel/test-util-inspect.js [main 91d903dbe3] test: replace more uses of `global` with `globalThis` Author: James M Snell <jasnell@gmail.com> Date: Wed Jan 22 15:30:30 2025 -0800 74 files changed, 246 insertions(+), 242 deletions(-) ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. Rebasing (2/8) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- test: simplify common/index.js

Move single or trivial and limited use things out of
common/index.js for the purpose of simplifying and reducing
common/index.js

PR-URL: #56712
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 3531464810] test: simplify common/index.js
Author: James M Snell <jasnell@gmail.com>
Date: Wed Jan 22 13:05:54 2025 -0800
6 files changed, 19 insertions(+), 28 deletions(-)
Rebasing (3/8)
Rebasing (4/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: rely less on duplicative common test harness utilities

There are several cleanups here that are not just style nits...

The common.isMainThread was just a passthrough to the
isMainThread export on the worker_thread module. It's
use was inconsistent and just obfuscated the fact that
the test file depend on the worker_threads built-in.
By eliminating it we simplify the test harness a bit and
make it clearer which tests depend on the worker_threads
check.
The common.isDumbTerminal is fairly unnecesary since
that just wraps a public API check.
Several of the common.skipIf.... checks were inconsistently
used and really don't need to be separate utility functions.

A key part of the motivation here is to work towards making more
of the tests more self-contained and less reliant on the common
test harness where possible.

PR-URL: #56712
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 4f6ab03b3b] test: rely less on duplicative common test harness utilities
Author: James M Snell <jasnell@gmail.com>
Date: Wed Jan 22 14:19:38 2025 -0800
148 files changed, 672 insertions(+), 290 deletions(-)
Rebasing (5/8)
Rebasing (6/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: make common/index slightly less node.js specific

s/global/globalThis
clean up knownGlobals a bit, make it a Set instead of an array
and condense a bit.

PR-URL: #56712
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 98eb0492e7] test: make common/index slightly less node.js specific
Author: James M Snell <jasnell@gmail.com>
Date: Wed Jan 22 15:08:23 2025 -0800
1 file changed, 57 insertions(+), 83 deletions(-)
Rebasing (7/8)
Rebasing (8/8)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: replace more uses of global with globalThis

PR-URL: #56712
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 6e4c511660] test: replace more uses of `global` with `globalThis`
Author: James M Snell <jasnell@gmail.com>
Date: Wed Jan 22 15:30:30 2025 -0800
74 files changed, 246 insertions(+), 242 deletions(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/12959421676

PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Move single or trivial and limited use things out of common/index.js for the purpose of simplifying and reducing common/index.js PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

There are several cleanups here that are not just style nits... 1. The `common.isMainThread` was just a passthrough to the `isMainThread` export on the worker_thread module. It's use was inconsistent and just obfuscated the fact that the test file depend on the `worker_threads` built-in. By eliminating it we simplify the test harness a bit and make it clearer which tests depend on the worker_threads check. 2. The `common.isDumbTerminal` is fairly unnecesary since that just wraps a public API check. 3. Several of the `common.skipIf....` checks were inconsistently used and really don't need to be separate utility functions. A key part of the motivation here is to work towards making more of the tests more self-contained and less reliant on the common test harness where possible. PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

* s/global/globalThis * clean up knownGlobals a bit, make it a Set instead of an array and condense a bit. PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Move single or trivial and limited use things out of common/index.js for the purpose of simplifying and reducing common/index.js PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

There are several cleanups here that are not just style nits... 1. The `common.isMainThread` was just a passthrough to the `isMainThread` export on the worker_thread module. It's use was inconsistent and just obfuscated the fact that the test file depend on the `worker_threads` built-in. By eliminating it we simplify the test harness a bit and make it clearer which tests depend on the worker_threads check. 2. The `common.isDumbTerminal` is fairly unnecesary since that just wraps a public API check. 3. Several of the `common.skipIf....` checks were inconsistently used and really don't need to be separate utility functions. A key part of the motivation here is to work towards making more of the tests more self-contained and less reliant on the common test harness where possible. PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

* s/global/globalThis * clean up knownGlobals a bit, make it a Set instead of an array and condense a bit. PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Move single or trivial and limited use things out of common/index.js for the purpose of simplifying and reducing common/index.js PR-URL: nodejs#56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

There are several cleanups here that are not just style nits... 1. The `common.isMainThread` was just a passthrough to the `isMainThread` export on the worker_thread module. It's use was inconsistent and just obfuscated the fact that the test file depend on the `worker_threads` built-in. By eliminating it we simplify the test harness a bit and make it clearer which tests depend on the worker_threads check. 2. The `common.isDumbTerminal` is fairly unnecesary since that just wraps a public API check. 3. Several of the `common.skipIf....` checks were inconsistently used and really don't need to be separate utility functions. A key part of the motivation here is to work towards making more of the tests more self-contained and less reliant on the common test harness where possible. PR-URL: nodejs#56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

* s/global/globalThis * clean up knownGlobals a bit, make it a Set instead of an array and condense a bit. PR-URL: nodejs#56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#56712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>