[replays-dont-count-cases] #295
Conversation
proptest/src/test_runner/errors.rs
Outdated
| pub(crate) enum TestCaseOk { | ||
| NewCaseSuccess, | ||
| ReplaySuccess, | ||
| CacheHitSuccess, | ||
| Reject, | ||
| } |
There was a problem hiding this comment.
Not expecting any changes inline to this PR as these thoughts aren't fully baked but wanted to run some stuff by you and see how you're thinking of these things. Also since this is a crate-private type, we can evolve this independently of crate version so that gives us some flexibility for the future.
I've been thinking of how to add some new functionality to support #284 (providing manual, explicit cases) and was thinking of introducing a new wrapper type for cases that carry context on their source so we can have something like:
enum Case<T: fmt::Debug, S: Strategy<Value=T>> {
UserProvided(T),
Generated(S),
Persisted(S, Seed),
Replay(S, TestCaseResult),
}
or something along these lines and then the test-runner code can branch on these accordingly to affect seed, success counts, whether we actually run the test or just emulate it based on replay value, etc.
We could be left with a result type of:
enum TestCaseOk {
Success,
CacheHit,
Reject,
}
Not entirely sure all this would end up working out with the current code structure as I haven't actually started implementing it.
There was a problem hiding this comment.
And more direct comment on this code after reading some more below, I think we'd want a PersistedCaseSuccess variant. Right now it seems ReplaySuccess is being overloaded for both persisted tests as well as forked tests.
There was a problem hiding this comment.
that seems like a good approach to me 👍
proptest/src/test_runner/runner.rs
Outdated
| result | ||
| result.map(|_| { | ||
| if is_from_persisted_seed { | ||
| TestCaseOk::ReplaySuccess |
There was a problem hiding this comment.
I don't think this is the right return value. we're actually running the test for this result so its not really a "replay" in the context of what "replay" means for forked tests. I commented on the enum def as well but to be explicit, what do you think about a PersistedCaseSuccess variant?
There was a problem hiding this comment.
good call out, will make that change 👍
proptest/src/test_runner/runner.rs
Outdated
| // Since all of our test runs happened in forks already, we need to | ||
| // force the runner to count successful replays against our total | ||
| // success count. | ||
| true, |
There was a problem hiding this comment.
not a huge fan of having boolean flags flip behavior like this. having explicit variants for PersistedCaseSuccess and ReplaySuccess would remove the need for this right? I think we always want replays to count against the success count and we never want persisted cases to count against success count.
rex-remind101
force-pushed
the
replays-dont-count-cases
branch
2 times, most recently
from
35ad221
to
5ec56a6
Compare
… successful cases. this way when `PROPTEST_CASES` <= `number of persisted cases` we will not end up never creating new cases to test against. rather, all persisted cases will be tested _and then_ a new number of cases will run equal to the number of `PROPTEST_CASES`.
rex-remind101
force-pushed
the
replays-dont-count-cases
branch
from
5ec56a6
to
a854d2e
Compare
There was a problem hiding this comment.
Apologies for taking so long looking into this 😅
Code looks good, one minor thought I have is that it may be surprising to users if you have PROPTEST_CASES=1 and it still runs multiple tests. This is something I do fairly frequently - I have some tests that take a fairly long time to run, so want to only run it once for a shorter local dev cycle.
At the very least, we should document this (though IIRC the new behaviour is what the documentation said, if so, then that's probably fine).
Perhaps it's worth adding something like PROPTEST_TOTAL_CASES or something, to set an absolute limit on the number of tests run, even if that means skipping known regressions. Though that's for sure out of scope for this PR. I'm just not sure about whether it's something we should block 1.1 on
replays/persisted failures are not counted against successful cases.
this way when
PROPTEST_CASES<=number of persisted caseswe will not end up never creating new cases to test against. rather, all persisted cases will be tested and then a new number of cases will run equal to the number ofPROPTEST_CASES.addresses #290