Array slice FIX + TEST #765

turboMaCk · Nov 30, 2016

Replicates issue https://github.com/elm-lang/core/issues/474 in tests
also related to https://github.com/elm-lang/core/issues/754

Array Slice issue

This test should replicate issue described in https://github.com/elm-lang/core/issues/474 and https://github.com/elm-lang/core/issues/754.
~~Anyway it's quite hard to proof this since master build is failing.~~

base: elm-lang@1de91ae

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot · Nov 30, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

turboMaCk · Nov 30, 2016

turboMaCk reviewed Nov 30, 2016

View changes

turboMaCk · Nov 30, 2016

tests/Test/Array.elm

@@ -80,6 +80,11 @@ tests =
        , test "filter" <| assertEqual (Array.fromList [2,4,6]) (Array.filter (\x -> x % 2 == 0) (Array.fromList [1..6]))
        ]

+      sliceArrayTest = suite "Slice Array"
+        [ test "array < 32" <| assertEqual (Array.initialize 31 identity |> Array.slice 31 32) (Array.fromList [])


this test should pass

turboMaCk · Nov 30, 2016

tests/Test/Array.elm

@@ -80,6 +80,11 @@ tests =
        , test "filter" <| assertEqual (Array.fromList [2,4,6]) (Array.filter (\x -> x % 2 == 0) (Array.fromList [1..6]))
        ]

+      sliceArrayTest = suite "Slice Array"
+        [ test "array < 32" <| assertEqual (Array.initialize 31 identity |> Array.slice 31 32) (Array.fromList [])
+        , test "array == 32" <| assertEqual (Array.initialize 32 identity |> Array.slice 32 33) (Array.fromList [])


this test should fail

I'm not able to make it pass in CI for some reason. However this works as expected on my local machine.

turboMaCk · Nov 30, 2016

I'm not able to make it pass in CI for some reason. However this works as expected on my local machine.

It should get elm 0.18, not 0.17.1.

Also, these tests will be broken right? I think it's better to have the fix and the tests that prove the fix in one PR.

evancz · Nov 30, 2016

It should get elm 0.18, not 0.17.1.

Also, these tests will be broken right? I think it's better to have the fix and the tests that prove the fix in one PR.

Hello!

Sorry I was traveling because of the work for past few days.

First let me explain why I did what I did till now. Tests on master are currently failing. I think this make it really harder to reproduce (/describe) any issue and proof possible fix. I looked at commits in master and found latest which passed. I wanted to demonstrate this issue using tests. This is why this PR uses that commit as Base.

~~However I think this will be a bit bigger issue and longer run so this is what I did next:~~

I've created new repository which is using core lib as submodule and build new test suit (0.18.0 !!) around core. I have to admit it was quite challenging itself. However I've managed to do it so this repository https://github.com/turboMaCk/elm-array-tests is ready I think.

~~Next steps will be:~~

Move whole test suite for Array to that repository and make it pass
- turboMaCk/elm-array-tests#1
Add tests for cases for issues with current implementation
- turboMaCk/elm-array-tests#2
Try to fix some of the biggest issues
- Exception without this path: https://travis-ci.org/turboMaCk/elm-array-tests/builds/180989210#L244
- Fixed after checkout to this branch: https://travis-ci.org/turboMaCk/elm-array-tests
- Tests can be found here: turboMaCk/elm-array-tests#2
Based on what we've learn about all of this update tests and in new implementation

~~Hope it make sense. Let me know if you have any objections or ideas.~~

Update: Based on experience with that repo I've managed to upgrade elm-test & all tests directly in here. PR is already merged in master

turboMaCk · Dec 3, 2016

Hello!

Sorry I was traveling because of the work for past few days.

First let me explain why I did what I did till now. Tests on master are currently failing. I think this make it really harder to reproduce (/describe) any issue and proof possible fix. I looked at commits in master and found latest which passed. I wanted to demonstrate this issue using tests. This is why this PR uses that commit as Base.

~~However I think this will be a bit bigger issue and longer run so this is what I did next:~~

I've created new repository which is using core lib as submodule and build new test suit (0.18.0 !!) around core. I have to admit it was quite challenging itself. However I've managed to do it so this repository https://github.com/turboMaCk/elm-array-tests is ready I think.

~~Next steps will be:~~

Move whole test suite for Array to that repository and make it pass
- turboMaCk/elm-array-tests#1
Add tests for cases for issues with current implementation
- turboMaCk/elm-array-tests#2
Try to fix some of the biggest issues
- Exception without this path: https://travis-ci.org/turboMaCk/elm-array-tests/builds/180989210#L244
- Fixed after checkout to this branch: https://travis-ci.org/turboMaCk/elm-array-tests
- Tests can be found here: turboMaCk/elm-array-tests#2
Based on what we've learn about all of this update tests and in new implementation

~~Hope it make sense. Let me know if you have any objections or ideas.~~

Update: Based on experience with that repo I've managed to upgrade elm-test & all tests directly in here. PR is already merged in master

Failing test is this one:
turboMaCk/elm-array-tests@7674f75

turboMaCk · Dec 3, 2016

Failing test is this one:
turboMaCk/elm-array-tests@7674f75

There is already a fix, in this PR -> https://github.com/elm-lang/core/pull/399
I looked into this issue a lot and you can read my breakdown there.
It was closed as a pure elm version was decided to be the better route.

eeue56 · Dec 3, 2016

There is already a fix, in this PR -> https://github.com/elm-lang/core/pull/399
I looked into this issue a lot and you can read my breakdown there.
It was closed as a pure elm version was decided to be the better route.

turboMaCk · Dec 3, 2016

turboMaCk reviewed Dec 3, 2016 • edited

View changes

Issue

during recursion in slice undefined is passed instead of valid argument to length sliceLeft and sliceRight

Solution

For length there is new check added and in case of invalid ref 0 is returned. I think there is no need for more complex code here.

For both sliceLeft and sliceRight when wrong ref is passed it becomes empty since that way I think it keeps behavior consistent and easy to reason about.

Other Changes

I made minor changes while walking through code. For example I made leaks of variables from block obvious since I believe it's good practice to do so especially in code that feels so C like like this one.

Mess

I'm sorry for indentation issues. I'm using Emacs and this is one thing which is really hard to set up. I'll fix it in case you'll want to merge this PR (I don't expect it though since previous fixes had been closed). I also recommend using http://editorconfig.org/ which make life of bad people like me easier. Sorry for that.

turboMaCk · Dec 3, 2016

src/Native/Array.js

@@ -815,6 +834,7 @@ function nodeCopy(a)
 // Returns how many items are in the tree.
 function length(array)
 {
+  if (!array) { return  0; }


part of actual fix.

turboMaCk · Dec 3, 2016

src/Native/Array.js

@@ -459,15 +472,21 @@ function sliceRight(to, a)

 function sliceLeft(from, a)
 {
+  if (!a) {
+    a = empty;
+  }


part of actual fix.

turboMaCk · Dec 3, 2016

src/Native/Array.js

@@ -419,16 +423,25 @@ function slice(from, to, a)

 function sliceRight(to, a)
 {
+  if (!a) {
+    a = empty;
+  }


part of actual fix.

Rebased agains master!

This is runtime exception without fix: https://travis-ci.org/elm-lang/core/builds/181118399#L248

This is proof that this is solution to that problem: https://travis-ci.org/elm-lang/core/builds/181118740#L253

turboMaCk · Dec 4, 2016

Rebased agains master!

This is runtime exception without fix: https://travis-ci.org/elm-lang/core/builds/181118399#L248

This is proof that this is solution to that problem: https://travis-ci.org/elm-lang/core/builds/181118740#L253

Alternative approach

I would also like to mention one more time that there are some alternative PRs which looks worth exploring again:

https://github.com/elm-lang/core/pull/396
https://github.com/elm-lang/core/pull/399

Now when we have tests working we can proof their correctness easily.

I'm 100% for choosing alternative fix when we came to conclusion it's better than mine or closing this in case new implementation is ready to ship. However I think it's good idea to make decision which is the best for users of core library - which is to fix known issue.

turboMaCk · Dec 4, 2016

Alternative approach

I would also like to mention one more time that there are some alternative PRs which looks worth exploring again:

https://github.com/elm-lang/core/pull/396
https://github.com/elm-lang/core/pull/399

Now when we have tests working we can proof their correctness easily.

I'm 100% for choosing alternative fix when we came to conclusion it's better than mine or closing this in case new implementation is ready to ship. However I think it's good idea to make decision which is the best for users of core library - which is to fix known issue.

The latest version of core includes @Skinney's array implementation, so this should be resolved by the rewrite.

evancz · Jul 8, 2017

The latest version of core includes @Skinney's array implementation, so this should be resolved by the rewrite.

turboMaCk changed the title from replicate issue in tests to Array slice issue investigation Nov 30, 2016

turboMaCk referenced this pull request Dec 3, 2016
Open
Issue/array slice #2

turboMaCk referenced this pull request Dec 3, 2016
Merged
Upgrade elm-test & test suite #771
13 of 13 tasks complete

turboMaCk added some commits Dec 4, 2016

Add test for slice

20e5d83

Fix slice issue
- return `0` from `length` when array is undefiend - use `empty` in `sliceRight` & `sliceLeft` when array is undefiend

066b951

turboMaCk changed the title from Array slice issue investigation to Array slice FIX + TEST Dec 4, 2016

turboMaCk referenced this pull request Jan 3, 2017
Closed
Runtime error (TypeError: Cannot read property 'height' of undefined) Array cannot shuffle a poker deck more than once. #789

evancz closed this Jul 8, 2017

elm/core

Array slice FIX + TEST #765

turboMaCk commented Nov 30, 2016 •

edited

Edited 1 time

turboMaCk edited Nov 30, 2016 (most recent)

This comment has been minimized.

process-bot commented Nov 30, 2016

turboMaCk changed the title from replicate issue in tests to Array slice issue investigation Nov 30, 2016

turboMaCk reviewed Nov 30, 2016

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

turboMaCk commented Nov 30, 2016

This comment has been minimized.

evancz commented Nov 30, 2016

This comment has been minimized.

turboMaCk commented Dec 3, 2016 •

edited

Edited 1 time

turboMaCk edited Dec 4, 2016 (most recent)

turboMaCk referenced this pull request Dec 3, 2016

Issue/array slice #2

This comment has been minimized.

turboMaCk commented Dec 3, 2016

This comment has been minimized.

eeue56 commented Dec 3, 2016

turboMaCk referenced this pull request Dec 3, 2016

Upgrade elm-test & test suite #771

turboMaCk reviewed Dec 3, 2016 • edited

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

turboMaCk added some commits Dec 4, 2016

This comment has been minimized.

turboMaCk commented Dec 4, 2016

turboMaCk changed the title from Array slice issue investigation to Array slice FIX + TEST Dec 4, 2016

This comment has been minimized.

turboMaCk commented Dec 4, 2016 •

edited

Edited 1 time

turboMaCk edited Dec 4, 2016 (most recent)

turboMaCk referenced this pull request Jan 3, 2017

Runtime error (TypeError: Cannot read property 'height' of undefined) Array cannot shuffle a poker deck more than once. #789

This comment has been minimized.

evancz commented Jul 8, 2017

evancz closed this Jul 8, 2017

elm/core

Join GitHub today

Array slice FIX + TEST #765

Conversation

turboMaCk commented Nov 30, 2016 • edited Edited 1 time turboMaCk edited Nov 30, 2016 (most recent)

Array Slice issue

This comment has been minimized.

process-bot commented Nov 30, 2016

turboMaCk changed the title from replicate issue in tests to Array slice issue investigation Nov 30, 2016

turboMaCk reviewed Nov 30, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

turboMaCk commented Nov 30, 2016

This comment has been minimized.

evancz commented Nov 30, 2016

This comment has been minimized.

turboMaCk commented Dec 3, 2016 • edited Edited 1 time turboMaCk edited Dec 4, 2016 (most recent)

turboMaCk referenced this pull request Dec 3, 2016

This comment has been minimized.

turboMaCk commented Dec 3, 2016

This comment has been minimized.

eeue56 commented Dec 3, 2016

turboMaCk referenced this pull request Dec 3, 2016

turboMaCk reviewed Dec 3, 2016 • edited View changes

Issue

Solution

Other Changes

Mess

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

turboMaCk added some commits Dec 4, 2016

This comment has been minimized.

Rebased agains master!

turboMaCk commented Dec 4, 2016

Rebased agains master!

turboMaCk changed the title from Array slice issue investigation to Array slice FIX + TEST Dec 4, 2016

This comment has been minimized.

Alternative approach

turboMaCk commented Dec 4, 2016 • edited Edited 1 time turboMaCk edited Dec 4, 2016 (most recent)

Alternative approach

turboMaCk referenced this pull request Jan 3, 2017

This comment has been minimized.

evancz commented Jul 8, 2017

evancz closed this Jul 8, 2017

turboMaCk commented Nov 30, 2016 •

edited

Edited 1 time

turboMaCk edited Nov 30, 2016 (most recent)

turboMaCk reviewed Nov 30, 2016

View changes

turboMaCk commented Dec 3, 2016 •

edited

Edited 1 time

turboMaCk edited Dec 4, 2016 (most recent)

turboMaCk reviewed Dec 3, 2016 • edited

View changes

turboMaCk commented Dec 4, 2016 •

edited

Edited 1 time

turboMaCk edited Dec 4, 2016 (most recent)