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 RangeError: Maximum call stack size exceeded #13318

Closed
wants to merge 2 commits into from

Conversation

rmdm
Copy link
Contributor

@rmdm rmdm commented May 30, 2017

Fixes: #13314
Refs: #6416

Checklist
  • make -j4 test passes
  • tests are included
Affected core subsystem(s)

assert

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 30, 2017
@Fishrock123 Fishrock123 requested a review from addaleax May 30, 2017 21:55
@Fishrock123
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/8377/

I'm a bit confused as to how this fixes it, though.

@cjihrig
Copy link
Contributor

cjihrig commented May 30, 2017

The CI seems to not like this change.

@Trott
Copy link
Member

Trott commented May 30, 2017

/cc @addaleax, @joyeecheung

@rmdm
Copy link
Contributor Author

rmdm commented May 31, 2017

Ouch. Somehow I mistakenly thought that $ python tools/test.py -J --mode=release parallel/test-assert would run all the test files with that prefix. And it's not. Thats my fail, shame on me, sorry...

Btw, it seems like the only failing case is the following:

AssertionError [ERR_ASSERTION]: Set { [ 1, 2 ], [ 3, 4 ] } deepEqual Set { [ 3, 4 ], [ 1, 2 ] }
    at assertDeepAndStrictEqual (/home/d/p/node/test/parallel/test-assert-deep.js:124:10)
    at Object.<anonymous> (/home/d/p/node/test/parallel/test-assert-deep.js:159:1)

And it's strange... will investigate.

lib/assert.js Outdated
if (actualPosition === memos.expected.map.get(expected)) {
return true;
}
return actualPosition === memos.expected.map.get(expected);
Copy link
Member

@joyeecheung joyeecheung May 31, 2017

Choose a reason for hiding this comment

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

I think the failed test case is caused by memos.expected.map.get(expected) returning undefined? To make sure the two are visited in the same order, we need to first make sure expected has actually been visited before, so this should not return false when expected has not been visited (memos.expected.map.get(expected) === undefined).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the point. I tried it and indeed, it makes the case pass. However, at first, it breaks (or changes expectations of the) test added by the PR from "throws" to "not throws", which is probably not a bad thing, but rather a point that requires some discussion. Secondly, I was able to create a case that still fails:

var a = [ 1, 2 ]
var b = [ 3, 4 ]
var c = [ 1, 2 ]
var d = [ 3, 4 ]

assertDeepAndStrictEqual({
  v: a,
  s: new Set([a, b])
}, {
  v: c,
  s: new Set([d, c])
});

This commit changes semantics of the memos cycles tracker. Before
the change it was used to track all currently wisited nodes of an
object tree, which is a bit shifted from its original intention of
tracking cycles. The change brings intended semantics, by tracking
only objects of the current branch of the object tree.

Fixes: nodejs#13314
@rmdm
Copy link
Contributor Author

rmdm commented Jun 1, 2017

I have changed memos semantics from tracking all currently visited nodes of an object tree to tracking visited nodes of only current branch, which brings intended semantics of tracking cycles. After that all assert-related tests passed (though some of non assert-related were failed on my machine even before my changes even on master branch - that's why I used not make test but that python script). Feeling like ready for reviews and probably CI.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

CI failure is unrelated

jasnell pushed a commit that referenced this pull request Jun 2, 2017
Fixes: #13314
Refs: #6416

This commit changes semantics of the memos cycles tracker. Before
the change it was used to track all currently wisited nodes of an
object tree, which is a bit shifted from its original intention of
tracking cycles. The change brings intended semantics, by tracking
only objects of the current branch of the object tree.

PR-URL: #13318
Fixes: #13314
Ref: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

Landed in b1ed55f

@jasnell jasnell closed this Jun 2, 2017
jasnell pushed a commit that referenced this pull request Jun 5, 2017
Fixes: #13314
Refs: #6416

This commit changes semantics of the memos cycles tracker. Before
the change it was used to track all currently wisited nodes of an
object tree, which is a bit shifted from its original intention of
tracking cycles. The change brings intended semantics, by tracking
only objects of the current branch of the object tree.

PR-URL: #13318
Fixes: #13314
Ref: #6416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@Trott
Copy link
Member

Trott commented Jul 18, 2017

Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@MylesBorins Note that people who are not in the Collaborators team cannot add or remove labels. So if a PR is authored by a non-Collaborator, perhaps ask the contributor to comment instead of replacing a label.

@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

Agreed. @MylesBorins it's probably also worth adding the link to the backporting guide

This is my saved reply:

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.

Maybe we should have a stock one.

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

Successfully merging this pull request may close these issues.

assert: RangeError: Maximum call stack size exceeded is still present
9 participants