Closed sageserpent-open closed 1 year ago
Hmm - I'm not sure whether .numberOfPreviousCycles
is simply a misnomer. It was named .numberOfPreviousShrinkages
, which also misleading - perhaps what it should signify is the depth of shrinkage, so both an ordinary and a following panic mode shrinkage should get the same value. If so, then presumably we should reflect the difference in modes as an additional property in CaseSupplyCycle
?
The name is a misnomer - what .numberOfPreviousCycles
really yields is the number of nested shrinkage attempts, each of which resulted in a failing trial. So the older name was a little more accurate, but certainly not intuitive. What happens in each shrinkage attempt is that a cycle is run for normal shrinkage, and if that doesn't encounter a failing trial, an additional cycle for panic mode shrinkage is run.
So a better name would be .numberOfPreviousFailures
- this captures what is going on, but isn't too tied in with the shrinkage implementation details.
For the sake of not breaking API compatibility, maybe the existing .numberOfPreviousCycles
should be left in, but cutover to actually count the number of cycles?
Fixed in commit SHA: 0a64297fa0930275fcfaedac844759e4287afaef , went out in release 1.10.1
Running this code in JShell:
Yields an incorrect number of cases cycles for the final invocation of the callback supplied to
.withStrategy
:Note the count is repeated. Possibly this is something to do with the shrinkage switching between ordinary shrinkage and panic mode shrinkage? A cursory inspection of
shrink
buried deep withinSupplyToSyntaxSkeletalImplementation
suggests this is the case.