diff --git a/engine/src/main/java/net/jqwik/engine/properties/state/ShrinkableChainIteration.java b/engine/src/main/java/net/jqwik/engine/properties/state/ShrinkableChainIteration.java index 48dc5b659..0481dde9d 100644 --- a/engine/src/main/java/net/jqwik/engine/properties/state/ShrinkableChainIteration.java +++ b/engine/src/main/java/net/jqwik/engine/properties/state/ShrinkableChainIteration.java @@ -2,16 +2,63 @@ import java.util.*; import java.util.function.*; +import java.util.stream.*; import net.jqwik.api.*; import net.jqwik.api.state.*; -import org.jetbrains.annotations.*; - class ShrinkableChainIteration { + // Larger values might improve shrink quality, however, they increase the shrink space, so it might increase shrink duration + private final static int NUM_SAMPLES_IN_EAGER_CHAIN_SHRINK = Integer.getInteger("jqwik.eagerChainShrinkSamples", 2); + + static class ShrinkableWithEagerValue implements Shrinkable { + protected final Shrinkable base; + private final ShrinkingDistance distance; + final T value; + + ShrinkableWithEagerValue(Shrinkable base) { + this.base = base; + this.distance = base.distance(); + this.value = base.value(); + } + + @Override + public T value() { + return value; + } + + @Override + public Stream> shrink() { + return base.shrink(); + } + + @Override + public ShrinkingDistance distance() { + return distance; + } + } + + static class EagerShrinkable extends ShrinkableWithEagerValue { + private final List> shrinkResults; + + EagerShrinkable(Shrinkable base, int numSamples) { + super(base); + this.shrinkResults = + base.shrink() + .sorted(Comparator.comparing(Shrinkable::distance)) + .limit(numSamples) + .map(ShrinkableWithEagerValue::new) + .collect(Collectors.toList()); + } + + @Override + public Stream> shrink() { + return shrinkResults.stream(); + } + } + final Shrinkable> shrinkable; private final Predicate precondition; - private final @Nullable Transformer transformer; final boolean accessState; final boolean changeState; @@ -33,10 +80,17 @@ private ShrinkableChainIteration( this.precondition = precondition; this.accessState = accessState; this.changeState = changeState; - this.shrinkable = shrinkable; - // Transformer method might access state, so we need to cache the value here - // otherwise it might be evaluated with wrong state (e.g. after chain executes) - this.transformer = accessState ? shrinkable.value() : null; + // When the shrinkable does not access state, we could just use it as is for ".value()", and ".shrink()" + // If we get LazyShrinkable here, it means we are in a shrinking phase, so we know ".shrink()" will be called only + // in case the subsequent execution fails. So we can just keep LazyShrinkable as is + // Otherwise, we need to eagerly evaluate the shrinkables to since the state might change by appyling subsequent transformers, + // so we won't be able to access the state anymore. + // See https://github.com/jlink/jqwik/issues/428 + if (!accessState || shrinkable instanceof ShrinkableChainIteration.ShrinkableWithEagerValue) { + this.shrinkable = shrinkable; + } else { + this.shrinkable = new EagerShrinkable<>(shrinkable, NUM_SAMPLES_IN_EAGER_CHAIN_SHRINK); + } } @Override @@ -71,6 +125,6 @@ String transformation() { } Transformer transformer() { - return transformer == null ? shrinkable.value() : transformer; + return shrinkable.value(); } } diff --git a/engine/src/test/java/net/jqwik/engine/properties/state/ActionChainArbitraryTests.java b/engine/src/test/java/net/jqwik/engine/properties/state/ActionChainArbitraryTests.java index 56026a411..5ff10bb1c 100644 --- a/engine/src/test/java/net/jqwik/engine/properties/state/ActionChainArbitraryTests.java +++ b/engine/src/test/java/net/jqwik/engine/properties/state/ActionChainArbitraryTests.java @@ -121,6 +121,7 @@ ActionChainArbitrary xOrFailing() { static class SetMutatingChainState { final List actualOps = new ArrayList<>(); + boolean hasPrints; final Set set = new HashSet<>(); @Override @@ -131,6 +132,14 @@ public String toString() { @Property void chainActionsAreProperlyDescribedEvenAfterChainExecution(@ForAll("setMutatingChain") ActionChain chain) { + chain = chain.withInvariant( + state -> { + if (state.hasPrints) { + assertThat(state.actualOps).hasSizeLessThan(5); + } + } + ); + SetMutatingChainState finalState = chain.run(); assertThat(chain.transformations()) @@ -168,7 +177,7 @@ public ActionChainArbitrary setMutatingChain() { ) ) .addAction( - 2, + 4, (Action.Dependent) state -> Arbitraries @@ -177,16 +186,33 @@ public ActionChainArbitrary setMutatingChain() { .map(i -> { if (state.set.contains(i)) { return Transformer.noop(); - } else { - return Transformer.mutate("add " + i + " to " + state.set, newState -> { - newState.actualOps.add("add " + i + " to " + newState.set); - newState.set.add(i); - }); } + return Transformer.mutate("add " + i + " to " + state.set, newState -> { + newState.actualOps.add("add " + i + " to " + newState.set); + newState.set.add(i); + }); + } + ) + ) + .addAction( + 2, + (Action.Dependent) + state -> + state.set.isEmpty() ? Arbitraries.just(Transformer.noop()) : + Arbitraries + .of(state.set) + .map(i -> { + if (!state.set.contains(i)) { + throw new IllegalStateException("The set does not contain " + i + ", current state is " + state); + } + return Transformer.mutate("print " + i + " from " + state.set, newState -> { + newState.actualOps.add("print " + i + " from " + newState.set); + newState.hasPrints = true; + }); } ) ) - .withMaxTransformations(5); + .withMaxTransformations(7); } @Property