Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upUpgrade elm-test & test suite #771
Conversation
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
process-bot
Dec 3, 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.
process-bot
commented
Dec 3, 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
changed the title from
WIP: update elm-test & test suite
to
WIP: upgrade elm-test & test suite
Dec 3, 2016
turboMaCk
added some commits
Dec 3, 2016
turboMaCk
changed the title from
WIP: upgrade elm-test & test suite
to
Upgrade elm-test & test suite
Dec 3, 2016
| @@ -63,21 +63,21 @@ customTests = | ||
| "I want to see this message!" | ||
| myDecoder = | ||
| Json.customDecoder ("foo" := Json.string) (\_ -> Err customErrorMessage) | ||
| Json.field "foo" Json.string |> Json.andThen (\_ -> Json.fail customErrorMessage) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
tests/run-tests.sh
| node test.js | ||
| elm-make --yes --output test.js Main.elm | ||
| npm test |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
turboMaCk
Dec 3, 2016
Contributor
I think that this depend caching (elm-test does not overwrite & recompile anything but will try though). That doesn't feels right:-/ Please pay attention to this line & integration with elm-test
turboMaCk
Dec 3, 2016
•
edited
Edited 1 time
-
turboMaCk
edited Dec 3, 2016 (most recent)
Contributor
edited
-
Dec 3, 2016 (most recent)
I think that this depend caching (elm-test does not overwrite & recompile anything but will try though). That doesn't feels right:-/ Please pay attention to this line & integration with elm-test
| , takingArraysApartTests | ||
| , mappingAndFoldingTests | ||
| , nativeTests | ||
| ] |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
turboMaCk
Dec 3, 2016
Contributor
This is the only file formatted by elm-format. Please let me know if you want to keep old formatting.
turboMaCk
Dec 3, 2016
Contributor
This is the only file formatted by elm-format. Please let me know if you want to keep old formatting.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
turboMaCk
Dec 3, 2016
Contributor
I would like to format all tests then. I think it's good time to do so since tests ware broken anyway so there is really low possibility it will make conflicts with any opened PR. What do you think?
turboMaCk
Dec 3, 2016
Contributor
I would like to format all tests then. I think it's good time to do so since tests ware broken anyway so there is really low possibility it will make conflicts with any opened PR. What do you think?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
turboMaCk
Dec 3, 2016
Contributor
- I'm speaking only about tests. Doing same thing in src can cause nasty conflicts I don't want to be associated with :D
turboMaCk
Dec 3, 2016
Contributor
- I'm speaking only about tests. Doing same thing in src can cause nasty conflicts I don't want to be associated with :D
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
rtfeldman
Dec 4, 2016
Member
@turboMaCk If you'd like to make a separate PR that runs elm-format on all tests, I'd be happy to accept it!
rtfeldman
Dec 4, 2016
Member
@turboMaCk If you'd like to make a separate PR that runs elm-format on all tests, I'd be happy to accept it!
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
turboMaCk
Dec 4, 2016
Contributor
@rtfeldman Not super important but here we go. https://github.com/elm-lang/core/pull/774.
turboMaCk
Dec 4, 2016
Contributor
@rtfeldman Not super important but here we go. https://github.com/elm-lang/core/pull/774.
.travis.yml
| @@ -3,6 +3,7 @@ language: node_js | ||
| node_js: | ||
| - "4.3" | ||
| install: | ||
| - npm install -g elm@0.17 | ||
| - npm install -g elm@0.18 | ||
| - npm install |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
rtfeldman
suggested changes
Dec 3, 2016
This is super great @turboMaCk! Thank you so much for doing this!
My only comment is that I don't think package.json makes sense here. I would rather we changed the instructions from "run npm install and then npm test to run the tests" to "run npm install -g elm-test if you haven't already, and then run elm-test to run the tests."
That would also eliminate the npm install step in .travis.yml, which I think is a good thing.
Sound reasonable?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
turboMaCk
Dec 3, 2016
Contributor
@rtfeldman No problem at all. I did it this way only because it makes elm-test version independent on global installation. If it's preferable to use global install I don't see any blocker for switching to that.
|
@rtfeldman No problem at all. I did it this way only because it makes |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
@rtfeldman done in elm-lang@2942be8 |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Looks great to me! |
rtfeldman
merged commit d8df24b
into
elm:master
Dec 3, 2016
1 check passed
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Thank you so much @turboMaCk! |
turboMaCk commentedDec 3, 2016
•
edited
Edited 1 time
-
turboMaCk
edited Dec 4, 2016 (most recent)
Why?
I think it's important to have working tests. Otherwise it's really hard to tell if any PR actually fixes problem and doesn't broke anything else.
How
I was trying to reproduce some issues with array and made repository with test. I think it make sense to give it back to community so we all can benefit from having working tests for core library
Choices Made
I'm using (local) npm installation of elm-test by @rtfeldman which is de facto community standard. I'm also usingnpm testscript as alias to local installation. On the other hand I don't want to make any unnecessary change to current work flow. This is why main script for running tests is almost the same as before (and usingnpm testinternally).*This was changed after code review. We are using global elm-test install. See: https://github.com/elm-lang/core/pull/771#pullrequestreview-11285372
Elm-test version is updated. One other change is that I've moved
tests/Test.elm=>tests/Main.elmwhich is more standard way to do it.Fixing Tests
There are some breaking changes in elm-test. This means tests (files in
tests/Tests) should be updated one by one:This PR do not and will not include any other changes beside what is necessary to make current tests work as they are.