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

assert,util: change WeakMap and WeakSet comparison handling #53495

Closed
wants to merge 1 commit into from

Conversation

synapse
Copy link
Contributor

@synapse synapse commented Jun 18, 2024

Fixes #18227

As discussed in the task I've added :

  • a check if the compared params are either WeakSet or WeakMap and compare their instance
  • I slightly updated the doc to reflect this change
  • added tests for Weak* instances comparison

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jun 18, 2024
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thank you for picking this up!
Time has passed and I agree by now that this behavior is better.

+1 with my suggestions

lib/internal/util/comparisons.js Outdated Show resolved Hide resolved
lib/internal/util/comparisons.js Outdated Show resolved Hide resolved
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

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM % comments

test/parallel/test-assert-deep.js Outdated Show resolved Hide resolved
lib/internal/util/comparisons.js Outdated Show resolved Hide resolved
test/parallel/test-assert-deep.js Outdated Show resolved Hide resolved
test/parallel/test-assert-deep.js Outdated Show resolved Hide resolved
test/parallel/test-assert-deep.js Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fast following up on the suggestions!

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2024
@BridgeAR
Copy link
Member

@synapse just a small nit that does not have to be followed up upon:

We have a commit message rule about adding the subsystem to each commit. In this specific case it could e.g., look like: assert,util: change WeakMap and WeakSet comparison handling

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 18, 2024
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2024
@tniessen tniessen changed the title Fix: WeakMap and WeakSet comparison handling assert,util: change WeakMap and WeakSet comparison handling Jun 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 19, 2024
@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2024
@mcollina mcollina removed the needs-ci PRs that need a full CI run. label Jun 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jun 26, 2024
@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 Jun 26, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/53495
✔  Done loading data for nodejs/node/pull/53495
----------------------------------- PR info ------------------------------------
Title      assert,util: change WeakMap and WeakSet comparison handling (#53495)
Author     Cristian Barlutiu  (@synapse, first-time contributor)
Branch     synapse:weak-handle -> nodejs:main
Labels     assert, util, semver-major, author ready, commit-queue-rebase
Commits    1
 - assert,util: change WeakMap and WeakSet comparison handling
Committers 1
 - Cristian Barlutiu 
PR-URL: https://github.com/nodejs/node/pull/53495
Reviewed-By: Matteo Collina 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: James M Snell 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Moshe Atlow 
Reviewed-By: Luigi Pinca 
Reviewed-By: Minwoo Jung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53495
Reviewed-By: Matteo Collina 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: James M Snell 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Moshe Atlow 
Reviewed-By: Luigi Pinca 
Reviewed-By: Minwoo Jung 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 18 Jun 2024 06:26:58 GMT
   ✔  Approvals: 7
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53495#pullrequestreview-2125017937
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/53495#pullrequestreview-2125500391
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53495#pullrequestreview-2127119543
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53495#pullrequestreview-2129045696
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53495#pullrequestreview-2129451730
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53495#pullrequestreview-2133314585
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/53495#pullrequestreview-2133810643
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-06-25T13:43:09Z: https://ci.nodejs.org/job/node-test-pull-request/59964/
- Querying data for job/node-test-pull-request/59964/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9682614533

@BridgeAR
Copy link
Member

Does anyone know why fedora always fails? I will start the CI again.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina added 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 Jun 28, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 28, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 9cd9aa8...764b13d

nodejs-github-bot pushed a commit that referenced this pull request Jun 28, 2024
PR-URL: #53495
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert 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. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util.isDeepStrictEqual would be better to return false to compare WeakMap/WeakSet
10 participants