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

We should probably enforce in Travis that our stub solutions compile #421

Closed
petertseng opened this issue Nov 7, 2016 · 8 comments · Fixed by #464
Closed

We should probably enforce in Travis that our stub solutions compile #421

petertseng opened this issue Nov 7, 2016 · 8 comments · Fixed by #464

Comments

@petertseng
Copy link
Member

In #412 I made a type change to the tests + example solution but forgot to change the stub solution.

In #420 we discuss that we should keep packages in package.yaml if the stub solution needs them.

It would be good if we had a way to automatically check this. I think this means we should probably check that the stub solution compiles in Travis.

Are there any stub solutions that we expect not to compile? any other concerns with this approach? (It will increase the time taken, probably, but perhaps it is worth it. We will see when we do it)

@rbasso
Copy link
Contributor

rbasso commented Nov 7, 2016

I think that, ideally, the stub solution would compile and fail at the first test, but some problems have incomplete stub solutions (maybe because implementing a data type is part of the exercise), and cannot compile.

We could try to fix those exercises using incomplete data types, but I'm not sure if that's possible in all cases.

What we are facing in this issue is a fundamental incompatibility between test-driven development and "statically-typed languages". There is no way to run the tests before everything type-checks.

We had a lot of work trying to make test suites that accept multiple types to mitigate the fact that, in Haskell, the types usually come before writing the tests.

If we can "fix" each exercise to compile without ruining the challenge, then I think it would be great to test the stub solutions in Travis.

@rbasso
Copy link
Contributor

rbasso commented Nov 7, 2016

One way to improve in this direction is to first move all exercises to the new proposed format. Removing the examples from the src folder, it would be even easier to test (locally or in Travis) if the stub solution compiles.

@petertseng
Copy link
Member Author

petertseng commented Nov 8, 2016

All right, now that the examples are moved out of the way, let's try to think about how checking all stubs might be done. We'll take allergies exercise as an example, since that is the alphabetically first that has the problem.

The stub solution gives:

module Allergies (Allergen(..), allergies, isAllergicTo) where

allergies :: Int -> [Allergen]
allergies = undefined

isAllergicTo :: Allergen -> Int -> Bool
isAllergicTo = undefined

And the problem is simply that Allergen isn't there.

So what can we do about this?

  • Put in the stub solution something like data Allergen = Dummy
    • It is a very simple solution, and doesn't give away anything.
    • Potential risk: What if a student doesn't realize it is allowed to change data to type or newtype and suddenly we see less variety in solutions because everyone blindly uses the choice we handed to them?
  • For now, we add it to a list of stub solutions that we expect to fail to build, and we just test everything else.
    • Can easily be done now without taking any further decision
    • ... but we probably have to make a decision later.
    • I believe this affects 15/74 of our exercises right now, that fail to build: allergies bank-account binary-search-tree clock custom-set forth grade-school linked-list matrix meetup robot-name robot-simulator simple-linked-list sublist zipper
  • Instead of testing the stub directly, just have a fail example (as proposed in Allow marking an example as should-fail #397 (comment)) that is identical to the stub, but has the extra data Allergen = Dummy line that lets it compile
    • it's a bit annoying to have the duplication though...
  • Set up something that, before running stack build for an exercise that exports a type, inserts data ThatType = Dummy
    • Requires a bit of work, and is another hurdle for anyone who wants to contribute an exercise.
    • Unless we make something fully automatic that reads the export list. Is there a way to do that?

@rbasso
Copy link
Contributor

rbasso commented Nov 8, 2016

Except for the first alternative, all the other introduce complexity and maintenance, which I think is against your original goal of this PR.

The biggest problem with the first option is that it doesn't work for all exercises.

But we can change the exercises ❗

46/74 exercises fail when built with --pedantic, which is expected.
(15 stub + 5 test)=20/74 exercises fail when built without --pedantic.

Here is the list of the problematic ones:

  • allergies
  • bank-account
  • binary-search-tree
  • clock
  • custom-set
  • forth
  • grade-school
  • largest-series-product
  • linked-list
  • matrix
  • meetup
  • pythagorean-triplet
  • secret-handshake
  • series
  • robot-name
  • robot-simulator
  • simple-linked-list
  • space-age
  • sublist
  • zipper

Edit: Five of the above exercises compile the stub, but do not compile the test suite. It would be desirable to check that the test suite can be compiled too, because this proves that the stub solution is somehow correct and can be used by the students.

The cause of our headache here is that some test suite depends on the data structures. Maybe it was not a good design choice in the fist place to expose data structures to the test suite, because this usually hinders improvements in design.

Functions that return a custom data type are OK. Even when it is expected that the data type has some instances needed by the test suite. What is problematic is exposing data constructors.

Before accepting that we need stubs that don't compile, I would like to spend some time rewriting the inconvenient exercises to see if it is worthy to allow stubs that don't build.

@rbasso
Copy link
Contributor

rbasso commented Nov 8, 2016

Potential risk: What if a student doesn't realize it is allowed to change data to type or newtype and suddenly we see less variety in solutions because everyone blindly uses the choice we handed to them?

It's always bad to induce a kind of solution, but having a stub solution that doesn't compile is also annoying.

Hummm...If the student is able to use data I'm happy with that, because going to newtype and type is easy and people will probably learn by looking at other solutions.

We can explain that in the HINTS.md file.

@rbasso
Copy link
Contributor

rbasso commented Nov 8, 2016

Oh! Sorry @petertseng. I didn't saw that you had already made the list of stubs that cannot build in a previous commit. I added more 5 that can build but cannot build the test suite.

@petertseng
Copy link
Member Author

petertseng commented Nov 8, 2016

I didn't saw that you had already made the list of stubs that cannot build in a previous commit.

It is partially my fault since I edited it into my comment, so it wouldn't have gotten emailed (I am saying this with the assumption that you read the email first, but I only assume that because.. that's what I do!)

That's why sometimes if I want to edit, I will first make it a separate comment, then edit, then delete the separate comment, so that my edit does get emailed.

@rbasso
Copy link
Contributor

rbasso commented Nov 8, 2016

Smart trick. Nice to know that! 😄

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

Successfully merging a pull request may close this issue.

2 participants