assert: fix deepEqual inconsistencies #14491
Conversation
lib/assert.js
Outdated
There was a problem hiding this comment.
What about sets that contain the undefined value?
There was a problem hiding this comment.
It should say map.has(x) and not set. The only values in the map are numbers as it is a internal value.
There was a problem hiding this comment.
Sorry, my question is: what if actual or expected is the undefined value (I'm not sure it can be)? In that case, map.has(actual) is not the same as map.get(actual) !== undefined.
Edit: forget it, I was really confused by this map/set thing...
|
@nodejs/testing |
|
@nodejs/collaborators This could use some reviews. |
There was a problem hiding this comment.
Can you move the tests in a separate commit to simplifying reviews? Or fire a different PR that moves the tests? I would prefer the latter.
Looking at this PR it is not clear which case is added.
|
@mcollina I tried to do exactly that. The first commit changes the code and adds the regression tests. The second one moves them. The third one is a fixup for commit 1. If you still prefer me to open a second PR as follow up, I'll do that. |
test/parallel/test-assert-deep.js
Outdated
There was a problem hiding this comment.
Can you please add a link to the issues that originated this bug?
|
@BridgeAR one is fine then, but when landing we should keep them separate. I saw 3 commits vs 2 that I was expecting. Can you squash the last one in the first? (the comment fixup) |
4078eeb to
55ac919
Compare
|
@mcollina done |
There was a problem hiding this comment.
Subsystem of the second commit should be `test´ IMO.
55ac919 to
b7ecf4d
Compare
|
@tniessen you are absolutely correct. Fixed |
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>
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>
|
Hi! 😄 Once again, this doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR. Fwiw, only the |
|
@addaleax I guess it is not be necessary to backport these as those tests are probably never touched |
|
Great – in that case the label can be removed and this should be picked up by the tooling automatically again :) |
|
Should this be backported to |
|
This should not be backported because the issue does not exist in v6. |
| // We prevent up to two map.has(x) calls by directly retrieving the value | ||
| // and checking for undefined. The map can only contain numbers, so it is | ||
| // safe to check for undefined only. | ||
| const expectedMemoA = memos.actual.get(actual); |
There was a problem hiding this comment.
Hey @BridgeAR, old PR, I know, but I am trying to understand the implementation in latest master and am confused by this code. Why is this variable called expectedMemoA, if it comes from the actual Map? Shouldn't it be called actualMemo?
Fixes #14441
I moved some tests over to assert-deep as they now always test all variations and it makes more sense to keep the deep-equal tests in there anyway.
Note: the original test added in #13318 was actually faulty.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
assert