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: fix deepEqual always return true on URL #50853

Merged
merged 3 commits into from
Oct 13, 2024

Conversation

meixg
Copy link
Member

@meixg meixg commented Nov 22, 2023

Fixes: #50836

@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 Nov 22, 2023
@tniessen tniessen added assert Issues and PRs related to the assert subsystem. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Nov 22, 2023
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

LGTM

@aduh95
Copy link
Contributor

aduh95 commented Nov 22, 2023

Related test failure:

=== release test-whatwg-url-custom-inspect ===
Path: parallel/test-whatwg-url-custom-inspect
Error: --- stderr ---
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '{ a: ***host.name:8080/path/name/?que=ry#hash }'
- '{ a: URL {} }'
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-whatwg-url-custom-inspect.js:65:8)
    at Module._compile (node:internal/modules/cjs/loader:1375:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1434:10)
    at Module.load (node:internal/modules/cjs/loader:1206:32)
    at Module._load (node:internal/modules/cjs/loader:1022:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '{ a: ***host.name:8080/path/name/?que=ry#hash }',
  expected: '{ a: URL {} }',
  operator: 'strictEqual'
}

@meixg
Copy link
Member Author

meixg commented Nov 23, 2023

Related test failure:

=== release test-whatwg-url-custom-inspect ===
Path: parallel/test-whatwg-url-custom-inspect
Error: --- stderr ---
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '{ a: ***host.name:8080/path/name/?que=ry#hash }'
- '{ a: URL {} }'
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-whatwg-url-custom-inspect.js:65:8)
    at Module._compile (node:internal/modules/cjs/loader:1375:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1434:10)
    at Module.load (node:internal/modules/cjs/loader:1206:32)
    at Module._load (node:internal/modules/cjs/loader:1022:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '{ a: ***host.name:8080/path/name/?que=ry#hash }',
  expected: '{ a: URL {} }',
  operator: 'strictEqual'
}

fixed

@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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.

The detection is not checking for an actual instance of URL and it's just a heuristic which can trigger for lots of other objects as well. Assert has to be very strict about knowing what to check for. We would need a proper check for being an URL object and that's something we can not easily do.

Update: The check is actually just additive, so it should be ok in assert. I would still like to see if we can just rely upon changing URL instead of assert and util.

lib/internal/util/inspect.js Show resolved Hide resolved
@theoludwig
Copy link
Contributor

What is the status of this PR? Is there any way, can anyone help to move this forward? 😄

@MCprotein
Copy link
Contributor

@meixg hi, What is the current status of this PR?

@RedYetiDev RedYetiDev added the stalled Issues and PRs that are stalled. label Aug 9, 2024
Copy link
Contributor

github-actions bot commented Aug 9, 2024

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@RedYetiDev
Copy link
Member

Hi! People appear to have been requesting updates since March. For that reason, I've marked this PR as stalled. When action is taken, I'll be happy to un-mark it :-).

@RedYetiDev RedYetiDev removed the stalled Issues and PRs that are stalled. label Oct 1, 2024
@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Oct 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (98788da) to head (722bdd8).
Report is 366 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #50853      +/-   ##
==========================================
+ Coverage   87.89%   88.41%   +0.52%     
==========================================
  Files         652      652              
  Lines      186589   186792     +203     
  Branches    35750    36052     +302     
==========================================
+ Hits       163999   165149    +1150     
+ Misses      15816    14900     -916     
+ Partials     6774     6743      -31     
Files with missing lines Coverage Δ
lib/internal/util/comparisons.js 100.00% <100.00%> (ø)
lib/internal/util/inspect.js 99.95% <100.00%> (-0.05%) ⬇️

... and 130 files with indirect coverage changes

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2024
@nodejs-github-bot
Copy link
Collaborator

@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2024
@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 Oct 13, 2024
@BridgeAR
Copy link
Member

@meixg thank you for following up and sticking to it!

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@meixg meixg added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 13, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2024
@nodejs-github-bot nodejs-github-bot merged commit ac49b20 into nodejs:main Oct 13, 2024
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ac49b20

@meixg meixg deleted the fix-deep-queal branch October 13, 2024 14:02
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Oct 14, 2024
PR-URL: nodejs#50853
Fixes: nodejs#50836
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
jaydenseric added a commit to jaydenseric/find-unused-exports that referenced this pull request Oct 15, 2024
Fixes #4 .

Note that some newer supported Node.js versions have a regression where `deepStrictEquals` fails to compare the URL HREF, but a fix has recently been merged:

- nodejs/node#50836
- nodejs/node#50853 (comment)

Until the fix has been published in new Node.js releases, we can rely on the GitHub Actions CI workflow testing with Node.js v18 which doesn’t have the regression.
aduh95 pushed a commit that referenced this pull request Oct 19, 2024
PR-URL: #50853
Fixes: #50836
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 24, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#50853
Fixes: nodejs#50836
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#50853
Fixes: nodejs#50836
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@ruyadorno
Copy link
Member

This commit does not land cleanly on v22.x-staging and will need manual backport in case we want it in v22.x.

@ruyadorno ruyadorno added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Nov 27, 2024
aduh95 pushed a commit that referenced this pull request Nov 27, 2024
PR-URL: #50853
Fixes: #50836
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@aduh95 aduh95 added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Nov 27, 2024
ruyadorno pushed a commit that referenced this pull request Nov 27, 2024
PR-URL: #50853
Fixes: #50836
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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. backported-to-v22.x PRs backported to the v22.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. util Issues and PRs related to the built-in util module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Node v20] assert.deepEqual doesn't detect two different URLs