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

Implement labelledExamples #167

Merged
merged 4 commits into from
Aug 17, 2020
Merged

Conversation

dwijnand
Copy link
Collaborator

Fixes #163

This introduces some duplication but, more generally, I'm not sure it's doing exactly the right thing.
Particularly: how is it meant to behave on a failing property? What about None results? Also, should it only
print the journal logs, as it's doing, or also the result logs? And did I make all the correct modifications in
my "takeSmallest" copy? Lots of questions.

I'd gladly have this land in a "we're not totally sure"/experimental feature and dogfood it in a project of
mine, which might give some better insights on what design choices are better. (But I can also copy this impl
into my project, so I wouldn't be blocked either - pure FP ftw.)

@dwijnand dwijnand force-pushed the labelledExamples branch 2 times, most recently from d92955b to 3bf9c82 Compare July 19, 2020 16:51
@charleso
Copy link
Collaborator

@dwijnand I'm definitely ok with taking some experimental code. I think my only preference here would not be to duplicate any of that logic though (those 2 functions in particular are critical to how it all hangs together). If we can either share what we can with common functions and possibly have some parameters/callbacks that might avoid us having to keep the two in sync. In particular, I know those functions have been refactored for Future support.

#147

I don't have any specific suggestions though I'm afraid.

@dwijnand
Copy link
Collaborator Author

Alright, I'll seek to dedup.

@dwijnand dwijnand force-pushed the labelledExamples branch 3 times, most recently from 58149d5 to ad8662c Compare July 20, 2020 20:39
@dwijnand dwijnand requested a review from charleso July 20, 2020 20:39
@dwijnand dwijnand force-pushed the labelledExamples branch 2 times, most recently from 309d9d3 to 932a814 Compare July 20, 2020 20:52
... if present and different to the label name itself.
@dwijnand
Copy link
Collaborator Author

Can this be merged and released? I think I left this in a state I was very happy with: useful and deduped - but happy to have another look if a fresh review spots anything.

@eed3si9n
Copy link
Collaborator

Is there a way of doing partest neg equivalent to see what gets printed on the screen when some test fails?

@dwijnand
Copy link
Collaborator Author

No, there's no setup for testing the behaviour of runner, just unit tests for the core's behaviour. (See also test/README.md 🙂)

@eed3si9n
Copy link
Collaborator

My interest is in the negative behavior like in this case how the example affect the test failure output, not so much the details of different runners.

@dwijnand
Copy link
Collaborator Author

Before:

[info] + hedgehog.LabelledExamplesTest.testReverse: OK, passed 100 tests
[info] > 72% nonempty
[info] > 28% empty
[info] - hedgehog.LabelledExamplesTest.testReverseFail: Falsified after 20 passed tests
[info] > List(a, b)
[info] > === Not Equal ===
[info] > --- lhs ---
[info] > List(b, a)
[info] > --- rhs ---
[info] > List(a, b)
[info] > 65% empty
[info] > 35% nonempty

After:

[info] + hedgehog.LabelledExamplesTest.testReverse: OK, passed 100 tests
[info] > 69% nonempty List(a)
[info] > 31% empty List()
[info] - hedgehog.LabelledExamplesTest.testReverseFail: Falsified after 19 passed tests
[info] > List(a, b)
[info] > === Not Equal ===
[info] > --- lhs ---
[info] > List(b, a)
[info] > --- rhs ---
[info] > List(a, b)
[info] > 78% empty List()
[info] > 21% nonempty List(a)

using

    , property("testReverse", testReverse).withExamples
    , property("testReverseFail", testReverseFail).withExamples

...

  def testReverse: Property =
    for {
      xs <- Gen.alpha.list(Range.linear(0, 10)).forAll
        .classify("empty", _.isEmpty)
        .classify("nonempty", _.nonEmpty)
    } yield xs.reverse.reverse ==== xs

  def testReverseFail: Property =
    for {
      xs <- Gen.alpha.list(Range.linear(0, 10)).forAll
        .classify("empty", _.isEmpty)
        .classify("nonempty", _.nonEmpty)
    } yield xs.reverse ==== xs

Copy link
Collaborator

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 it

@kevin-lee
Copy link
Member

Looks good! @dwijnand Would you mind updating docs as well?

@charleso
Copy link
Collaborator

👍 Looks good @dwijnand. You should definitely feel comfortable merging if and someone else are happy with it. Don't wait for me.

Just one final (probably unhelpful) thought was whether it would just be easier to always collect the examples on every run which might simply that code. It might not make sense, or not worth spending on more time on it, and in any case feel free to ignore me. :)

Thanks again!

@kevin-lee
Copy link
Member

That minimum coverage with cover works really well so the test failed. 😆

[info] - hedgehog.examples.ReverseTest.reverse: Falsified after 100 passed tests
[info] > Insufficient coverage.
[info] > 95% nonempty 50% ✓ List(a)
[info] > 5% empty 50% ✗ List()

@kevin-lee
Copy link
Member

@charleso I think it's good to merge it now and we can consider your thought afterwards.
Dale created this PR about a month ago and it's a nice feature to have so we should probably not spend more time on this.

@kevin-lee
Copy link
Member

@dwijnand As Charles said, please feel free to merge it. If you want to merge it but can't do it, please let me know.

@dwijnand dwijnand merged commit 0bfbd9b into hedgehogqa:master Aug 17, 2020
@dwijnand dwijnand deleted the labelledExamples branch August 17, 2020 14:48
@kevin-lee
Copy link
Member

@@ -247,6 +269,15 @@ case class DiscardCount(value: Int) {
DiscardCount(value + 1)
}

/** Whether the report should include an example for each label. */
sealed trait WithExamples
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer used? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did that come from? #175

@dwijnand dwijnand mentioned this pull request Aug 18, 2020
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.

Port of QuickCheck's labelledExamples?
4 participants