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

Add 'MonadFail' instance for 'PropertyT' #267

Merged

Conversation

geigerzaehler
Copy link
Contributor

Add MonadFail instance for PropertyT so users can write the following tests

property $ do
  Just val <- lookupUnderTest
  assertSomeProperties val

The above test will produce a test failure when lookupUnderTest returns Nothing.

We build on the MonadFail instances for TestT provided by #233.

Add `MonadFail` instance for `PropertyT` so users can write the
following tests

    property $ do
      Just val <- lookupUnderTest
      assertSomeProperties val

The above test will produce a test failure when `lookupUnderTest`
returns `Nothing`.

We build on the `MonadFail` instances for `TestT` provided by hedgehogqa#233.
@moodmosaic
Copy link
Member

FWIW, here's why there isn't a MonadFail instance for PropertyT: #233 (comment) /cc @HuwCampbell

@geigerzaehler
Copy link
Contributor Author

FWIW, here's why there isn't a MonadFail instance for PropertyT: #233 (comment) /cc @HuwCampbell

Thanks for the hint. I didn’t see that.

As I understand the comments the instance was not added because it was not clear what the behavior should be. I would make the case that having the test failure behavior makes a lot of sense. It is basically fail = error (which used to be the default) with more information.

It would be great if the original decision would be reconsidered.

@moodmosaic
Copy link
Member

[..]the instance was not added because it was not clear what the behavior should be.

Not quite, I actually think @HuwCampbell makes it clear (in that same thread):

People expect to see MonadFail errors when upgrading to 8.6. It's part and parcel with the upgrade and that's a good thing. We shouldn't add a MonadFail instances just to counter that.

See also nick8325/quickcheck#228 (comment) on a similar discussion in QC space.

@jacobstanley
Copy link
Member

I certainly do have reservations about fail meaning discard for properties, and even a little for generators. I see properties as advanced tests, rather than generators, so I think error is a sensible choice.

@jacobstanley jacobstanley merged commit 3bfe63e into hedgehogqa:master Apr 15, 2019
@geigerzaehler
Copy link
Contributor Author

🎉

@geigerzaehler geigerzaehler deleted the monad-fail-for-property-t branch April 15, 2019 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants