Report accurate ScalaCheck seed on failing test #195
Conversation
There was a problem hiding this comment.
Looks great, thanks @nevillelyh for tracking this down and fixing it!
|
Not sure how to test though, probably need some variation of |
|
Actually DO NOT MERGE. Let me verify if this actually works as expected first. |
|
I am happy to merge without a test case (although it would be nice to have a test 😄 ) |
|
Think this actually broke another case. It's hard to verify since the test is non-deterministic 😭. At least let me verify some basic scenarios. |
| scalaCheckTestParameters.initialSeed.getOrElse( | ||
| Seed.fromBase64(scalaCheckInitialSeed).get | ||
| ) | ||
| val initialSeed = makeSeed() |
There was a problem hiding this comment.
This should be the actual "initial seed", one that's used to generate seeds for all test runs (default 100) of the same property.
| val result = check( | ||
| scalaCheckTestParameters, | ||
| Prop(genParams => prop(genParams.withInitialSeed(seed))) | ||
| Prop { genParams => | ||
| val r = prop(genParams.withInitialSeed(seed)) |
There was a problem hiding this comment.
A single test run here. And advance seed to the next.
| ) | ||
| def renderResult(r: Result): String = { | ||
| val resultMessage = Pretty.pretty(r, scalaCheckPrettyParameters) | ||
| if (r.passed) { | ||
| resultMessage | ||
| } else { | ||
| val seedMessage = s"""|Failing seed: ${seed.toBase64} | ||
| val seedMessage = s"""|Failing seed: ${initialSeed.toBase64} |
There was a problem hiding this comment.
If a test failed, say 67th out of 100, we still report the 1st seed in case the test is not pure.
| final class ScalaCheckInitialSeedSuite extends ScalaCheckSuite { | ||
|
|
||
| // initial seed should be used for the first out of 100 `minSuccessfulTests` only | ||
| override val scalaCheckInitialSeed = "9SohH7wEYXCdXK4b9yM2d6TKIN2jBFcBs4QBta-2yTD=" |
There was a problem hiding this comment.
This test checks that when initial seed is overridden, we still generate different input per test run instead of reusing the same.
olafurpg
changed the title
There was a problem hiding this comment.
@nevillelyh Thank you for double checking! A few minor comments, otherwise this looks ready to go
| @@ -15,6 +15,26 @@ final class ScalaCheckSeedSuite extends ScalaCheckSuite { | |||
| } | |||
| } | |||
|
|
|||
| test("generated int are not all the same") { | |||
| println(ints) | |||
| } | ||
| } | ||
|
|
||
| final class ScalaCheckInitialSeedSuite extends ScalaCheckSuite { |
There was a problem hiding this comment.
Can we move this to a separate file? I normally try to organize one test suite per file.
Failing seed reporting seems to be incorrect when
scalaCheckInitialSeedis not overridden. In that casedef seedwould usedef scalaCheckInitialSeed: String = Seed.random().toBase64and be different every time. Hence the 2 seeds seen here are actually not the ones causing test failure.https://travis-ci.org/github/spotify/magnolify/jobs/708925430#L369