Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActionChain shrink should be called only with state objects that correspond to the relevant state position #428

Closed
vlsi opened this issue Dec 12, 2022 · 31 comments

Comments

@vlsi
Copy link
Contributor

vlsi commented Dec 12, 2022

Testing Problem

It looks like .shrink() can be called on a wrong state instance.

The key idea behind the test:

  1. There are actions to add int to a set, and clear the set
  2. The very last action attempts to print a value out of a set. Note: it selects the value to print based on the state.set, so I assume it should select valid values only.

If the value is not present in the "print transformer", then I believe it is illegal.

The following test throws IllegalStateException

static class SetMutatingChainState {
	final List<String> actualOps = new ArrayList<>();
	final Set<Integer> set = new HashSet<>();
	boolean hasPrints = false;

	@Override
	public String toString() {
		return "set=" + set + ", actualOps=" + actualOps;
	}
}

@Property(shrinking = ShrinkingMode.FULL, seed = "7077187739734332001")
void chainActionsAreProperlyDescribedEvenAfterChainExecution(@ForAll("setMutatingChain") ActionChain<SetMutatingChainState> chain) {
	chain = chain.withInvariant(
		state -> {
			if (state.hasPrints) {
				assertThat(state.actualOps).hasSizeLessThan(5);
			}
		}
	);

	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<SetMutatingChainState> setMutatingChain() {
	return
		ActionChain
			.startWith(SetMutatingChainState::new)
			.withAction(
				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
			.withAction(
				1,
				(Action.Dependent<SetMutatingChainState>)
					state ->
						Arbitraries
							.just(
								state.set.isEmpty()
									? Transformer.noop()
									: Transformer.<SetMutatingChainState>mutate("clear " + state.set, set -> {
									state.actualOps.add("clear " + set.set);
									state.set.clear();
								})
							)
			)
			.withAction(
				4,
				(Action.Dependent<SetMutatingChainState>)
					state ->
						Arbitraries
							.integers()
							.between(1, 10)
							.map(i -> {
									 if (state.set.contains(i)) {
										 return Transformer.noop();
									 }
									 return Transformer.mutate("add " + i + " to " + state.set, newState -> {
										 newState.actualOps.add("add " + i + " to " + newState.set);
										 newState.set.add(i);
									 });
								 }
							)
			)
			.withAction(
				2,
				(Action.Dependent<SetMutatingChainState>)
					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(7);
}
timestamp = 2022-12-12T18:16:17.809136, ActionChainArbitraryTests:chainActionsAreProperlyDescribedEvenAfterChainExecution = 
  java.lang.IllegalStateException:
    The set does not contain 1, current state is set=[], actualOps=[add 4 to [], add 6 to [4], add 1 to [4, 6], print 4 from [1, 4, 6], clear anyway]

                              |-----------------------jqwik-----------------------
tries = 1                     | # of calls to property
checks = 1                    | # of not rejected calls
generation = RANDOMIZED       | parameters are randomly generated
after-failure = SAMPLE_FIRST  | try previously failed sample, then previous seed
when-fixed-seed = ALLOW       | fixing the random seed is allowed
edge-cases#mode = MIXIN       | edge cases are mixed in
edge-cases#total = 0          | # of all combined edge cases
edge-cases#tried = 0          | # of edge cases tried in current run
seed = 7077187739734332001    | random seed to reproduce generated values

Sample
------
  chain: ActionChain[NOT_RUN]: 7 max actions
  
  After Execution
  ---------------
    chain:
      ActionChain[FAILED]: ["add 4 to []", "add 6 to [4]", "add 1 to [4, 6]", "print 4 from [1, 4, 6]", "clear anyway"]



The set does not contain 1, current state is set=[], actualOps=[add 4 to [], add 6 to [4], add 1 to [4, 6], print 4 from [1, 4, 6], clear anyway]
java.lang.IllegalStateException: The set does not contain 1, current state is set=[], actualOps=[add 4 to [], add 6 to [4], add 1 to [4, 6], print 4 from [1, 4, 6], clear anyway]
	at net.jqwik.engine.properties.state.ActionChainArbitraryTests.lambda$setMutatingChain$21(ActionChainArbitraryTests.java:206)
	at net.jqwik.engine.properties.shrinking.MappedShrinkable.value(MappedShrinkable.java:21)
	at net.jqwik.engine.properties.state.ShrinkableChainIteration.<init>(ShrinkableChainIteration.java:39)
	at net.jqwik.engine.properties.state.ShrinkableChainIteration.withShrinkable(ShrinkableChainIteration.java:59)
	at net.jqwik.engine.properties.state.ShrinkableChainShrinker.lambda$shrinkOneIterationAfterTheOther$3(ShrinkableChainShrinker.java:152)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:271)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.ArrayList$SubList$2.tryAdvance(ArrayList.java:1488)
	at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.lambda$initPartialTraversalState$0(StreamSpliterators.java:294)
	at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.fillBuffer(StreamSpliterators.java:206)
	at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.doAdvance(StreamSpliterators.java:161)
	at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.tryAdvance(StreamSpliterators.java:300)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:720)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:723)
	at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.lambda$initPartialTraversalState$0(StreamSpliterators.java:294)
	at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.fillBuffer(StreamSpliterators.java:206)
	at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.doAdvance(StreamSpliterators.java:161)
	at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.tryAdvance(StreamSpliterators.java:300)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:720)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:723)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:727)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:278)
	at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1632)
	at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.lambda$initPartialTraversalState$0(StreamSpliterators.java:294)
	at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.fillBuffer(StreamSpliterators.java:206)
	at java.base/java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.doAdvance(StreamSpliterators.java:161)
	at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.tryAdvance(StreamSpliterators.java:300)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:720)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:723)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:727)
	at java.base/java.util.stream.Streams$ConcatSpliterator.tryAdvance(Streams.java:727)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:127)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:502)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:488)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.findAny(ReferencePipeline.java:548)
	at net.jqwik.engine.properties.shrinking.AbstractSampleShrinker.shrink(AbstractSampleShrinker.java:63)
	at net.jqwik.engine.properties.shrinking.OneAfterTheOtherParameterShrinker.shrinkSingleParameter(OneAfterTheOtherParameterShrinker.java:43)
	at net.jqwik.engine.properties.shrinking.OneAfterTheOtherParameterShrinker.shrink(OneAfterTheOtherParameterShrinker.java:25)
	at net.jqwik.engine.properties.shrinking.ShrinkingAlgorithm.shrinkOneParameterAfterTheOther(ShrinkingAlgorithm.java:52)
	at net.jqwik.engine.properties.shrinking.ShrinkingAlgorithm.shrink(ShrinkingAlgorithm.java:32)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.shrinkAsLongAsSampleImproves(PropertyShrinker.java:135)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.lambda$shrink$3(PropertyShrinker.java:87)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.shrink(PropertyShrinker.java:89)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.shrink(PropertyShrinker.java:73)
	at net.jqwik.engine.properties.GenericProperty.shrink(GenericProperty.java:214)
	at net.jqwik.engine.properties.GenericProperty.shrinkAndCreateCheckResult(GenericProperty.java:183)
	at net.jqwik.engine.properties.GenericProperty.check(GenericProperty.java:80)
	at net.jqwik.engine.execution.CheckedProperty.check(CheckedProperty.java:67)
	at net.jqwik.engine.execution.PropertyMethodExecutor.executeProperty(PropertyMethodExecutor.java:90)
	at net.jqwik.engine.execution.PropertyMethodExecutor.executeMethod(PropertyMethodExecutor.java:69)
	at net.jqwik.engine.execution.PropertyMethodExecutor.lambda$execute$0(PropertyMethodExecutor.java:49)
	at net.jqwik.api.lifecycle.AroundPropertyHook.lambda$static$0(AroundPropertyHook.java:46)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$0(HookSupport.java:26)
	at net.jqwik.api.PropertyDefaults$PropertyDefaultsHook.aroundProperty(PropertyDefaults.java:91)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$1(HookSupport.java:31)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$0(HookSupport.java:26)
	at net.jqwik.engine.hooks.lifecycle.PropertyLifecycleMethodsHook.aroundProperty(PropertyLifecycleMethodsHook.java:57)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$1(HookSupport.java:31)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$0(HookSupport.java:26)
	at net.jqwik.engine.hooks.statistics.StatisticsHook.aroundProperty(StatisticsHook.java:37)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$1(HookSupport.java:31)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$0(HookSupport.java:26)
	at net.jqwik.engine.hooks.lifecycle.AutoCloseableHook.aroundProperty(AutoCloseableHook.java:13)
	at net.jqwik.engine.execution.lifecycle.HookSupport.lambda$wrap$1(HookSupport.java:31)
	at net.jqwik.engine.execution.PropertyMethodExecutor.execute(PropertyMethodExecutor.java:47)
	at net.jqwik.engine.execution.PropertyTaskCreator.executeTestMethod(PropertyTaskCreator.java:166)
	at net.jqwik.engine.execution.PropertyTaskCreator.lambda$createTask$1(PropertyTaskCreator.java:51)
	at net.jqwik.engine.execution.lifecycle.CurrentDomainContext.runWithContext(CurrentDomainContext.java:28)
	at net.jqwik.engine.execution.PropertyTaskCreator.lambda$createTask$2(PropertyTaskCreator.java:50)
	at net.jqwik.engine.execution.pipeline.ExecutionTask$1.lambda$execute$0(ExecutionTask.java:31)
	at net.jqwik.engine.execution.lifecycle.CurrentTestDescriptor.runWithDescriptor(CurrentTestDescriptor.java:17)
	at net.jqwik.engine.execution.pipeline.ExecutionTask$1.execute(ExecutionTask.java:31)
	at net.jqwik.engine.execution.pipeline.ExecutionPipeline.runToTermination(ExecutionPipeline.java:82)
	at net.jqwik.engine.execution.JqwikExecutor.execute(JqwikExecutor.java:46)
	at net.jqwik.engine.JqwikTestEngine.executeTests(JqwikTestEngine.java:70)
	at net.jqwik.engine.JqwikTestEngine.execute(JqwikTestEngine.java:53)
@vlsi vlsi changed the title ActionChain shrink should be called only with state objects that correspond to the relevant state position. ActionChain shrink should be called only with state objects that correspond to the relevant state position Dec 12, 2022
@vlsi
Copy link
Contributor Author

vlsi commented Dec 12, 2022

I'm afraid it boils down to .shrink() must not be called "after the state has been modified" when action accesses state.

In other words, it is similar to #427, however, the difference is that the offending method is .shink() rather than .transformation().

@vlsi
Copy link
Contributor Author

vlsi commented Dec 12, 2022

The example above is

  • add 4 to []
  • add 6 to [4]
  • add 1 to [4, 6]
  • print 4 from [1, 4, 6] <-- shrinking of this action fails since the model state is empty as clear anyway below action was executed during the property run
  • clear anyway

@vlsi
Copy link
Contributor Author

vlsi commented Dec 14, 2022

It looks like the ways out are:
a) Sample (and keep) several .shrink() results for each action before the action is executed.
Pro: we would have valid shrink results (the shrink result would strictly correspond to the state)
Cons: it would spend time and memory on keeping .shrink() results even in case the chain succeeds.
Cons: it does not fully fit "try many different shrink types, order them, and start with the most promising ones" since we would be able to keep only a few shrink results for each action.

b) Variation of "a": first execute the chain as usual (no snapshotting, no shrinking), however, if the chain fails, then re-execute it and capture several .shrink() values for every action.
Pros: no overhead for "successful case"
Cons: it does not fully fit "try many different shrink types, order them, and start with the most promising ones" since we would be able to keep only a few shrink results for each action.
Cons: re-executing the chain might take significant time, and it might be better to just follow "a" and shrink early.

c) Delay shrinking till "re-execution" of the chain. In other words, when re-executing, try shrinking some of the actions before executing them. The difference from "a" is that we can't really weigh the shrink result before we start re-execution.
Pro: no need to shrink actions in advance
Pro: no CPU and RAM overhead in case the chain succeeds
Cons: it does not fit "try many different shrink types, order them, and start with the most promising ones" since the engine would have absolutely no visibility on which action to shrink first.

d) Enforce the state to be serializable (or externalizable in some way). Then the engine could snapshot the state before executing each action, and then it could restore from those snapshots in cases shrink is needed.
Pro: it might be reasonably hard to implement from the user's perspective
Pro: it would allow after-the-fact shrinking
Cons: the engine would probably need to snapshot every state (at least every time the state changes), so it would incur CPU overhead even for successful executions

e) Enforce users to clone all the data they need when producing state-dependent transformations.
Pro: if the transformation clones all the data it needs, then it could be safely recreated and shrunk even if the later actions have been applied to the state.
Cons: it might be really hard for the users to "clone all the data from the model"
Cons: it would be hard to debug since there will be virtually no way to tell if "model copying" is missing in one of the actions.

f) Variation of "e": enforce immutable approach for the state. For instance, if the user uses only immutable collections for the state (e.g. https://github.com/usethesource/capsule, https://github.com/andrewoma/dexx), then they are guaranteed to have an immutable state.
Pro: works in case the user can rewrite the code with immutable collections
Cons: the approach is not compatible with Consumer<State> mutate
Cons: the approach is not compatible with testing mutable data structures

WDYT?

For now, I incline to variation a.

One of my actions takes ~2 minutes, so I think it should be just fine to keep several shrink results in memory for every action even in case the chain succeeds.

vlsi pushed a commit to vlsi/jqwik that referenced this issue Dec 14, 2022
…ccess 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 jqwik-team#428
vlsi pushed a commit to vlsi/jqwik that referenced this issue Dec 14, 2022
…ccess 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 jqwik-team#428
vlsi pushed a commit to vlsi/jqwik that referenced this issue Dec 14, 2022
…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 jqwik-team#428
@jlink
Copy link
Collaborator

jlink commented Dec 22, 2022

@vlsi Could you add the code for SetMutatingChainState if you have it still lying around? Would simplify playing around with the options a bit.

@vlsi
Copy link
Contributor Author

vlsi commented Dec 22, 2022

Added

@jlink
Copy link
Collaborator

jlink commented Jan 5, 2023

I think a) is not a feasible option since prefetching shrinking results can access the state in behaviour changing way. This could lead to additional queries to a database or other external resources and thereby completely change the stateful object's behaviour. The same holds for b) unless the chain would be executed for each shrinking attempt.

c) would - as you note - require a different overall shrinking approach
d) is just not possible in Java - at least not in a generic way. May something like improveShrinkingWith(Serializer<T>) could be added to allow that. e) is the manifestation of this idea.
f) is more or less what I suggest below...

So my current idea is to move in two steps:

  1. Do not shrink a single transformation if it is accessing state and any following transformation changes state.
  2. Add a flag for chain creation - eg ActionChain.startWith(Supplier<T>, boolean isImmutableType).

Knowing to work with immutables would also make allow more powerful shrinking in some cases,
since immutable chains could use improveShrinkingWith(detector) by default.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

Do not shrink a single transformation if it is accessing state and any following transformation changes state.

All my actions access state, so discarding the tail after shrinking one of the elements is virtually useless as it would be almost equivalent to executing the head of the chain.

Add a flag for chain creation - eg ActionChain.startWith(Supplier, boolean isImmutableType)

Technically speaking, my state is mutable: a part of the state is within mutable objects, and a part of the state is in the database; so “is immutable” flag won’t help me.

I think a) is not a feasible option since prefetching shrinking results can access the state in behaviour changing way

I think it is reasonable to ask that .shrink methods should not mutate the state. Just like producing actions should not mutate the state even if it accesses the state for producing action. So prefetching shrink results is a workable from the technical point of view.

d) is just not possible in Java - at least not in a generic way. May something like improveShrinkingWith(Serializer) could be added to allow that

d is exactly about requiring the user to implement serialiser and deserialiser, so it is possible in Java modulo the fact that user would have to code it.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

However, it looks like shrinking of the individual actions is not helpful for stateful chains.

For instance, my case is somewhat comparable to testing a file system: one can create and remove files and folders.
Suppose the actions are createFile(parentFolder: Folder, name: String), createFolder(parentFolder: Folder, name: String), deleteFile(file: File), and so on.
Suppose the chain starts with createFolder that is not related to the identified bug. The ideal shrink should be able to remove the irrelevant createFolder, and it should keep the rest of the sequence intact. Removal is trivial, however, the rest actions won’t be able to keep the same pattern. For instance, Arbitrary that produces createFile would likely use Arbitrarties.of(state.folders), so removal of any of the folders would shuffle the generated outcome, and the resulting chain would be very far away from the initial one.

In other words, even if someone implements serializer-based or immutable-based shrink improvements, it won’t really help me as the shrinking time would be unacceptably long.

I wonder if there could be a design where actions refer to another actions. Suppose that Arbitrary that produces createFile(parentFolder: Folder, name: String) would stick to an action that generated the parent folder rather than selecting a folder from a “all folders” collection.
In that case, removals of actions that have no dependants would be simpler, and it would keep the shape of the chain.

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

Do not shrink a single transformation if it is accessing state and any following transformation changes state.

All my actions access state, so discarding the tail after shrinking one of the elements is virtually useless as it would be almost equivalent to executing the head of the chain.

Actions could still be shrunk away, just not be shrunk individually.

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

However, it looks like shrinking of the individual actions is not helpful for stateful chains.

For instance, my case is somewhat comparable to testing a file system: one can create and remove files and folders. Suppose the actions are createFile(parentFolder: Folder, name: String), createFolder(parentFolder: Folder, name: String), deleteFile(file: File), and so on. Suppose the chain starts with createFolder that is not related to the identified bug. The ideal shrink should be able to remove the irrelevant createFolder, and it should keep the rest of the sequence intact. Removal is trivial, however, the rest actions won’t be able to keep the same pattern. For instance, Arbitrary that produces createFile would likely use Arbitrarties.of(state.folders), so removal of any of the folders would shuffle the generated outcome, and the resulting chain would be very far away from the initial one.

State in Java can potentially have so many side-effects that relying on any library to fully handle it, it just not feasible. In the example above, I'd probably switch from naive state-based properties to a model-based approach (https://johanneslink.net/model-based-testing). If the model itself is implemented as immutable object than most problems just disappear.

I wonder if there could be a design where actions refer to another actions. Suppose that Arbitrary that produces createFile(parentFolder: Folder, name: String) would stick to an action that generated the parent folder rather than selecting a folder from a “all folders” collection. In that case, removals of actions that have no dependants would be simpler, and it would keep the shape of the chain.

Which gets us to #300. Haven't checked if it can be done as easily with the new stateful properties.

In general, putting more implementation complexity on top of the existing one - like pre-calculating a configurable amount of shrinks or dealing with hand-made (de-)serializers - to just cover a few more edge cases but not solving the overall problem, is not where I want to go at the moment. That probably means that I will release 1.7.2 without a solution to this issue.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

Actions could still be shrunk away they could just not be shrunk individually.

What do you mean?
Of course, I understand that labels and values for individual actions could be shrinked/simplified, however, the most important is to reduce the number of steps, and shrink the values only after all the irrelevant steps are removed. Otherwise the engine would spend too much time on trying to figure out the best label, however, it won't make the case easier to understand.

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

Actions could still be shrunk away they could just not be shrunk individually.

What do you mean? Of course, I understand that labels and values for individual actions could be shrinked/simplified, however, the most important is to reduce the number of steps, and shrink the values only after all the irrelevant steps are removed. Otherwise the engine would spend too much time on trying to figure out the best label, however, it won't make the case easier to understand.

The access to the wrong state happens during individual shrinking - shortest filename etc. That's the part that leads to the IllegalStateException in the test above. If that is left out, then this particular problem goes away. Others might hide underneath, though.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

If the model itself is implemented as immutable object than most problems just disappear.

I don't understand how immutability or "model-based-testing" improves shrinking for cases like "testing filesystem with files and folders".
The article you refer to uses the same index % users.size() approach, so removal of any user from a collection would break the shape of all the subsequent actions, and it would likely fail to reproduce the original failure.

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

The article you refer to uses the same index % users.size() approach,

That's because the example uses the old stateful API. With the new one it could directly choose from an existing user. The article also does not implement an immutable model, which would be necessary for working well with the new API.

The advantage of an explicit model is that all shrinking can be done without accessing the real thing (eg a database or a file system). Thus, it's usually faster and does not have side effects.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

The access to the wrong state happens during individual shrinking - shortest filename etc

That is helpful, however, I would consider shrinking individual names only after the eninge figures out the minimal number of steps to reproduce the failure.

It just makes no sense trying to shrink all individual file names, especially, if the file was not needed for reproducing a bug.

Here's a bug I have in mind: suppose a filesystem crashes when two different folders contain file with the same name.

The chain that reproduces the crash might look like

createFolder(parentFolder = null, name="A") // <-- non-essential for the case
createFolder(parentFolder = null, name="B")
createFile(parentFolder = null, name="fileA") // <-- non-essential for the case
createFile(parentFolder = "B", name="fileC")
createFolder(parentFolder = "A", name="D")
createFile(parentFolder = "D", name="fileC") // <-- crash, because B/fileC and D/fileC collide

I would like that shrinker removes createFolder A and createFile fileA actions since they are not essential for the failure.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

With the new one it could directly choose from an existing user

How? I really do not understand it. I use the new stateful API, and I use Arbitraries.of(Collection) when I need an element of a collection. Of course, its implementation boils down to net.jqwik.engine.properties.arbitraries.randomized.RandomGenerators#chooseValue which is effectively the same as index % users.size(). Is there a better API?

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

The access to the wrong state happens during individual shrinking - shortest filename etc

That is helpful, however, I would consider shrinking individual names only after the eninge figures out the minimal number of steps to reproduce the failure.

I think you make my case :-) That's why I consider the safe guard I suggested as first implementation change still worthwhile. It will only prevent individual shrinking of actions not shrinking them away.

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

How? I really do not understand it. I use the new stateful API, and I use Arbitraries.of(Collection) when I need an element of a collection.

But you do not use an explicit, immutable model, right? Your actions are based on the real SUT's state as far as I understand. Which leads to many (or all) of the potential shrinking problems.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

But you do not use an explicit, immutable model, right? Your actions are based on the real SUT's state as far as I understand

My current state is a bunch of MutableMap (which backs Kotlin DSL for describing the test) + database state.

Frankly speaking, I believe another approach would be "manual" shrinking.
In other words, I incline to implement converter from Chain to my Kotlin DSL test code. Then I would be able to comment irrelevant bits of the test code. At the same time, a structured test code would be easier to reason than a flat list of actions.

@jlink
Copy link
Collaborator

jlink commented Jan 6, 2023

Frankly speaking, I believe another approach would be "manual" shrinking.
In other words, I incline to implement converter from Chain to my Kotlin DSL test code. Then I would be able to comment irrelevant bits of the test code. At the same time, a structured test code would be easier to reason than a flat list of actions.

That's maybe a new and interesting approach. I haven't seen a structural approach to stateful generations of actions anywhere. Go for it! If it's best implemented on top of chains or on top of plain jqwik, I cannot really tell. Most complexity in chain implementation comes from shrinking, so I assume that manual shrinking could do without some of it.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

I haven't seen a structural approach to stateful generations of actions anywhere

I mean more like unparse to a structured form rather than "use structural approach for generation".

For reference, here's a test case crafted manually.
I would unparse actions like "add table", "set attribute statistics" to the DSL like below. Of course, the unparser would have to be tailored to my custom DSL to make the resulting code look nice, however, I believe a generic "print chain as a test code" would be helpful anyway, since it is basically the only way to pin regression tests.
seed values do not really help for regression testing, since having fixed seeds can't survive changing weights, or adding/removing test actions. In other words, if I improve the generated actions, old tests with fixed seeds change their semantics completely.

open class SeveralTypeColumnTestSuite : BaseTest() {
    @Test
    override fun test() {
        val mapping = createMapping<Mapping>()
    
        mapping.applyModifyAndApply {
            table.configure { // This is effectively "add column to table" action
                "TEXT"(textAttribute)
            }
        }
    }

    open class InitialMapping : BaseMapping() {
        lateinit var table: PlainTable
        val textAttribute by textAttribute()

        val type1 by objectType()
        val type2 by objectType()

        override fun Mapping.init() {
            configFiles {
                configFile("test") {
                    statistics {
                        statistic(attribute = textAttribute, numRows = 100)
                        statistic(objectType = type1, numRows = 100)
                        statistic(objectType = type2, numRows = 100)
                    }
                    odb {
                        tables {
                            table("TEST_UT_S__MAP_TO_OT1") {
                                +type1
                                "TYPE"(ObjectField.OBJECT_TYPE_ID)
                            }
                            table = table("TEST_UT_S__MAP_TO_OT2") {
                                +type2
                                "TYPE"(ObjectField.OBJECT_TYPE_ID)
                                "TYPE2"(ObjectField.OBJECT_TYPE_ID)
                                statistics(numRows = 100)
                                threshold(attribute = 1, objectType = 1, table = 1)
                            }
                        }
                    }
                }
            }
        }
    }
}

@vlsi
Copy link
Contributor Author

vlsi commented Jan 6, 2023

I've created #447 for tracking "generate test source code" feature.

@jlink
Copy link
Collaborator

jlink commented Jan 10, 2023

Here's a sketch of my idea to allow more differential specification of transformer based stateful chains.

Introduction of new class ChainSpec:

class ChainSpec<T> {
  boolean isImmutable;
  boolean hasSideEffects;
  Supplier<ChangeDetector<T>> detectorSupplier;
  // Maybe more attributes eg Serializer/Deserializer for state

  // Some static factories, e.g.
  static <T> ChainSpec<T> immutable() { ... }
  static <T> ChainSpec<T> mutableWithoutSideEffects() { ... }
}

Default would be mutable, with side effects and no change detector. This covers everything, but has the worst shrinking behaviour.

Now, Chain.startWith can get an optional second argument:

class Chain {
  static <T> ChainArbitrary<T> startWith(Supplier<? extends T> initialSupplier, ChainSpec<T> spec) {...}
}

With spec in place we could get rid of ChainArbitrary.improveShrinkingWith(Supplier<ChangeDetector>).

@vlsi
Copy link
Contributor Author

vlsi commented Jan 13, 2023

I am indifferent to ChainSpec, as it does not help shrinking in my case.

@jlink
Copy link
Collaborator

jlink commented Jan 13, 2023

I am indifferent to ChainSpec, as it does not help shrinking in my case.

I think it could in two ways:

  • Use an immutable, side-effect-free model-based approach
  • Use the spec to insert (de-)serialiser for enabling more shrinking steps in actions on mutable state

@vlsi
Copy link
Contributor Author

vlsi commented Jan 13, 2023

Use an immutable, side-effect-free model-based approach

I do not see how immutable, side-effect-free model-based approach can help me.

Use the spec to insert (de-)serialiser for enabling more shrinking steps in actions on mutable state

I do not see how it helps.

I described a sample case that is close to the testing problem I have: #428 (comment)
I do not see how immutable, side-effect-free model-based approach and/or (de-)serialiser for enabling more shrinking steps in actions on mutable state would help shrinking createFile, createFolder actions.

@jlink
Copy link
Collaborator

jlink commented Jan 21, 2023

I tried out your file system example with an immutable model here: https://github.com/jlink/pbt-stateful-examples
Shrinking is not as good as I'd like it to be, but the issue of having wrong state-objects being called during shrinking goes away.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 21, 2023

Of course, wrong-state object issue goes away, however, shrinking goes away as well 🤷

@jlink
Copy link
Collaborator

jlink commented Jan 22, 2023

Of course, wrong-state object issue goes away, however, shrinking goes away as well 🤷

This issue is about fixing the bug without making shrinking worse. That’s what the approach achieves.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 22, 2023

That's fair.
When I created the issue, I thought resolving it would enable me to activate shrinking.
Unfortunately the shrinking is inefficient, so I have to keep it disabled.

For now, I am more interested in exploring the way shrinking could survive removal of elements in collections.

@jlink
Copy link
Collaborator

jlink commented May 14, 2024

Closing since jqwik 2 will generate actions more robustly and with (hopefully) much better shrinking behaviour.

@jlink jlink closed this as completed May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants