From 5b9c3b2673cf45ba14eb8449e32a94dd71a57eba Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Mon, 12 Dec 2022 15:03:04 +0300 Subject: [PATCH 1/2] Fix ActionChain failure include never-executed actions since it uses wrong state object when creating transformations This change caches the transformation if it accesses state. fixes https://github.com/jlink/jqwik/issues/426 --- .../state/ShrinkableChainIteration.java | 12 +++- .../state/ActionChainArbitraryTests.java | 70 +++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) 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 4567d964d..48dc5b659 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 @@ -6,9 +6,12 @@ import net.jqwik.api.*; import net.jqwik.api.state.*; +import org.jetbrains.annotations.*; + class ShrinkableChainIteration { final Shrinkable> shrinkable; private final Predicate precondition; + private final @Nullable Transformer transformer; final boolean accessState; final boolean changeState; @@ -31,18 +34,21 @@ private ShrinkableChainIteration( 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; } @Override public String toString() { return String.format( "Iteration[accessState=%s, changeState=%s, transformation=%s]", - accessState, changeState, shrinkable.value().transformation() + accessState, changeState, transformer().transformation() ); } boolean isEndOfChain() { - return shrinkable.value().equals(Transformer.END_OF_CHAIN); + return transformer().equals(Transformer.END_OF_CHAIN); } Optional> precondition() { @@ -65,6 +71,6 @@ String transformation() { } Transformer transformer() { - return shrinkable.value(); + return transformer == null ? shrinkable.value() : transformer; } } 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 bda69b935..56026a411 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 @@ -119,6 +119,76 @@ ActionChainArbitrary xOrFailing() { .withMaxTransformations(30); } + static class SetMutatingChainState { + final List actualOps = new ArrayList<>(); + final Set set = new HashSet<>(); + + @Override + public String toString() { + return "set=" + set + ", actualOps=" + actualOps; + } + } + + @Property + void chainActionsAreProperlyDescribedEvenAfterChainExecution(@ForAll("setMutatingChain") ActionChain chain) { + SetMutatingChainState finalState = chain.run(); + + assertThat(chain.transformations()) + .describedAs("chain.transformations() should be the same as the list of operations in finalState.actualOps, final state is %s", finalState.set) + .isEqualTo(finalState.actualOps); + } + + @Provide + public ActionChainArbitrary setMutatingChain() { + return + ActionChain + .startWith(SetMutatingChainState::new) + // This is an action that does not depend on the state to produce the transformation + .addAction( + 1, + Action.just("clear anyway", state -> { + state.actualOps.add("clear anyway"); + state.set.clear(); + return state; + }) + ) + // Below actions depend on the state to derive the transformations + .addAction( + 1, + (Action.Dependent) + state -> + Arbitraries + .just( + state.set.isEmpty() + ? Transformer.noop() + : Transformer.mutate("clear " + state.set, set -> { + state.actualOps.add("clear " + set.set); + state.set.clear(); + }) + ) + ) + .addAction( + 2, + (Action.Dependent) + state -> + Arbitraries + .integers() + .between(1, 10) + .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); + }); + } + } + ) + ) + .withMaxTransformations(5); + } + @Property void chainChoosesBetweenTwoActions(@ForAll("xOrY") ActionChain chain) { String result = chain.run(); From 2f131ec84567446d82808a4d7c128468ac312357 Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Mon, 12 Dec 2022 19:05:58 +0300 Subject: [PATCH 2/2] Cache several shrink results for every chain step, so it does not re-access state model state during shrink phase This might take slightly more memory and produce suboptimal shrinks, however makes shrink possible for state-accessing transformations. Fixes https://github.com/jlink/jqwik/issues/428 --- .../state/ShrinkableChainIteration.java | 70 ++++++++++++++++--- .../state/ActionChainArbitraryTests.java | 40 +++++++++-- 2 files changed, 95 insertions(+), 15 deletions(-) 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