Diamond: Implement property-based tests

[chiroptical]

Here is the initial implementation for property-based tests for the diamond exercise. For any non-letter character diamond should produce Nothing. I am very open to feedback as I would like to contribute to this project.

To do:

• Read this PR carefully allergies: Property-based tests. #735
• Read this Issue carefully Property-based-tests Diamond. #600

[chiroptical]

Question: Would you prefer I convert the current unit tests to property based ones? I could easily generate a Map Char String which contains all of the results and implement one PBT for all characters.

[sshine]

Would you prefer I convert the current unit tests to property based ones?

I think we should keep the canonical unit tests.

one PBT for all characters

The one test you have added so far is a negative property.

I would probably write the positive properties in an order based on on this comment in #600 or Mark Seemann's blog post.

Just in case you haven't stumbled upon it, choose ('A', 'Z') will give you an arbitrary uppercase letter:

Test.QuickCheck.choose :: Random a => (a, a) -> Gen a

[chiroptical]

I added some of the basic PBTs from Mark Seemann's blog post. I can implement some of the trickier ones if you think it is necessary.

[sshine]

I added some of the basic PBTs from Mark Seemann's blog post.

That is excellent!

I can implement some of the trickier ones if you think it is necessary.

I think you should decide if they may be useful, but most importantly if you want to.

Edit: I've compared the ones you made with the ones in the exercise description, and I think we could go either way of (a) saying they demonstrate property-based testing enough, and place some encouragement to extend the property-based tests, or (b) add more tests, but preferrably leave some out.

---

For fun, I tried to run the tests on the solution

diamond :: Char -> Maybe [String]
diamond c
  | c `elem` ['A'..'Z'] = Just [""]
  | otherwise = Nothing

and got:

Progress 1/2: diamond
diamond
  non-Alpha characters should produce `Nothing`
    +++ OK, passed 100 tests.
  Length of a diamond should be odd
    +++ OK, passed 100 tests.
  Top and bottom of a diamond should be equal
    +++ OK, passed 100 tests.
  Check the middle of the diamond is correct FAILED [1]

Failures:

  test/Tests.hs:28:3: 
  1) diamond Check the middle of the diamond is correct
       Falsifiable (after 1 test):
         'Y'

We could enable shrinking here, too, so we get the least letter for which the property fails.

But more importantly, I don't really understand what failed.

What's the middle supposed to look like?

In making what failed more explicit I have two ideas:

• The cheap solution I used in allergies: Property-based tests. #735: Spell out the property in a comment above.
• Using the counterexample combinator. I lack a good example I can point to.

[sshine]

The complement of your very first (negative) property test may be worthwhile as a second test:

  it "There are diamonds for letter characters" $
    forAll genDiamond isJust

(attempting to stay ambiguous about lowercase letters.)

The properties that are listed in the exercise description are are:

• The first row contains one 'A'.
• The last row contains one 'A'.
• All rows, except the first and last, have exactly two identical letters.
• All rows have as many trailing spaces as leading spaces. (This might be 0).
• The diamond is horizontally symmetric. (Sounds like your "Top and bottom of a diamond should be equal".)
• The diamond is vertically symmetric.
• The diamond has a square shape (width equals height). (Sounds like a weaker but more clearly written variation of your "Check the dimensionality of the diamond is correct", because you don't say what is correct.).
• The letters form a diamond shape.
• The top half has the letters in ascending order.
• The bottom half has the letters in descending order.
• The four corners (containing the spaces) are triangles.

We can defend not making all (or even any) of them, since then instead we can put a hint below the exercise test that encourages the student to implement some of the missing ones. For that reason we should probably omit both some easy ones and some hard ones.

---

You may not have had a chance to notice that the QuickCheck with Hspec tutorial shows an Arbitrary-based approach to expressing properties, rather than the forAll explicit generator-based approach used in #735 and here.

On this and many other details, there's an article that discusses why Arbitrary is preferrable to avoid: https://www.fpcomplete.com/blog/quickcheck-hedgehog-validity -- it also covers the Hedgehog library and shows how property-based testing has moved quite a bit since QuickCheck. We're not using Hedgehog anywhere on this track (yet).

[chiroptical]

@sshine Thanks for the awesome feedback! I will work on this Thursday (7/12/19)

[chiroptical]

@sshine I went through all of your comments and I think this PR is in much better shape. Thanks again for the great comments!

[sshine]

@barrymoo: Thanks for your latest pushes! I'll look at this PR this weekend.

[sshine]

Excellent work.

One suggestion for change: I'd only keep the following combinators at the bottom of the file:

• genNonAlphaChar
• genAlphaChar
• genDiamond
• forAllDiamond
• (forAllCharDiamond)
• shrinkNonAlphaChar

and embed the actual properties right below each description.

I've left suggestions for improvements for all but checkMiddle.

[petertseng]

I think this is good enough to approve. I think the commits must be squashed since there are intermediate states that don't pass tests.

Here are some optional changes to think about that could possibly be done before merging. If a reasonable response is done, there's no need to wait for another approval from me. If no response is had within a reasonable time, ignoring it and merging is fine too.

• Is there a rule for the ordering of imports? I couldn't find any.
• I marked some lines that were only indented by one space, since I hadn't seen it before.
• I don't think the existing imports needed to be reindented. I am adding a commit that changes them back.

Since we don't have enforced rules around style or anything, the changes are optional.

I leave the possibility open that maybe additional tests may be added in the future. For example, if the existing example-based tests were all discounted, perhaps a student finds that it is possible to pass tests with some strange output. But those are all hypothetical, since the example-based tests do exist.

[sshine]

Excellent work, @barrymoo. Thanks a lot for your contribution.

Catch you on the twitch stream. :-)