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: simplify common/index.js #56712

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 22, 2025

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.
@jasnell jasnell requested review from mcollina and anonrig January 22, 2025 23:33
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 22, 2025
@nodejs-github-bot

This comment was marked as outdated.

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@mcollina mcollina 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

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 24, 2025

@jasnell
Copy link
Member Author

jasnell commented Jan 24, 2025

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

@jasnell
Copy link
Member Author

jasnell commented Jan 24, 2025

Issue for the persistently flaky test: #56753

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2025
@nodejs-github-bot
Copy link
Collaborator

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 <[email protected]> (@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 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56712
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56712
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
--------------------------------------------------------------------------------
   ℹ  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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 3531464810] test: simplify common/index.js
Author: James M Snell <[email protected]>
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...

  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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 4f6ab03b3b] test: rely less on duplicative common test harness utilities
Author: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 98eb0492e7] test: make common/index slightly less node.js specific
Author: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 6e4c511660] test: replace more uses of global with globalThis
Author: James M Snell <[email protected]>
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

@lpinca lpinca added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 25, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in 4a5d2c7...97a3a82

nodejs-github-bot pushed a commit that referenced this pull request Jan 25, 2025
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jan 25, 2025
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jan 25, 2025
* 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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jan 25, 2025
PR-URL: #56712
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants