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

Comprehensive rewrite of List tests #150

Merged
merged 2 commits into from Feb 1, 2015

Conversation

Projects
None yet
3 participants
@jonathanhefner
Contributor

jonathanhefner commented Jan 29, 2015

I rewrote the List tests to be more comprehensive. The tests now cover all exposed API functions, with the exception of map3..5 (which were also previously not covered). I can add coverage for map3..5 as well as non-exposed functions, if desired.

The tests are also now parameterized, so that empty list, 1-element list, and N-element list use-cases are all covered equally. This also enables easy stress testing, either for stack overflow checking or for benchmarking.

I tried to keep the code as clean and clear as possible. If there are any parts that need improvement, please let me know.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Feb 1, 2015

Member

Very cool, thank you!

My main concern is that if reverse is broken, lots of tests will behave in odd ways. That feels weird to me.

Member

evancz commented Feb 1, 2015

Very cool, thank you!

My main concern is that if reverse is broken, lots of tests will behave in odd ways. That feels weird to me.

evancz pushed a commit that referenced this pull request Feb 1, 2015

@evancz evancz merged commit ced7e63 into elm:master Feb 1, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Feb 1, 2015

Contributor

Regarding the reverse concern, wouldn't the obvious solution be to replace the reverse imported from List by a local, guaranteed-to-be-correct implementation of reverse in these lines:

https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L111
https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L157
https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L161-162
https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L166-167

?

Contributor

jvoigtlaender commented Feb 1, 2015

Regarding the reverse concern, wouldn't the obvious solution be to replace the reverse imported from List by a local, guaranteed-to-be-correct implementation of reverse in these lines:

https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L111
https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L157
https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L161-162
https://github.com/elm-lang/core/blob/master/tests/Test/List.elm#L166-167

?

@jonathanhefner

This comment has been minimized.

Show comment
Hide comment
@jonathanhefner

jonathanhefner Feb 1, 2015

Contributor

I agree, it was a concern of mine. The problem is (without TCO) you have to rely on either reverse or map (or their underpinnings: foldl and foldr, respectively). Basically, there is always some non-primitive function you are relying on. But it may be worthwhile to remove as many layers as possible, e.g. limit yourself to just foldl, so I'll make another pass at it soon.

Contributor

jonathanhefner commented Feb 1, 2015

I agree, it was a concern of mine. The problem is (without TCO) you have to rely on either reverse or map (or their underpinnings: foldl and foldr, respectively). Basically, there is always some non-primitive function you are relying on. But it may be worthwhile to remove as many layers as possible, e.g. limit yourself to just foldl, so I'll make another pass at it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment