Make seed part of config and pass into recheck data#482
Make seed part of config and pass into recheck data#482njlr wants to merge 1 commit intohedgehogqa:masterfrom
Conversation
moodmosaic
left a comment
There was a problem hiding this comment.
Thank you for the PR!
The core feature is a good addition, but the default should be RandomSeed to preserve existing behavior. The withSeed API lets users opt into determinism explicitly, which is the right trade-off for a property testing library.
| secondValues.Count > 10 |> Expect.isTrue | ||
| thirdValues.Count > 10 |> Expect.isTrue | ||
|
|
||
| testCase "Seed is deterministic" <| fun () -> |
There was a problem hiding this comment.
This is great! Though I think more cases are warranted:
- same fixed seed, same results
- same fixed seed in sync and async path give same results
withRandomSeeddiffers from fixed-seed behaviorrecheckpath preserves configured seed semantics
| { TestLimit = 100<tests> | ||
| ShrinkLimit = None } | ||
| ShrinkLimit = None | ||
| SeedConfig = FixedSeed (Seed.from 0UL) } |
There was a problem hiding this comment.
This changes the default config from “effectively random every run” to always deterministic.
I'd have to thoroughly check but if the shrinking/recheck model depends on initial seed plus shrink path, changing default seed behavior affects:
- reproduced counterexamples
- golden tests
- user workflows that expect “rerun gives new samples”
This is a significant semantic change. Every user who calls Property.check, Property.checkBool, or any function that flows through PropertyConfig.defaults will now:
- Always test the same inputs across runs (unless they opt into
withRandomSeed) - Potentially miss bugs that only manifest with certain random seeds
- Get different behavior from what they've been relying on without changing any of their own code
AFAIK, in most property-based testing systems, the default is random seeds with the seed printed on failure for reproducibility.
The deterministic-by-default philosophy might exist, but flipping an established default can be surprising to the users, if not risky.
| /// This blocks until all tests complete. | ||
| let reportWith (config : IPropertyConfig) (p : Property<unit>) : Report = | ||
| p |> reportWith' (PropertyArgs.init ()) config | ||
| let seed = SeedConfig.init config.SeedConfig |
There was a problem hiding this comment.
That's good: it means only one concrete seed per property run, not reevaluating randomness multiple times.
SeedConfigfor defining starting seed configurationIPropertyConfigwith aSeedConfigSeedConfigin aIPropertyConfigSeedConfigto set initialRecheckDatainreportWithPropertyConfig.defaultsRelates to #450