Fix test skipping when tests have been discarded. #489
Conversation
|
Why is it necessary to track |
|
If we only track the sum, we'd need to handle the case when that's larger than the number of tests we want to run. Which might be fine - I think this is a case where it's not clear exactly what behavior is best. It also means that we'd get a failure "after 3 tests and 7 discards", and then we'd try to reproduce it and get told it failed "after 10 tests", which seems mildly bad. |
|
I think that message is not helpful when recording |
|
I agree it's not really helpful, but if it's going to be there it seems kinda confusing if it doesn't match the one they got before. We could remove it when replaying I guess, but that would involve more changes. Ultimately, it would be possible to only track (sum of tests and discards), but I think tracking them separately is mildly more user friendly and mildly easier to implement. |
|
@moodmosaic gentle nudge - it would be good to get this fix in, I think. |
|
For the rest of the changes, as also mentioned in #454, it'd be nice also here if our friends can take a look wrt tasty/hspec hedgehog integrations. /cc @parsonsmatt @dalaing @gwils |
|
@moodmosaic This is easy for us to support; I have a patch ready to go. It will be especially easy if you do release this as part of hedgehog 1.4 instead of 1.3.x, since then our upper bounds will work correctly without revision. |
|
Sure, no problem for 1.4 as it's also more correct. If that works, we can merge after @ChickenProp resolves the conflicts and then do a hedgehog 1.4 release this weekend. 🦔 |
I might be misinterpreting, but is the question "could this break packages that depend on hedgehog?" I don't expect downstream users to commit code with a |
Closes hedgehogqa#487. In hedgehogqa#454 we introduced test skipping, with the idea that a failed test could report a way for you to jump back to reproduce it without all the preceding tests. But it didn't work if any of the preceding tests had been discarded, because each discard also changes the seed and the size. Users could manually add the discard count to the test count in the `Skip`, but that's no fun. Plus, it wouldn't work if the test count plus discard count exceeded the test limit, because that would generate a success without running any tests. So now a `Skip` (other than `SkipNothing`) includes a `DiscardCount` as well as a `TestCount`. It's rendered in the compressed path as `testCount/discardCount`, or just `testCount` if `discardCount` is 0. The exact sequence of passing tests and discards doesn't affect the final seed or size, so the counts are all we need. This changes an exposed type, so PVP requires a major version bump.
ChickenProp
force-pushed
the
fix-skipping-with-discards
branch
from
c569c61
to
6018bdb
Compare
|
Rebased with conflicts fixed. |
|
Thank you, @ChickenProp 🚀 A |
|
Release just went out. 🦔 |
Closes #487.
In #454 we introduced test skipping, with the idea that a failed test could report a way for you to jump back to reproduce it without all the preceding tests.
But it didn't work if any of the preceding tests had been discarded, because each discard also changes the seed and the size. Users could manually add the discard count to the test count in the
Skip, but that's no fun. Plus, it wouldn't work if the test count plus discard count exceeded the test limit, because that would generate a success without running any tests.So now a
Skip(other thanSkipNothing) includes aDiscardCountas well as aTestCount. It's rendered in the compressed path astestCount/discardCount, or justtestCountifdiscardCountis 0. The exact sequence of passing tests and discards doesn't affect the final seed or size, so the counts are all we need.This changes an exposed type, so PVP requires a major version bump.
This is awkward for me personally because it's incompatible with the approach I took in #474, I'll leave a comment there with more details.