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

Fixes runtime exception when comparing inequal lists with over 100 elements #1031

Merged
merged 2 commits into from Nov 13, 2019
Merged

Fixes runtime exception when comparing inequal lists with over 100 elements #1031

merged 2 commits into from Nov 13, 2019

Conversation

robinheghan
Copy link
Contributor

This has the same solution as #1018, but uses fuzz testing instead of unit tests. This PR is also not conflated with upgrading the testing code for 0.19.

@jerith666
Copy link
Contributor

Sorry for the long delay in responding here, but I finally had a chance to run the tests at this commit (with the 0.19.1 updates from #1059), and they don't pass for me:

elm-test 0.19.1
---------------

Running 1268 tests. To reproduce these results, run: elm-test --fuzz 100 --seed 193469607216503 /home/matt/git/elm/core/tests/tests/Main.elm

↓ Test.Equality
↓ Equality Tests
↓ List equality
✗ Simple comparison

    This test failed because it threw an exception: "TypeError: Cannot read property '$' of undefined"



TEST RUN FAILED

Duration: 65658 ms
Passed:   1267
Failed:   1

@jerith666
Copy link
Contributor

jerith666 commented Oct 26, 2019

I also tried running the tests added by your 3520419 commit with the fix from my 6eff09b commit, and the tests pass.

@jerith666
Copy link
Contributor

Finally, I also tried running the tests added by my 6eff09b against the fix from your 3520419; the tests also fail in that case.

@jerith666
Copy link
Contributor

I think what this all means is that it's the typeof x !== 'object' || x === null || y === null check that prevents Tuple2s containing undefined from being pushed onto the stack, not the x === y check.

@robinheghan
Copy link
Contributor Author

Hmm. So the typeof check should be moved up to before the depth check?

@jerith666
Copy link
Contributor

I think so, yes. My fix moves both the x === y and the typeof ... checks above the depth check.

I should say that I don't have any deep knowledge of how Elm objects get represented as JS objects that's informing me here, I'm just going by what I saw in the browser debugger when I was exploring the original failures from the SSCCE I posted in #1011. That's maybe why I got a bit more elaborate in my test cases than you, as well?

I'm happy to open a new PR with my fix rebased onto the current tip of master if that helps.

@jerith666
Copy link
Contributor

Yep, that leaves Utils.js identical to what I had in #1018, so it should work!

@harrysarson
Copy link
Contributor

Should we add the extra tests to prevent regressions?

@jerith666
Copy link
Contributor

I'm always a fan of more tests -- esp. for something as critical as elm/core! :)

I can make a PR with the combined set of tests if you'd like.

jerith666 added a commit to jerith666/core that referenced this pull request Nov 10, 2019
do non-recursive tests before the depth check.  this ensures that the
'for(key in x)' loop properly short-circuits, and avoids putting
Tuple2s containing 'undefined' on the stack.

fixes elm#1011; see that issue and elm#1031 for further discussion
jerith666 added a commit to jerith666/core that referenced this pull request Nov 10, 2019
do non-recursive tests before the depth check.  this ensures that the
'for(key in x)' loop properly short-circuits, and avoids putting
Tuple2s containing 'undefined' on the stack.

fixes elm#1011; see that issue and elm#1031 for further discussion

listTests by @Skinney
@jerith666
Copy link
Contributor

My new PR is #1062

@evancz evancz merged commit d95e1f3 into elm:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants