-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
travis: Build all stubs, and test all stubs except for untestable ones #464
Conversation
@@ -6,5 +6,6 @@ with `Eq` and `Show` instances, and implement the following functions: | |||
- `allergies` | |||
- `isAllergicTo` | |||
|
|||
Your can use the provided signatures if you are unsure about the types, but | |||
don't let them restrict your creativity. | |||
You will find a dummy data declaration and type signatures already in place, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is too different from the original ("Don't let them restrict your creativity"), please help me to combine the two in a sensible manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is great the way it is. 👍
@@ -12,5 +12,6 @@ The function `fromInteger`, from `Num`, must convert minutes | |||
to 24 hour clock time. It is not necessary to have a sensible | |||
implementation of `abs` or `signum`. | |||
|
|||
Your can use the provided signatures if you are unsure about the types, but | |||
don't let them restrict your creativity. | |||
You will find a dummy data declaration and type signatures already in place, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is too different from the original ("Don't let them restrict your creativity"), please help me to combine the two in a sensible manner.
@@ -0,0 +1,2 @@ | |||
No type signatures on functions (TODO: Should we have them?), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you believe we should have them, I will gladly add them and remove this DONT-TEST-STUB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason to not have them. 👍
But we can also fix that later.
@@ -1,3 +1,6 @@ | |||
module Sublist (Sublist(..), sublist) where | |||
|
|||
data Sublist = Equal | Sublist | Superlist | Unequal deriving (Eq, Show) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially controversial. If we don't do this though, we have to DONT-TEST-STUB
Sublist. I think it's reasonable to do it because it's not the student really has a choice on how to do this, they'll just read the test file and do it this way.
compare to:
go-counting
where we definedata Color = Black | White
connect
where we definedata Mark = Cross | Nought
bowling
where we definedata BowlingError = IncompleteGame | InvalidRoll { rollIndex :: Int, rollValue :: Int }
robot-simulator
where we definedata Bearing = North | East | South | West
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
I'm usually in favor of anything to avoid the students having to read the test file.
Haskell is already hard enough to learn, and it is better to allow the students to see a lot of data declarations until they get used to it than let them stuck reading the test suites.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
Otherwise, there are ambiguous type constraints regarding the output of `largestProduct` and `Eq`. As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
As part of the work in #421.
In #421 we express a desire that all our stubs should compile. Not all of them can successfully run their tests, so we will allow a file that says not to run the tests. This file will be stored in the .meta directory. Support for this was added in Trackler in exercism/DEPRECATED.trackler#4 - in particular, it will not be served to students. Regardless, all stubs should build, ensuring that we have not made a syntax error! If we desire, we can evaluate from time to time whether it is legitimate for the exercises declaring DONT-TEST-STUB to remain that way. We don't use `--pedantic` since there are many cases where, say, a `Dummy` constructor is not used or a function argument is not used. Closes #421
@@ -53,6 +53,27 @@ script: | |||
exercisename=$(basename "$exercise") | |||
pushd ${exercise} | |||
|
|||
examplename="stub" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines (stub-testing code) have very high duplication with the example-testing code below. I'm not sure if it's desirable that we deduplicate that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine for me. If we change our mind we can rewrite it later. 👍
Highly likely to increase our build times. Reviewers should check to make sure the increased time is worth the increased assurance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we added hlint to Travis, the build times and cache size where:
Building time (without cache)
nightly-2016-07-17 : 21m03s
nightly : 21m09s
Rebuild time (with cache)
nightly-2016-07-17 : 1m41s
nightly : 2m03s
Now we have:
Build: 25m52s / 25m12s
Rebuild: 2m21s / 3m3s
Cache: 1651.95MB
So I think we are fine! 😄
Thanks for all the work, @petertseng.
@@ -0,0 +1,2 @@ | |||
No type signatures on functions (TODO: Should we have them?), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason to not have them. 👍
But we can also fix that later.
@@ -1,3 +1,6 @@ | |||
module Sublist (Sublist(..), sublist) where | |||
|
|||
data Sublist = Equal | Sublist | Superlist | Unequal deriving (Eq, Show) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
I'm usually in favor of anything to avoid the students having to read the test file.
Haskell is already hard enough to learn, and it is better to allow the students to see a lot of data declarations until they get used to it than let them stuck reading the test suites.
@@ -53,6 +53,27 @@ script: | |||
exercisename=$(basename "$exercise") | |||
pushd ${exercise} | |||
|
|||
examplename="stub" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine for me. If we change our mind we can rewrite it later. 👍
@@ -6,5 +6,6 @@ with `Eq` and `Show` instances, and implement the following functions: | |||
- `allergies` | |||
- `isAllergicTo` | |||
|
|||
Your can use the provided signatures if you are unsure about the types, but | |||
don't let them restrict your creativity. | |||
You will find a dummy data declaration and type signatures already in place, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is great the way it is. 👍
Looking over it one last time, and if I don't see anything I'm merging it. |
Clock is the last remaining exercise that has the .meta/DONT-TEST-STUB file because the type in the src/Clock.hs stub is not compatible with the test suite. Other examples of exercises that were revised so that testing the stub became possible are listed in #466. The difference is that those all had `data` definitions, whereas Clock is ideally solved with a 'newtype' and an appropriate alias. The DONT-TEST-STUB mechanism was added by @petertseng in #464 and in particular the commits - 1d91ceb (Travis code) - 342b932 (Clock-specific) The change made in this commit argues that since the example solution has `deriving Eq`, adding this to the stub *will* make the exercise a bit easier, but it still won't give away the central piece. The motivation behind removing DONT-TEST-STUB in Clock is that with this single effort, we can say that all stubs can be compiled and tested. To motivate further, I will cite @rbasso in #466: > I think that this is the way to go. We have to sacrifice a little > flexibility in designing exercises, but we gain a more coherent > user-experience and automatic testing of stubs. 👍 This bumps the exercise's version from 2.4.0.9 to 2.4.0.10.
Closes #421
Now all stubs work with
stack build
in Travis, and all stubs except for a few exceptions work withstack test --no-run-tests
, which compiles with the tests.We use the .meta directory of exercism/DEPRECATED.trackler#4 to store a DONT-TEST-STUB marker. This entire directory is not served to students.
This PR makes the assumption: We want all the stubs to build.
We can relax this assumption, and not even run
stack build
on selected stubs, by adding a.meta/DONT-BUILD-STUB
file, if we desire. If you believe any change in this PR was made to make the stub build but should not have been made, please state the case, and we could add.meta/DONT-BUILD-STUB
.This PR makes the acknowledgement that it would be ideal if we can get all the stubs to run the first test and then fail. It then also acknowleges that we couldn't get all stubs to meet this goal at once (in fact, there are some stubs for which we might never want this goal to be met?!), but lets us track how close/far we are from this goal by inspecting how many
DONT-TEST-STUB
files we have.