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

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

Closed
regseb opened this issue Nov 21, 2023 · 7 comments · Fixed by #50853
Closed

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

regseb opened this issue Nov 21, 2023 · 7 comments · Fixed by #50853
Labels
confirmed-bug Issues with confirmed bugs.

Comments

@regseb
Copy link
Contributor

regseb commented Nov 21, 2023

Version

v20.9.0

Platform

Linux regseblaptop 6.2.0-36-generic #37~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Mon Oct 9 15:34:04 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

  • create file index.js:

    const assert = require("node:assert/strict");
    
    const url1 = new URL("http://foo1.com/");
    const url2 = new URL("http://foo2.com/");
    assert.deepEqual(url1, url2);
  • node index.js

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior? Why is that the expected behavior?

The index.js script must raise an error because the two URLs are different.

What do you see instead?

No error.

Additional information

  • npx node@18 index.js

    node:assert:125
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
    + actual - expected ... Lines skipped
    
      URL {
        [Symbol(context)]: URLContext {
          hash_start: 4294967295,
          host_end: 15,
          host_start: 7,
    +     href: 'http://foo1.com/',
    -     href: 'http://foo2.com/',
          pathname_start: 15,
    ...
          username_end: 7
        }
      }
        at Object.<anonymous> (/home/regseb/testcase/index.js:5:8)
        at Module._compile (node:internal/modules/cjs/loader:1256:14)
        at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
        at Module.load (node:internal/modules/cjs/loader:1119:32)
        at Module._load (node:internal/modules/cjs/loader:960:12)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:86:12)
        at node:internal/main/run_main_module:23:47 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: URL {
        [Symbol(context)]: URLContext {
          href: 'http://foo1.com/',
          protocol_end: 5,
          username_end: 7,
          host_start: 7,
          host_end: 15,
          pathname_start: 15,
          search_start: 4294967295,
          hash_start: 4294967295,
          port: 4294967295,
          scheme_type: 0
        }
      },
      expected: URL {
        [Symbol(context)]: URLContext {
          href: 'http://foo2.com/',
          protocol_end: 5,
          username_end: 7,
          host_start: 7,
          host_end: 15,
          pathname_start: 15,
          search_start: 4294967295,
          hash_start: 4294967295,
          port: 4294967295,
          scheme_type: 0
        }
      },
      operator: 'deepStrictEqual'
    }
    
    Node.js v18.18.2
    
@marco-ippolito

This comment was marked as resolved.

@marco-ippolito marco-ippolito changed the title [Node v20] assert.deepEqual doesn't detect two different URLs [Node v18] assert.deepEqual doesn't detect two different URLs Nov 21, 2023
@regseb
Copy link
Contributor Author

regseb commented Nov 21, 2023

The two URLs are different.

  • With Node v18, assert.deepEqual shows that there is a difference.
  • With Node v20, assert.deepEqual doesn't return an error, although the URLs aren't equal.

It works in v18 and the bug is in v20.

@marco-ippolito marco-ippolito changed the title [Node v18] assert.deepEqual doesn't detect two different URLs [Node v20] assert.deepEqual doesn't detect two different URLs Nov 21, 2023
@marco-ippolito marco-ippolito added the confirmed-bug Issues with confirmed bugs. label Nov 21, 2023
@marco-ippolito
Copy link
Member

cc @anonrig

@BridgeAR
Copy link
Member

The difference is not about assert. It's about URL changing the properties of the object. In Node.js 18 the symbol was enumerable and therefore it was compared by assert. But the URL object changed in the meanwhile.

@climba03003
Copy link
Contributor

I believe it is closely related to #46904 which changing the symbol to private property.
assert.deepEqual only compare the enumerable properties, so it no longer able to check that symbol property.

Will it be acceptable to check like Date in here, and create an dedicated checking for URL object?

} else if (isDate(val1)) {
if (!isDate(val2) ||
DatePrototypeGetTime(val1) !== DatePrototypeGetTime(val2)) {
return false;
}
} else if (isRegExp(val1)) {
if (!isRegExp(val2) || !areSimilarRegExps(val1, val2)) {
return false;
}
} else if (isArrayBufferView(val1)) {
if (TypedArrayPrototypeGetSymbolToStringTag(val1) !==
TypedArrayPrototypeGetSymbolToStringTag(val2)) {
return false;
}
if (!strict && (isFloat32Array(val1) || isFloat64Array(val1))) {
if (!areSimilarFloatArrays(val1, val2)) {
return false;
}
} else if (!areSimilarTypedArrays(val1, val2)) {
return false;
}

@regseb
Copy link
Contributor Author

regseb commented Mar 7, 2024

The util.isDeepStrictEqual(val1, val2) method has the same problem (probably because it uses the same code).

const util = require("node:util");

const url1 = new URL("http://foo1.com/");
const url2 = new URL("http://foo2.com/");
console.log(util.isDeepStrictEqual(url1, url2));
// true

@regseb
Copy link
Contributor Author

regseb commented Oct 1, 2024

FYI There's the same problem with Promise. #55198 (comment)

import assert from 'node:assert/strict';

const promise1 = Promise.resolve("foo");
const promise2 = Promise.resolve("bar");
assert.deepEqual(promise1, promise2);

marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue 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 issue 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 issue 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]>
louwers pushed a commit to louwers/node that referenced this issue 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 issue 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]>
aduh95 pushed a commit that referenced this issue 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]>
ruyadorno pushed a commit that referenced this issue 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
confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants