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

Adding recheck method and seed reporting #233

Merged
merged 13 commits into from Dec 1, 2020
Merged

Adding recheck method and seed reporting #233

merged 13 commits into from Dec 1, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2020

Fixes #179

Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

This would be nice to have 👍 I haven't looked into the implementation yet, but I checked out the branch and run a test and left you some feedback.

src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

We could have this into this release 👍 I just left two small suggestions/nitpicks. Thank you for your work on this, @adam-becker 💯

src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

I was adding some tests before merging and came across an issue with recheck where the message we show differs from the way the recheck function is defined.

src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

@adam-becker, I've passed the size to the message (which is basically tests + discards). One last thing, I'd like to see if we can have

> Property.recheck (Size 1) (Seed 2244935751425070623UL 5562567424033617363UL) <property>

instead of


> Property.recheck (Size 1) ({ Value = 2244935751425070623UL; Gamma = 5562567424033617363UL }) <property>

@moodmosaic moodmosaic self-requested a review November 27, 2020 13:07
@moodmosaic moodmosaic marked this pull request as draft November 27, 2020 13:35
@moodmosaic
Copy link
Member

FWIW, I've converted this into a draft; we're rendering the size in the output message and what's left is to also pass the size in. For example, reportWithSeed must take also size, and must be renamed.

@moodmosaic moodmosaic marked this pull request as ready for review November 27, 2020 13:56
@moodmosaic moodmosaic marked this pull request as draft November 27, 2020 14:01
@moodmosaic
Copy link
Member

@willsam100, @adam-becker, would you care trying this branch on your local?

@TysonMN
Copy link
Member

TysonMN commented Nov 27, 2020

I will also try this branch out later today

@TysonMN
Copy link
Member

TysonMN commented Nov 28, 2020

I don't think it is working. Maybe I am doing it wrong though. A second issue is that when calling Property.recheck on a failing test still outputs how to recheck the test. Below, I explain how I tested and then give some closing thoughts.


Here is the test I wrote to see if Property.recheck is actually rechecking the same previously failed test case.

let mutable failFlag = true

[<Fact>]
let ``only fail first check`` () =
    Property.check <| property {
        let! n = Range.constant System.Int64.MinValue System.Int64.MaxValue |> Gen.int64
        if failFlag then
            failFlag <- false
            failwith "failFlag was true"
    }

Here is the output from one of my executions.

Hedgehog.FailedException : *** Failed! Falsifiable (after 1 test):
65061791783614996L
System.Exception: failFlag was true
   at PropertyTests.only fail first [email protected](Int64 _arg1) in C:\Code\ThirdParty\fsharp-hedgehog\tests\Hedgehog.Tests\PropertyTests.fs:line 15
   at [email protected](Unit _arg1) in C:\Code\ThirdParty\fsharp-hedgehog\src\Hedgehog\Property.fs:line 304
This failure can be reproduced by running:
> Property.recheck (1 : Size) ({ Value = 1343219580531722066UL; Gamma = 6719782976908311835UL }) <property>

Now I expect executing the next test will cause 65061791783614996L to be generated again.

let mutable failFlag = true

[<Fact>]
let ``only fail first check`` () =
    Property.recheck (1 : Size) ({ Value = 1343219580531722066UL; Gamma = 6719782976908311835UL }) <| property {
        let! n = Range.constant System.Int64.MinValue System.Int64.MaxValue |> Gen.int64
        if failFlag then
            failFlag <- false
            failwith "failFlag was true"
    }

But this is not the case. Instead, -5078635085846143995L is generated.

Hedgehog.FailedException : *** Failed! Falsifiable (after 1 test):
-5078635085846143995L
System.Exception: failFlag was true
   at PropertyTests.only fail first [email protected](Int64 _arg1) in C:\Code\ThirdParty\fsharp-hedgehog\tests\Hedgehog.Tests\PropertyTests.fs:line 15
   at [email protected](Unit _arg1) in C:\Code\ThirdParty\fsharp-hedgehog\src\Hedgehog\Property.fs:line 304
This failure can be reproduced by running:
> Property.recheck (1 : Size) ({ Value = 5500455877167666293UL; Gamma = 6719782976908311835UL }) <property>

Now, I don't think Property.recheck should display instructions for how to repeat the test. However, given that this data is being displayed, we can take advantage of it for testing. Notice that the seed I gave is not the seed that message says should be used to reproduce the test.

On the other hand, at least that test is deterministic now: running that test will always generate -5078635085846143995L and will always incorrectly claim that the seed to reproduce is { Value = 5500455877167666293UL; Gamma = 6719782976908311835UL }.


First, I recommend automating this manual testing that I have done. I would do this by having an internal function that returns the size and seed. Then the current Property.check function and a test can both call this function (because internals can be given access to the test project). The test would record the value of n for which property fails, use the returned size and seed to execute the generator again, and see if the same value of n is returned. (Since renderf is impure, it should be called very near the entry point, which in this case is Property.check.)

Second, I recommend getting that test to pass.

Third, I recommend excluding the This failure can be reproduced by running output when Property.recheck is called.

@ghost
Copy link
Author

ghost commented Nov 28, 2020

@TysonMN I've fixed the issue you pointed out, though recheck still reports the seed when the case fails.
@moodmosaic I've changed the format specifier to be %0A when printing the seed, so everything is formatted properly.

The function is a little clunky to use, however. Since the size parameter in the message is always going to be (1 : Size), can't it be omitted from both the message and the function's parameter list?

@ghost ghost marked this pull request as ready for review November 28, 2020 23:50
@TysonMN
Copy link
Member

TysonMN commented Nov 29, 2020

@TysonMN I've fixed the issue you pointed out

Great! Did you push that change? I don't see it.

Since the size parameter in the message is always going to be (1 : Size), can't it be omitted from both the message and the function's parameter list?

It is not always 1. It will always be 1 in the test I gave above. Here is a test where it is always 2.

let mutable testsToPass = 1

[<Fact>]
let ``fail after first test`` () =
    Property.check <| property {
        let! n = Range.constant System.Int64.MinValue System.Int64.MaxValue |> Gen.int64
        if testsToPass = 0 then
            failwith "now always failing"
        else
            testsToPass <- testsToPass - 1
    }

Sample output:

Hedgehog.FailedException : *** Failed! Falsifiable (after 2 tests and 1 shrink):
-9223372036854775808L
System.Exception: now always failing
   at PropertyTests.fail after first [email protected](Int64 _arg1) in C:\Code\ThirdParty\fsharp-hedgehog\tests\Hedgehog.Tests\PropertyTests.fs:line 14
   at [email protected](Unit _arg1) in C:\Code\ThirdParty\fsharp-hedgehog\src\Hedgehog\Property.fs:line 304
This failure can be reproduced by running:
> Property.recheck (2 : Size) ({ Value = 6052627415793634358UL; Gamma = 17135851100886004659UL }) <property>

At the "top level", size is the test number. By default, each property is tested 100 times. The first value of the size parameter starts as 1 and its last value is 100.

@moodmosaic
Copy link
Member

Size shouldn't be hardcoded to 1 because size = tests + discards

@moodmosaic
Copy link
Member

I'm on my phone right now (hence the brevity) but here's a property worth testing on this branch

property {
    let! x = Gen.int <| Range.constantBounded ()
    let! y = Gen.int <| Range.constantBounded ()
    where (x > 0 && y > 0)
    counterexample (sprintf "x * y = %d" <| x * y)
    return x * y > 0
}

@moodmosaic moodmosaic marked this pull request as draft November 29, 2020 02:15
@ghost
Copy link
Author

ghost commented Nov 29, 2020

Great! Did you push that change? I don't see it.

Sorry, I thought I had.

Size shouldn't be hardcoded to 1 because size = tests + discards

Not sure how I was under that impression, thanks for clarifying.

@TysonMN
Copy link
Member

TysonMN commented Nov 29, 2020

I tested that commit. It works correctly now. Great work :)

src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
@moodmosaic
Copy link
Member

Great progress so far 👀 @adam-becker🏅@TysonMN 💯

@moodmosaic moodmosaic marked this pull request as ready for review November 29, 2020 02:50
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

Awesome! Getting really close. Left a comment which I think makes sense specially for F# Interactive users.

src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
src/Hedgehog/Property.fs Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Nov 30, 2020

Awesome! Getting really close. Left a comment which I think makes sense specially for F# Interactive users.

Done.

@moodmosaic moodmosaic merged commit 1f8b3a4 into hedgehogqa:master Dec 1, 2020
@moodmosaic
Copy link
Member

🎊

@ghost ghost deleted the add-recheck branch December 1, 2020 18:58
@ghost ghost added this to the 0.9.0 milestone Sep 22, 2021
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.

Print seed to reproduce failure (as FsCheck allows)
2 participants