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 classifications to properties #253

Merged
merged 2 commits into from Apr 21, 2019

Conversation

@felixmulder
Copy link
Contributor

commented Mar 16, 2019

Was recommended this great talk by John Hughes on labelling properties, thought that would be cool to have in Hedgehog too.

hedgehog/hedgehog.cabal Outdated Show resolved Hide resolved

@felixmulder felixmulder force-pushed the felixmulder:topic/add-classifications branch 6 times, most recently from 2c04184 to 07548d7 Mar 16, 2019

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Tried a bunch of things to work around the bug in GHC 8.2.1. Couldn't get it unborken tonight. How important is 8.2.1 support? 8.2.2 works fine...

@moodmosaic

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Thanks for sending a pull request 👍 Consider adding a [WIP] on the title if it's not finalized (from what I can tell you keep adding commits). Also, considering squashing all those wat commits once you're done.

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@felixmulder felixmulder changed the title Add classifications to properties [WIP] Add classifications to properties Mar 17, 2019

@felixmulder felixmulder force-pushed the felixmulder:topic/add-classifications branch from 188470b to a1c79e5 Mar 17, 2019

@felixmulder felixmulder changed the title [WIP] Add classifications to properties Add classifications to properties Mar 17, 2019

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@moodmosaic - I ended up figuring out which line was offending 8.2.1. I filter it out on 8.2.x series, maybe there's a more precise way to do this, but I can't figure one out.

So tl;dr, ready for review

@moodmosaic
Copy link
Member

left a comment

Thanks for working on this 👍 🚀 I left some high level feedback (and some nitpicks) though I haven't gotten into the implementation details yet. Let's wait for @jystic first.

hedgehog/hedgehog.cabal Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Runner.hs Outdated Show resolved Hide resolved
@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@moodmosaic - I think I addressed your comments.

If this patch is accepted, I'd like to continue work on the feature. Things I'd like to do:

  • Allow turning coverage warning into failure
  • Refactor tuple to data (Journal was a good name 👍)
@jacobstanley
Copy link
Member

left a comment

Sorry for all the style nitpicks, I should really have a style guide. I didn't comment on all the abbreviations but you please find / fix them all. The codebase is pristine at the moment and I want to keep it that way. Generally this looks pretty good though, would love to have this feature!

Fwiw I'm happy to make the style fixups myself in another PR if you can't be bothered.

hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
hedgehog/src/Hedgehog/Internal/Report.hs Outdated Show resolved Hide resolved
@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I'll amend the request in the morning (Stockholm)! Cheers for reviewing!

@felixmulder felixmulder force-pushed the felixmulder:topic/add-classifications branch 5 times, most recently from 8736454 to c001c80 Mar 20, 2019

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@jystic - good morning, that should be all of it. Hope I didn't miss anything 🙏

@felixmulder felixmulder force-pushed the felixmulder:topic/add-classifications branch from c001c80 to 59fc191 Mar 20, 2019

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Squashed the history, no more changes unless you have nitpicks 🙏

@felixmulder felixmulder force-pushed the felixmulder:topic/add-classifications branch 2 times, most recently from 7dbd8f5 to 9e8723d Apr 18, 2019

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@jystic - rebased and ready for your verdict 😃

@jacobstanley
Copy link
Member

left a comment

Just a few items, would also like to make sure @HuwCampbell is happy enough with this before going ahead. I would like to align this with the approach taken in #262, but I don't think we need to hold this PR up because of that. All those changes wouldn't affect the public API so I don't mind if we do it over time.

hedgehog/src/Hedgehog/Internal/Property.hs Outdated Show resolved Hide resolved
classify :: (MonadTest m, HasCallStack) => Bool -> String -> m ()
classify = cover 0

cover :: (MonadTest m, HasCallStack) => Double -> Bool -> String -> m ()

This comment has been minimized.

Copy link
@jacobstanley

jacobstanley Apr 19, 2019

Member

I would like to have a comment for this as it will be visible in the haddock.

@mankyKitty maybe we should mention this in the style guide, public facing functions should have a comment. Internal ones I think it's developer's discretion.

This comment has been minimized.

Copy link
@mankyKitty

mankyKitty Apr 20, 2019

Contributor

Sounds good

hedgehog/src/Hedgehog/Internal/Runner.hs Outdated Show resolved Hide resolved

@felixmulder felixmulder force-pushed the felixmulder:topic/add-classifications branch from 9e8723d to 59d79a6 Apr 20, 2019

@felixmulder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

@jystic - done 👍

@jacobstanley

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

Great, so I started adding a few newtypes around Int and String and just fixing a few nits before merge and got a bit carried away, spent all day on it but the failure output is much prettier now. @moodmosaic and I spent a long time bikeshedding type names 😆

An example:
Screenshot 2019-04-21 at 6 06 39 PM

It also prints the current coverage while the test is running, you'll need to run the example to see. I moved the test to hedgehog-examples because we actually want them to fail so we can check the output.

coverageFailures tests (Coverage kvs) =
filter (not . classifierCovered tests) (Map.elems kvs)

-- | Records the proportion of tests which satisfy a given condiiton.

This comment has been minimized.

Copy link
@felixmulder

felixmulder Apr 21, 2019

Author Contributor

condiiton

This comment has been minimized.

Copy link
@jacobstanley

jacobstanley Apr 21, 2019

Member

thanks! :D

@jacobstanley

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

@HuwCampbell I decided I will merge this and do the Log integration as a separate PR as it will be easier to review I think. I won't do a release until this happens so I don't think it's a big issue.

@jacobstanley jacobstanley force-pushed the felixmulder:topic/add-classifications branch from b1e21bf to 1c08e08 Apr 21, 2019

@charleso charleso referenced this pull request Apr 21, 2019

@jacobstanley jacobstanley force-pushed the felixmulder:topic/add-classifications branch from 1c08e08 to 451dc74 Apr 21, 2019

@jacobstanley jacobstanley force-pushed the felixmulder:topic/add-classifications branch from 451dc74 to d44f72a Apr 21, 2019

@jacobstanley

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

Semigroup/Monoid 7.10 fun :)

@jacobstanley jacobstanley merged commit ada4caa into hedgehogqa:master Apr 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@felixmulder felixmulder deleted the felixmulder:topic/add-classifications branch Apr 22, 2019

charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
charleso added a commit to hedgehogqa/scala-hedgehog that referenced this pull request Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.