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 inconsistencies #14491

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
9 participants
@BridgeAR
Copy link
Member

commented Jul 26, 2017

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

@BridgeAR BridgeAR referenced this pull request Jul 26, 2017

Closed

test: add known-issue test for issue 14441 #14488

3 of 3 tasks complete
if (memos.actual.has(actual)) {
return memos.actual.get(actual) === memos.expected.get(expected);
// We prevent up to two set.has(x) calls by directly retrieving the value
// and checking for undefined.

This comment has been minimized.

Copy link
@targos

targos Jul 26, 2017

Member

What about sets that contain the undefined value?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jul 26, 2017

Author Member

It should say map.has(x) and not set. The only values in the map are numbers as it is a internal value.

This comment has been minimized.

Copy link
@targos

targos Jul 26, 2017

Member

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...

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 27, 2017

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 28, 2017

@nodejs/collaborators This could use some reviews.

@mcollina
Copy link
Member

left a 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.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2017

@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.

@@ -303,6 +303,26 @@ assertOnlyDeepEqual(
new Set([undefined])
);

// Circular structures

This comment has been minimized.

Copy link
@mcollina

mcollina Jul 28, 2017

Member

Can you please add a link to the issues that originated this bug?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jul 28, 2017

Author Member

Done

@mcollina

This comment has been minimized.

Copy link
Member

commented Jul 28, 2017

@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)

@BridgeAR BridgeAR force-pushed the BridgeAR:fix-assert-deep-equal branch from 4078eeb to 55ac919 Jul 28, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2017

@mcollina done

@mcollina
Copy link
Member

left a comment

LGTM

@XadillaX
Copy link
Member

left a comment

LGTM if CI is green.

@XadillaX

This comment has been minimized.

@tniessen
Copy link
Member

left a comment

Subsystem of the second commit should be `test´ IMO.

@BridgeAR BridgeAR force-pushed the BridgeAR:fix-assert-deep-equal branch from 55ac919 to b7ecf4d Aug 1, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

@tniessen you are absolutely correct. Fixed

tniessen added a commit to tniessen/node that referenced this pull request Aug 1, 2017

assert: fix deepEqual inconsistencies
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>

tniessen added a commit to tniessen/node that referenced this pull request Aug 1, 2017

test: clean up some assert deepEqual tests
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>
@tniessen

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

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 test: commit is making trouble, if necessary we can just skip it.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

@addaleax I guess it is not be necessary to backport these as those tests are probably never touched

jasnell added a commit that referenced this pull request Sep 20, 2017

assert: fix deepEqual inconsistencies
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>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

@addaleax @jasnell it seems like this patch applies cleanly right now (even the tests). I think it does not make sense to open a PR for that in that case (just going through my PRs with the backporting label).

@addaleax

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

Great – in that case the label can be removed and this should be picked up by the tooling automatically again :)

jasnell added a commit that referenced this pull request Sep 25, 2017

test: clean up some assert deepEqual tests
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>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Oct 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

This should not be backported because the issue does not exist in v6.

@BridgeAR BridgeAR deleted the BridgeAR:fix-assert-deep-equal branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.