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: deepEqual inconsistencies #14441

Closed
BridgeAR opened this issue Jul 24, 2017 · 3 comments
Closed

assert: deepEqual inconsistencies #14441

BridgeAR opened this issue Jul 24, 2017 · 3 comments
Labels
assert Issues and PRs related to the assert subsystem.

Comments

@BridgeAR
Copy link
Member

BridgeAR commented Jul 24, 2017

  • Version: 8.1.0 - 8.x.x
  • Subsystem: assert

Seems like #13318 introduced a regression where deep equal objects are now failing.

Two small test cases:

// First
// (This worked with 8.0.0 and before)
const a = {}
const b = {}
a.a = a
b.a = {}
b.a.a = a
assert.deepEqual(b, a) // ok
assert.deepEqual(a, b) // fails

// Second
// (This lead to a RangeError before the mentioned PR)
const a = new Set()
const b = new Set()
const c = new Set()
a.add(a)
b.add(b)
c.add(a)
assert.deepEqual(c, b) // ok
assert.deepEqual(b, c) // fails

I am not sure if I find the time to look into it in the next few days, so I opened the issue about it.

@mscdex mscdex added assert Issues and PRs related to the assert subsystem. v8.x labels Jul 24, 2017
@XadillaX
Copy link
Contributor

What's the expected result?

@Trott
Copy link
Member

Trott commented Jul 25, 2017

What's the expected result?

@XadillaX In the first case, the expected result is probably that no AssertionError is thrown (which is the behavior in 8.0.0), and certainly that the result of assert.deepEqual(a, b) is the same as assert.deepEqual(b, a).

In the second case, I think the behavior of both 8.0.0 ("Maximum call stack size exceeded") and current master branch (assert.deepEqual(c, b) is fine but assert.deepEqual(b, c) throws) are undesirable. It seems to me like no error should be thrown at all.

Trott added a commit to Trott/io.js that referenced this issue Jul 25, 2017
@Trott
Copy link
Member

Trott commented Jul 25, 2017

Proposed known-issue test for this in #14488

BridgeAR added a commit to BridgeAR/node that referenced this issue Jul 28, 2017
tniessen pushed a commit to tniessen/node that referenced this issue Aug 1, 2017
PR-URL: nodejs#14491
Fixes: nodejs#14441
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
jasnell pushed a commit that referenced this issue Sep 20, 2017
PR-URL: #14491
Fixes: #14441
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
jasnell pushed a commit that referenced this issue Sep 25, 2017
PR-URL: #14491
Fixes: #14441
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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.
Projects
None yet
Development

No branches or pull requests

4 participants