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

State-Dependent (Ad-Hoc) Action Generator #134

Closed
DirkToewe opened this issue Nov 22, 2020 · 68 comments
Closed

State-Dependent (Ad-Hoc) Action Generator #134

DirkToewe opened this issue Nov 22, 2020 · 68 comments

Comments

@DirkToewe
Copy link
Contributor

Testing Problem

Let's say You want to test the List<E>::add(int, E) method of a custom implementation of a List. Since List is stateful, action sequences would be perfect for testing. So naively, a provider could be implemented as follows:

@Provide
Arbitrary<ActionSequence<List<String>>> actions()
{
  var insert = Arbitraries.integers().greaterOrEqual(0).flatMap( idx ->
               Arbitraries.strings()                       .map( val -> new Action<List<String>>()
  {
    public boolean precondition( List<String> list ) { return list.size() >= idx; }
    public List<String> run( List<String> list ) {
      list.add(idx,val);
      return list;
    }
    public String toString() { return format("add(%d,%d)", idx, val); }
  }));

  return Arbitraries.sequences(insert);
}

This will however very likely fail or run indefinitely, as idx is out of bounds most of the time and precondition evaluates to false
almost all of the time.

Suggested Solution

Would it be possible to support the creation of action sequences via a function of type State => Arbitrary<Action<State>>? Something along the lines of:

@Provide
Arbitrary<ActionSequence<List<String>>> actions()
{
  return Arbitraries.sequenceGen(                              list ->
         Arbitraries.integers().between(0,list.size()).flatMap( idx ->
         Arbitraries.strings()                            .map( val -> new Action<List<String>>()
  {
    public List<String> run( List<String> list ) {
      list.add(idx,val);
      return list;
    }
    public String toString() { return format("add(%d,%d)", idx, val); }
  })));
}

Where Arbitraries.<M>sequenceGen() would accept a Function<? super M, ? extends Arbitrary<Action<M>> as argument. The choice actions would then depend the current state. sequenceGen would be ideal for testing any kind of collection type like lists, maps, sets, ...

@jlink
Copy link
Collaborator

jlink commented Nov 23, 2020

@DirkToewe Thanks for bringing that up. The idea to generate the next step of a sequence based on a model's state is a good one and would facilitate some type of state-based testing. However, it does not fit together with the way jqwik is currently generating and shrinking sequences and would require more or less a re-implementation of the stateful package.

So the feature is on my long-term list but not yet prioritized.

Not all is lost, though. The work-around is to keep an ordered list of possible IDs in your model and generate a random index to that list. If the random index exceeds the length of the list you wrap it around using modulo arithmetic. I demonstrated the approach here:
https://johanneslink.net/model-based-testing/#action-create-a-post

I hope that helps. Feel free to get back to me if that doesn't solve your problem.

@DirkToewe
Copy link
Contributor Author

That would have been my idea for a Workaround as well. One concern I had with that is edge cases: Wouldn't values like 0 and Integer.MAX_VALUE % list.size() be tested much more frequently than for example the important edge case of list.size() (appending to the end of the list)?

@jlink
Copy link
Collaborator

jlink commented Nov 23, 2020

Edge cases will have a higher probability regardless. I'd also argue that trying the first and last element more often is a reasonable thing to do. That said, being able to switch edge case generation off for individual arbitraries is quite on top of jqwik's feature list. Currently planned for 1.4.0.

What may matter more IMO is a uniform distribution of indices. You can enforce uniform distribution of index like that

Arbitrary<Integer> index = Arbitraries
	   .integers()
	   .between(0, MAX_SIZE)
	   .withDistribution(RandomDistribution.uniform());

You may want to watch the statistics to see if distribution of idices is ok for you.

@vlsi
Copy link
Contributor

vlsi commented Feb 2, 2022

I see the same index % list.size() approach is suggested in https://johanneslink.net/model-based-testing/#action-create-a-post


I'm facing a related testing case: my test model is nested like configuration_file -> table_definition -> column_definition, so there might be 0..n configuration_file instances which contain 0..m table_definition instances, which contain 1..k column_definition instances.

Then actions like RemoveTable action makes sense only when there's at least one table. However, if mapping via % list.size(), then it could map to a configuration_file that has no tables.

I'm inclined to do the following: resolve Arbitrary<Integer> index into entity id right in precondition (!) method.
In other words, my precondition would try something like index % list.size() first, and if that value does not work, then it would try the other items, and it would store the result into the field of my Action.

That would rely on the fact that precondition is always called before running the action, however, I believe it would simplify the code, as I basically have very similar checks like "precondition() == verify that config file at given configFileIndex contains at least one table", and then in run I need to access exactly the same config file at given configFileIndex.

I think that would have the following good properties:

  1. There will be less code duplication between precondition() and run()
  2. The action would be more robust in face of shrinking. In other words, when the shrinker runs the action, the action might skip index % list.size() wrapping, and the action could reuse the previous entity ids if possible, so it would follow the same modifications to the state. If we still use index % list.size() during the shrink phase, then we could alter the meaning of actions significantly, and it might fail to shrink the case.
  3. toString could include the resolved entity ids, so test failure would include human-understandable entity ids/names instead of random index values.

@jlink
Copy link
Collaborator

jlink commented Feb 3, 2022

That said, being able to switch edge case generation off for individual arbitraries is quite on top of jqwik's feature list. Currently planned for 1.4.0.

BTW, this has been possible now for a while: Arbitrary.withoutEdgeCases()

@vlsi
Copy link
Contributor

vlsi commented Feb 4, 2022

The plan of abusing precondition was good on paper, however, it does not work in practice since jqwik re-instantiates values in net.jqwik.engine.properties.shrinking.CombinedShrinkable#value (since it calls createValue(parts))

@vlsi
Copy link
Contributor

vlsi commented Feb 8, 2022

I tried yet another approach: expose state into a class field before calling sequenceArbitrary.run(state).
That allows generators to use the current state as they generate values, so they can lookup the available values.

It almost works, and now I am able to generate basic actions.

The findings so far:

  1. Stateful generators should be re-instantiated for each try, however, jqwik caches the generator since DefaultActionSequenceArbitrary#generator(int, boolean) delegates to the default implementation that calls memoizedGenerator. I had to override the method and avoid caching there.

I wonder if some of the nested generators are cached.
It would be great to somehow specify that given arbitrary should not be cached (or provide an extra caching key).

  1. Sometimes, the generated values are not very compatible. In the "random index" based approach invalid actions would be filtered out via precondition, however, as I use Arbitraries.of(valuesFromCurrentState), jqwik might fail in net.jqwik.engine.properties.arbitraries.randomized.RandomGenerators#choose(java.util.List<U>) when given list is empty.

For instance, I have the following:

frequencyOf(
  10 to `add table`(),
  1 to `remove table`(),
)

Of course, it makes no sense to remove a table in case the tables are missing, however, frequency can just select remove table as the first action.

My solution is to use frequencyOf(...).ignoreException(NotFoundException::class.java)) and a static exception which I throw from arbitraries when they are not compatible.

  1. jqwik instantiates arbitraries before launching the property, so I can't access currentModel class field in my arbitraries.

My workaround is to use just(true).flatMap { Arbitraries.of(currentModel.tables.names) } instead Arbitraries.of(currentModel.tables.names). If I use lasy {...} then the generator is cached, and cachig is wrong for me since my generators depend on the current model state.

  1. Stateful generation does not require index->name mapping. I can just use values from the state, so Actions are easier to write 💪

  2. Code structure for generating arbitraries is still a question to me (especially when the action needs values from different levels of the hierarchy and when random selection is needed for them) :-/

@jlink
Copy link
Collaborator

jlink commented Feb 9, 2022

For instance, I have the following:

frequencyOf(
  10 to `add table`(),
  1 to `remove table`(),
)

Of course, it makes no sense to remove a table in case the tables are missing, however, frequency can just select remove table as the first action.

Would CompositeAction from #300 help in this case?

@vlsi
Copy link
Contributor

vlsi commented Feb 9, 2022

Would CompositeAction from #300 help in this case?

I do not think so.
I do want to have remove table() as one of the isolated actions (e.g. add table A, add table B, remove table A).

@jlink
Copy link
Collaborator

jlink commented Feb 9, 2022

  1. Stateful generators should be re-instantiated for each try, however, jqwik caches the generator since DefaultActionSequenceArbitrary#generator(int, boolean) delegates to the default implementation that calls memoizedGenerator. I had to override the method and avoid caching there.

What effect did you observe that came through caching? The generated result should be independent of caching so there may be something else going on...

@jlink
Copy link
Collaborator

jlink commented Feb 9, 2022

My workaround is to use just(true).flatMap { Arbitraries.of(currentModel.tables.names) } instead Arbitraries.of(currentModel.tables.names). If I use lasy {...} then the generator is cached, and cachig is wrong for me since my generators depend on the current model state.

4. Stateful generation does not require index->name mapping. I can just use values from the state, so Actions are easier to write 💪

Does shrinking still work with this approach?

@vlsi
Copy link
Contributor

vlsi commented Feb 9, 2022

What effect did you observe that came through caching?

I think the observed behavior was "add column" was unable to find a workable table name (it observed "empty state" always), however, now I reverted to regular sequences(frequencyOf(...) and lazy { ... }.flatMap { } and it still seems to work.

Does shrinking still work with this approach?

Shrinking seems to work in a sense of "excluding actions".
I have not verified if shrinking/edge cases work in terms of "1-char table name" vs "max limit table name".

@jlink
Copy link
Collaborator

jlink commented Feb 10, 2022

After all this discussion state-dependent generation seems a worthwhile feature for the near future. It's a major change though. My current plan is to do it for 1.7, the development of which I hope to start before Easter.

@vlsi
Copy link
Contributor

vlsi commented Feb 10, 2022

It looks like I figured out my way through generating and applying the nested dependent actions, and I would proceed with actions that do call the database to see how all this works.

I ended up using a more-or-less single class for all the actions, and I use a builder function to generate them:

fun <T> action(
    description: () -> String,
    validate: T.() -> Boolean,
    body: T.() -> Unit
)...


@Provide
fun mappingActions() = StatefulActionSequenceArbitrary(
    frequencyOf(
        1 to just(ClearMappingAction),
...
        // Arbitrary<T>.perform is basically combine(this, that) { this, that -> this.perform(that) }
        4 to arbitraryTable().perform(`update object field`()),
    ).ignoreException(NotFoundException::class.java)
)

fun `update object field`() =
    frequency(
        1 to ObjectField.OBJECT_ID,
        5 to ObjectField.NAME,
    ).map { field ->
        action<ColumnScope>(
            description = { "UseObjectField($field)" },
            validate = { true }
        ) {
            rawValue = field
        }
    }

So far the generated sequence seems to use actions I supply, and it seems to reuse the names appropriately.

@vlsi
Copy link
Contributor

vlsi commented Feb 15, 2022

It looks like shrinking does not work.

com.example.jqwik.NotFoundException
	at com.example.mapping.SampleProperty.getNOT_FOUND(SampleProperty.kt:50)
	at com.example.mapping.SampleProperty.arbitraryPlainTable(SampleProperty.kt:298)
	at com.example.mapping.SampleProperty.arbitraryPlainTable$lambda-19(SampleProperty.kt:303)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$0(FlatMappedShrinkable.java:24)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$1(FlatMappedShrinkable.java:30)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.generateShrinkable(FlatMappedShrinkable.java:39)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.shrinkable(FlatMappedShrinkable.java:83)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.distance(FlatMappedShrinkable.java:88)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:553)
	at net.jqwik.api.ShrinkingDistance.combine(ShrinkingDistance.java:38)
	at net.jqwik.engine.properties.shrinking.CombinedShrinkable.distance(CombinedShrinkable.java:54)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.distance(FlatMappedShrinkable.java:88)
	at net.jqwik.engine.properties.shrinking.IgnoreExceptionShrinkable.distance(IgnoreExceptionShrinkable.java:39)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:553)
	at net.jqwik.api.ShrinkingDistance.forCollection(ShrinkingDistance.java:28)
	at com.example.mapping.stateful.ShrinkableActionSequence.createShrinkableActionSequence(ShrinkableActionSequence.kt:60)
	at com.example.mapping.stateful.ShrinkableActionSequence.shrinkSequenceOfActions$lambda-0(ShrinkableActionSequence.kt:38)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.HashMap$KeySpliterator.tryAdvance(HashMap.java:1642)
	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:169)
	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: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:129)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.lambda$shrink$3(PropertyShrinker.java:86)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.shrink(PropertyShrinker.java:88)
	at net.jqwik.engine.properties.shrinking.PropertyShrinker.shrink(PropertyShrinker.java:72)
	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:156)
	at net.jqwik.engine.execution.PropertyMethodExecutor.executeMethod(PropertyMethodExecutor.java:135)
	at net.jqwik.engine.execution.PropertyMethodExecutor.lambda$executePropertyMethod$1(PropertyMethodExecutor.java:115)
	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.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)

The following looks suspicious:

	at net.jqwik.engine.properties.shrinking.IgnoreExceptionShrinkable.distance(IgnoreExceptionShrinkable.java:39)

Do you think net.jqwik.engine.properties.shrinking.IgnoreExceptionShrinkable#distance should ignore the exception rather than throw it from distance?


I guess the use case is if the shrinker removes "add table" action, then all the other actions that "modify the table" can't really succeed, and they can't "shrink" because the table they were about to modify just does not exist.

@jlink
Copy link
Collaborator

jlink commented Feb 16, 2022

Do you think net.jqwik.engine.properties.shrinking.IgnoreExceptionShrinkable#distance should ignore the exception rather than throw it from distance?

Arbitrary.ignoreException is about ignoring exception during value generation.
My guess is that in this case the exception comes from arbitraryTable().perform(update object field()), i.e. during arbitrary creation. Maybe you can arrange for this code to never fail but return a fixed arbitrary instead?

In the end, it's all a hack anyway, I'm afraid. Which will hopefully no longer be necessary with the introduction of the new state-dependent generation. I've already done a POC that looks promising. I hope to find time for further exploration this weekend.

@vlsi
Copy link
Contributor

vlsi commented Feb 16, 2022

My guess is that in this case the exception comes from arbitraryTable().perform(update object field()), i.e. during arbitrary creation

perform is a combine, so the creation itself is just fine. The instantiation of arbitraries does not seem to be an issue here.

The stacktrace clearly says that exception happens when ShrinkableActionSequence calls ShrinkingDistance.forCollection.

I have my own copy of stateful package which is effectively the same code as in jqwik.

Here's what happens:

  1. Initially jqwik generates actions. Note that it generates actions one by one (within initialRun), and it applies actions immediately to the model.
    That gives opportunity to other actions to see the updates and generate values based on the past model updates

  2. Then shrinker comes in, and it tries to shrink the thing.
    The problem is that net.jqwik.engine.properties.stateful.ShrinkableActionSequence#createShrinkableActionSequence attempts to evaluate distance for all the shrinkable actions without applying them to the model.


Suppose the actions were:

  1. Add table "users" (users is an arbitrary-generated name)
  2. Add a column to an arbitrary table (e.g. find an arbitrary existing table, flatmap it with "arbitrary name", and add the column)

That set of actions works well in the initial generation because "add column" finds a usable users table in the model, and that is fine. In other words, the arbitrary that suggests "table for adding columns" has a table (added by the first action) to work with.

However, when shrinker comes in place, it calls .distance() for both add table and add column actions. Unfortunately, the model is not really ready to "generate" or "shrink" add column action because there is not a single table present in the model (because jqwick calls .distance() before applying any actions).


So far I replaced ShrinkingDistance.forCollection with ShrinkingDistance.of(list.size.toLong()), and the exceptions from shrinker are gone (and the shrinker is able to remove irrelevant actions in certain cases)

@jlink
Copy link
Collaborator

jlink commented Feb 16, 2022

So far I replaced ShrinkingDistance.forCollection with ShrinkingDistance.of(list.size.toLong()), and the exceptions from shrinker are gone (and the shrinker is able to remove irrelevant actions in certain cases)

Where exactly did you replace it?

@jlink
Copy link
Collaborator

jlink commented Feb 16, 2022

Well, the code is a mess complicated because stateful.

I wonder if it's better to make ShrinkingDistance.forCollection and ShrinkingDistance.combine robust against exceptions during embedded Shrinkable.distance calls. Or if that would hide bugs in user code.

@vlsi
Copy link
Contributor

vlsi commented Feb 16, 2022

I wonder if it's better to make ShrinkingDistance.forCollection and ShrinkingDistance.combine robust against exceptions during embedded Shrinkabel.distance calls

Frankly speaking, that was my first idea, however now I am inclined hiding exceptions won't help.

Here's another stacktrace:

com.example.jqwik.NotFoundException
	at com.example.mapping.SampleProperty.getNOT_FOUND(SampleProperty.kt:51)
	at com.example.mapping.SampleProperty.arbitraryInstance(SampleProperty.kt:313)
	at com.example.mapping.SampleProperty.arbitraryInstance$lambda-15(SampleProperty.kt:318)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$0(FlatMappedShrinkable.java:24)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$1(FlatMappedShrinkable.java:30)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.generateShrinkable(FlatMappedShrinkable.java:39)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.shrinkable(FlatMappedShrinkable.java:83)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.distance(FlatMappedShrinkable.java:88)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.reduce(ReferencePipeline.java:553)
	at net.jqwik.api.ShrinkingDistance.combine(ShrinkingDistance.java:38)
	at net.jqwik.engine.properties.shrinking.CombinedShrinkable.distance(CombinedShrinkable.java:54)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.shrinkRightSide(FlatMappedShrinkable.java:52)
	at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.shrink(FlatMappedShrinkable.java:45)
	at net.jqwik.engine.properties.shrinking.IgnoreExceptionShrinkable.shrink(IgnoreExceptionShrinkable.java:24)
	at com.example.mapping.stateful.ShrinkableActionSequence.shrinkActionsOneAfterTheOther(ShrinkableActionSequence.kt:46)

The issue seems to be that it is not valid to call shrink on a shrinkable that has no model.
The model itself requires applying the previous actions.
However, before applying the actions, we need to sort them so we try "simplest sequences first".
That creates a loop :)

The way out might be rework net.jqwik.engine.properties.stateful.ShrinkableActionSequence#shrink in the following way:

  1. The distance for the sequence could be just its length (or its length + the distance of the first action)
  2. .shrink should be called only one item at time. For instance, instead of eagely creating lots of sequences in shrinkActionsOneAfterTheOther, generate something like generateIndexToBeShrunk().flatMap { shrinkItemIndex -> return a sequence that will call shrink at shrinkItemIndex }

WDYT?

@jlink
Copy link
Collaborator

jlink commented Feb 16, 2022

Without having dived into that too deeply, flat-mapping is always making shrinking much worse. The upcoming approach will have to do some flat-mapping anyway in some circumstances, but probably with more knowledge about runtime dependencies. Doing ad-hoc modifications to distance metrics and shrinking might improve the situation for one use case but deteriorate it for another. I'd rather invest my time in the new foundation than mending a soon to be replaced one.

@vlsi
Copy link
Contributor

vlsi commented Feb 16, 2022

Doing ad-hoc modifications to distance metrics and shrinking might improve the situation for one use case but deteriorate it for another

I believe ShrinkableActionSequence.distance is not really used anywhere.
ActionSequence is a top-level shrinkable, so noone really would call its distance method.

jqwik seems to use shrink suggestions in the exact order returned from ShrinkableActionSequence#shrink method.

@jlink
Copy link
Collaborator

jlink commented Feb 16, 2022

ShrinkableActionSequence.distance will be used when a stateful property has more than one for-all parameter.

@vlsi
Copy link
Contributor

vlsi commented Feb 17, 2022

Unfortunately, Arbitraries.of(valuesFromCurrentState) does not seem to support shrinking.
It has effectively the same shrinking pitfalls as index % model.length mapping: if one of the first actions is removed from the sequence, then subsequent actions that used that value do not know they no longer apply.

The code is easier to read and write, however, shrinking behaviour is still bad.

For instance:

add user 1
add user 2
add user 3
add post for user 2 <-- Here jqwik picks a random seed, and takes an existing user from the model. Suppose the random was 2, and it ended up with user 2 (because it was the second on the list)


Then, if shrinker removes the first action, the sequence becomes:
add user 2
add user 3
add post for user 3 (!). The seed is still 2 (because jqwik wants to keep actions intact), so it maps to the second item in the list that happens to be user 3 now :-/

That means "action removal" might augment all the subsequent actions.

If jqwik keeps shinkable.value() results (so it keeps action with user 2), then it won't survive "user renaming".
In other words, if we "shrink add user 2 action into add user 0", then the subsequent add post for user 2 won't know it should update to add post for user 0.

I've no idea how to heal that yet.

@jlink
Copy link
Collaborator

jlink commented Feb 17, 2022

Arbitraries.of(..) tries to shrink towards values on the left. The values themselves cannot be shrunk, though.

IMO you should either try to create your actions without reference to the state (e.g. through s.t. like the modulo approach together with more complex actions) or wait till state-dependent generation becomes available. I don't figure there's a sane way to add state-dependency ontop of the current implementation.

@jlink
Copy link
Collaborator

jlink commented Feb 24, 2022

Work on this issue, aka the new stateful support, started on https://github.com/jlink/jqwik/tree/new-stateful

@vlsi
Copy link
Contributor

vlsi commented May 8, 2022

Have you considered merging predicate(State) and transformer(..) methods?
For instance, what if transformer(State) could return something like Arbitraries.empty()?

@jlink
Copy link
Collaborator

jlink commented May 8, 2022

Have you considered merging predicate(State) and transformer(..) methods?
For instance, what if transformer(State) could return something like Arbitraries.empty()?

That was my first approach. I re-introduced preconditions because they enable much better shrinking.

@vlsi
Copy link
Contributor

vlsi commented May 8, 2022

they enable much better shrinking

Could you suggest some pointers?

I understand that trivial actions that have no preconditions and access no state for generating Arbitrary<Transformer<..>> might be easier to shrink. However, I do not understand how precondition(State) + transformer(State) is different from a single transformer(State) method.

The context of the question is that I struggle with supporting nested data types with the new API.

Suppose the structure is as follows:

class City {
  Map<HouseId, House> houses;
}

class House {
  Map<RoomId, Room> rooms;
}

class Room {
  Map<LampId, Lamp> lamps;
}

class Lamp {
  boolean isTurnedOn;
  Color color;
}

Here are the actions I would like to implement:

  1. Switch on an arbitrary lamp (the lamp has to be off before switching it on)
  2. Change the color of an arbitrary lamp (suppose it works with any lamp)

For instance:

class SwitchOnLampAction<City> implements Action<City> {
  boolean predicate(City city) {
    return city.houses.values()
      .stream().anyMatch(house ->
        house.rooms.values()
          .stream().anyMatch(room ->
            room.lamps.values().stream().anyMatch(lamp -> !lamp.isTurnedOn)));
  }

  Arbitrary<Transfomer<City>> transformer(City city) {
    Stream<Lamp> turnedOffLamps = city.houses.values()
      .stream().flatMap(house ->
        house.rooms.values()
          .stream().flatMap(room ->
            room.lamps.values().stream().filter(lamp -> !lamp.isTurnedOn)));
     return Arbitraries.of(turnedOffLamps).map(
        lamp -> Transformer.mutate(
           () -> "activate lamp " + lamp + ", in room " + room + ", in house " + house + ", in city " + city
           state -> { lamp.isTurnedOn = true }
        )
     )
  }
}

a) The code that "searches for an arbitrary lamp within an arbitrary room within an arbitrary house..." quickly becomes duplicated across many actions.
b) The code that checks "if there's at least one lamp that is turned off" seems to duplicate logic in Arbitrary<Transformer<..>> transformer(State)
c) "lamp switch on transformer" description includes full path (or even ids) of the room, house, city, and the logic of building the description would also become duplicated across actions.

That is why I was considering moving "predicate" into the transformer method (or even into Transformer interface)

I am trying to implement something like

class LampSwitchOnTransformer implements ./*?*/..Transfomer<Lamp> {
   String description() {
     return "switch on lamp " + lamp;
   }

   boolean predicate(Lamp lamp) {
     return !lamp.isTurnedOn;
   }

   void/*?*/ perform(Lamp lamp) {
      lamp.isTurnedOn = true;
   }
}

And then it would be nice to spawn it via something like new WithArbitraryLamp(new LampSwitchOnTransformer()).
In other words, I would like to factor the logic of finding the subjects from the logic that modifies them.

@jlink
Copy link
Collaborator

jlink commented May 9, 2022

Could you suggest some pointers?

An action with a precondition but no access to the state for providing a transformer can be handled in shrinking like an action without any state-access at all. If the precondition is no longer valid the shrinking candidate will be discarded on evaluation of precondition.

But you can always return a NOOP transformer if you want to. Maybe introducing something like Transformer.noop() could enable jqwik to do some improved shrinking on it.

@vlsi
Copy link
Contributor

vlsi commented May 9, 2022

An action with a precondition but no access to the state for providing a transformer can be handled in shrinking like an action without any state-access at all

I see. However, if the action accesses state for providing a transformer, then a separate precondition method does not seem helpful 🤷‍♂️

@jlink
Copy link
Collaborator

jlink commented May 9, 2022

I see. However, if the action accesses state for providing a transformer, then a separate precondition method does not seem helpful 🤷‍♂️

Not sure. A precondition is a different concern than the actual provisioning. Having Transformer.noop() could serve both approaches.

@vlsi
Copy link
Contributor

vlsi commented May 9, 2022

I agree Transformer.noop sounds useful, especially if Arbitrary.just(Transformer.noop()) would be automatically detected as an empty action.

@vlsi
Copy link
Contributor

vlsi commented May 9, 2022

By the way, if jqwik detected "empty arbitrary", then a precondition for UpdateValue in your sample won't be needed.

state.keys() will be empty, so Arbitraries.of(state.keys()) will be empty, and combine would yield "empty arbitrary", so jqwik could understand the action does not apply with this state.

	static class UpdateValue implements Action<MyStore<Integer, String>> {
		@Override
		public Arbitrary<Transformer<MyStore<Integer, String>>> transformer(MyStore<Integer, String> state) {
			Arbitrary<Integer> existingKeys = Arbitraries.of(state.keys());
			return Combinators.combine(existingKeys, values())
							  .as((key, value) -> Transformer.mutate(
								  String.format("update %s=%s", key, value),
								  store -> {
									  store.store(key, value);
									  assertThat(store.isEmpty()).isFalse();
									  assertThat(store.get(key)).isEqualTo(Optional.of(value));
								  }
							  ));
		}
	}

@jlink
Copy link
Collaborator

jlink commented May 10, 2022

I’m struggling a bit with the concept of empty arbitrary because jqwik is built on the assumption that an arbitrary will produce an endless stream of values. Introducing it for this special usage feels wrong. It’s more functional but less Java-ish.

@vlsi
Copy link
Contributor

vlsi commented May 10, 2022

Frankly speaking, I have previously been bitten by Arbitraries.of(emptylist) producing an arbitrary that throws at generation time. It was hard to diagnose since it was not clear where did the empty arbitrary come from.

So it might make sense to split the existing Arbitraries.of(array|collection|...) into "of(array|...) arbitrary or throw if empty" and "ofOrEmpty(array|collection..)" that would return empty arbitrary on empty input. Then, the source code would clearly highlight the call could yield empty arbitrary which is dangerous in many cases.

That would be like Iterable.first (throws on empty) and Iterable.firstOrNull (null on empty).


However, I have no strong preference here, and just(transformer.noop()) would be workable for me.

@jlink
Copy link
Collaborator

jlink commented May 10, 2022

So it might make sense to split the existing Arbitraries.of(array|collection|...) into "of(array|...) arbitrary or throw if empty" and "ofOrEmpty(array|collection..)" that would return empty arbitrary on empty input. Then, the source code would clearly highlight the call could yield empty arbitrary which is dangerous in many cases.

I think it's worthwhile to rethink this design and moving towards a more complete Arbitrary abstraction. It's too big a change to do it in connection with the new stateful API. I'd rather do it within the re-design for jqwik 2.

I'll go with Transformer.noop() for now and see if this is sufficiently clean for a first step.
I'd like to do a 1.7.0 release if possible this months to give more people the chance to experiment with the new stateful API.

@jlink
Copy link
Collaborator

jlink commented May 12, 2022

Sorry for the delay. I try to migrate to the new API, and the first question I have is have you considered splitting state.Action into state.StateDependentAction and state.StateIndependentAction?

interface Action<S> {
   boolean precondition(S state);

   interface Dependent<S> extends Action<S> {
       Arbitrary<Transformer<S>> transformer(S state);
   } 

   interface Independent<S> extends Action<S> {
       Arbitrary<Transformer<S>> transformer();
   } 
}

Or rather top-level types StateDependentAction and StateIndependentAction?

@jlink
Copy link
Collaborator

jlink commented May 12, 2022

As for the builder, I tend to NOT have an ActionChainArbitraryBuilder but introducing an Action.Builder together with ActionChainArbitrary.addAction(Function<Action.Builder, Action.Builder>).

My argument for not having an ActionChainArbitraryBuilder is that arbitraries are already some kind of builder. Nesting builders within builders promotes mental overload, especially since it does not seem necesary for the purpose and it would be a the first explicit builder in jqwik (except for Builders which are arguably necessary to cover certain types of combinations).

@vlsi
Copy link
Contributor

vlsi commented May 16, 2022

After trying various approaches, I was able to launch some tests with the new API.

I have the following failure.
It looks like SequentialActionChain.transformations attempts to re-instantiate all the shrinkables after running the action sequence. However, the state has already been modified by that time, thus generateShrinkable fails.

Do you think ``SequentialActionChain.transformations` could be implemented without re-instantiating the shrinkables and without re-running the sequence?

    context(MappingState)
    fun InstanceRef.arbitraryPlainTable(): Arbitrary<TableRef> {
        val names = mapping.run { value }.tables.names // retrieves the list of table names from the state
        if (names.isEmpty()) {
            throw NOT_FOUND // <-- Exception from here
        }
        return Arbitraries.of(names).map { TableRef(this, it) }
    }
com.acme.test.fuzz.jqwik.NotFoundException
	at app//com.acme.Properties.getNOT_FOUND(Properties.kt:68)
	at app//com.acme.Properties.arbitraryPlainTable(Properties.kt:500)
	at app//com.acme.Properties.arbitraryPlainTable$lambda-26(Properties.kt:506)
	at app//net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$0(FlatMappedShrinkable.java:24)
	at app//net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.lambda$new$1(FlatMappedShrinkable.java:30)
	at app//net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.generateShrinkable(FlatMappedShrinkable.java:39)
	at app//net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.shrinkable(FlatMappedShrinkable.java:83)
	at app//net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.value(FlatMappedShrinkable.java:79)
	at [email protected]/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at [email protected]/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
	at [email protected]/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at [email protected]/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at [email protected]/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at [email protected]/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at [email protected]/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
	at app//net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValues(CombinedShrinkable.java:29)
	at app//net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValue(CombinedShrinkable.java:25)
	at app//net.jqwik.engine.properties.shrinking.CombinedShrinkable.value(CombinedShrinkable.java:21)
	at app//net.jqwik.engine.properties.state.ShrinkableChain$ChainInstance.lambda$transformations$0(ShrinkableChain.java:114)
	at [email protected]/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at [email protected]/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
	at [email protected]/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at [email protected]/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at [email protected]/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at [email protected]/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at [email protected]/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
	at app//net.jqwik.engine.properties.state.ShrinkableChain$ChainInstance.transformations(ShrinkableChain.java:114)
	at app//net.jqwik.engine.properties.state.SequentialActionChain.transformations(SequentialActionChain.java:29)
	at app//net.jqwik.engine.properties.state.SequentialActionChain.toString(SequentialActionChain.java:126)
	at app//net.jqwik.engine.support.JqwikStringSupport.displayString(JqwikStringSupport.java:37)
	at app//net.jqwik.engine.execution.reporting.ObjectValueReport.toStringLines(ObjectValueReport.java:17)
	at app//net.jqwik.engine.execution.reporting.ObjectValueReport.<init>(ObjectValueReport.java:13)
	at app//net.jqwik.engine.execution.reporting.ValueReport.of(ValueReport.java:54)
	at app//net.jqwik.engine.execution.reporting.ValueReport.of(ValueReport.java:26)
	at app//net.jqwik.engine.execution.reporting.ValueReport.of(ValueReport.java:21)
	at app//net.jqwik.engine.execution.reporting.SampleReporter.reportParameters(SampleReporter.java:69)
	at app//net.jqwik.engine.execution.reporting.SampleReporter.reportTo(SampleReporter.java:62)
	at app//net.jqwik.engine.execution.reporting.SampleReporter.reportSample(SampleReporter.java:24)
	at app//net.jqwik.engine.execution.reporting.ExecutionResultReport.hasDifferentOutput(ExecutionResultReport.java:137)
	at app//net.jqwik.engine.execution.reporting.ExecutionResultReport.reportParameterChanges(ExecutionResultReport.java:115)
	at app//net.jqwik.engine.execution.reporting.ExecutionResultReport.lambda$appendSamples$2(ExecutionResultReport.java:83)
	at [email protected]/java.util.Optional.ifPresent(Optional.java:183)
	at app//net.jqwik.engine.execution.reporting.ExecutionResultReport.appendSamples(ExecutionResultReport.java:77)
	at app//net.jqwik.engine.execution.reporting.ExecutionResultReport.buildJqwikReport(ExecutionResultReport.java:55)
	at app//net.jqwik.engine.execution.reporting.ExecutionResultReport.from(ExecutionResultReport.java:35)
	at app//net.jqwik.engine.execution.PropertyMethodExecutor.reportResult(PropertyMethodExecutor.java:102)
	at app//net.jqwik.engine.execution.PropertyMethodExecutor.execute(PropertyMethodExecutor.java:59)
	at app//net.jqwik.engine.execution.PropertyTaskCreator.executeTestMethod(PropertyTaskCreator.java:166)
	at app//net.jqwik.engine.execution.PropertyTaskCreator.lambda$createTask$1(PropertyTaskCreator.java:51)
	at app//net.jqwik.engine.execution.lifecycle.CurrentDomainContext.runWithContext(CurrentDomainContext.java:28)
	at app//net.jqwik.engine.execution.PropertyTaskCreator.lambda$createTask$2(PropertyTaskCreator.java:50)
	at app//net.jqwik.engine.execution.pipeline.ExecutionTask$1.lambda$execute$0(ExecutionTask.java:31)
	at app//net.jqwik.engine.execution.lifecycle.CurrentTestDescriptor.runWithDescriptor(CurrentTestDescriptor.java:17)
	at app//net.jqwik.engine.execution.pipeline.ExecutionTask$1.execute(ExecutionTask.java:31)
	at app//net.jqwik.engine.execution.pipeline.ExecutionPipeline.runToTermination(ExecutionPipeline.java:82)
	at app//net.jqwik.engine.execution.JqwikExecutor.execute(JqwikExecutor.java:46)
	at app//net.jqwik.engine.JqwikTestEngine.executeTests(JqwikTestEngine.java:70)
	at app//net.jqwik.engine.JqwikTestEngine.execute(JqwikTestEngine.java:53)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:108)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:96)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:75)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	at [email protected]/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at [email protected]/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at [email protected]/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at [email protected]/java.lang.reflect.Method.invoke(Method.java:566)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

@jlink
Copy link
Collaborator

jlink commented May 16, 2022

I guess it would be possible to eagerly collect all transformations when they are executed, but then the lazy evaluation of description strings will no longer make sense. That’s just guesswork though without looking at the code.

@vlsi
Copy link
Contributor

vlsi commented May 16, 2022

Yeah, I did not envision that when suggested lazy displayName computation :-/
I wonder if it would be possible to collect Supplier<String> or even the full list of transformers for the purposes of List<String> ActionChain#transformations().

@vlsi
Copy link
Contributor

vlsi commented Jun 27, 2022

It looks like shrink for stateful actions does not work under the current model.

The issue is as follows:

  1. jqwik calls TransformerProvider only during initial chain creation
  2. That means, the generated Transformers might be bound to the state (at the time of the generation)
  3. Later at the shrinking phase, jqwik no longer re-instantiates the transformers, and it attempts to call .shrink() etc methods. It might cause applying transformations that are no longer valid, and there's no way to verify if given transformation is valid. At the same time, if user code contains arbitrary.filter { something with state }, then it will continue using old state (the one created during the initial run).

I wonder if the transformers should be re-instantiated one after another even during the shrinking phase.

@vlsi
Copy link
Contributor

vlsi commented Jun 27, 2022

It looks the solution could be:
a) Re-create transformers during shrink phase. It looks like it would be hard since even the same seeds could produce different transformers since they depend on the state.

b) Allow reusing the existing transformers, however, then jqwik should verify "feasibility" of each transformer one by one as it applies steps during shrink phase. On top of that, it would likely make sense to use Supplier<State> rather than State argument, so exactly the same Transformer could be adapted to a new state without re-creating the transformer.

In other words:

public interface Action<S> {
    Arbitrary<Transformer<S>> transformer(Supplier<S> stateSupplier) {

Then the generated Transformer could bind to the given stateSupplier, and jqwik would be able to "retarget" the existing Transformer to the new state object.

I'm inclined to b.

@vlsi
Copy link
Contributor

vlsi commented Jun 27, 2022

It might be that Arbitrary<Transformer<S>> transformer(Supplier<S> stateSupplier) unifies both state-accessing and state non-accessing actions.

@jlink
Copy link
Collaborator

jlink commented Jul 18, 2022

It looks like shrink for stateful actions does not work under the current model.

The issue is as follows:

1. jqwik calls `TransformerProvider` only during initial chain creation

2. That means, the generated Transformers might be bound to the state (at the time of the generation)

3. Later at the shrinking phase, jqwik no longer re-instantiates the transformers, and it attempts to call `.shrink()` etc methods. It might cause applying transformations that are no longer valid, and there's no way to verify if given transformation is valid. At the same time, if user code contains `arbitrary.filter { something with state }`, then it will continue using **old** state (the one created during the initial run).

It seems to me that this only happens if the transformers themselves are mutable (which they should never be).
More likely: I don't understand the point. Can you provide a test to reveal the problem?

@jlink
Copy link
Collaborator

jlink commented Jul 23, 2022

As for the builder, I tend to NOT have an ActionChainArbitraryBuilder but introducing an Action.Builder together with ActionChainArbitrary.addAction(Function<Action.Builder, Action.Builder>).

ActionChainArbitrary.addAction() is now taking Action objects.
I'm struggling with a good Action.Builder API to replace Action static factory methods, though. My assumption is that more complicated actions would be implemented in classes anyway. The simple cases are easier using static factory methods. So I tend to leave it like it is for the moment.

Feedback welcome.

@jlink
Copy link
Collaborator

jlink commented Aug 11, 2022

It looks like shrink for stateful actions does not work under the current model.
The issue is as follows:

1. jqwik calls `TransformerProvider` only during initial chain creation

2. That means, the generated Transformers might be bound to the state (at the time of the generation)

3. Later at the shrinking phase, jqwik no longer re-instantiates the transformers, and it attempts to call `.shrink()` etc methods. It might cause applying transformations that are no longer valid, and there's no way to verify if given transformation is valid. At the same time, if user code contains `arbitrary.filter { something with state }`, then it will continue using **old** state (the one created during the initial run).

It seems to me that this only happens if the transformers themselves are mutable (which they should never be). More likely: I don't understand the point. Can you provide a test to reveal the problem?

Any news on that @vlsi ? Otherwise, I tend to close this issue since all my plans have now been realized in 1.7.0-SNAPSHOT

@vlsi
Copy link
Contributor

vlsi commented Dec 12, 2022

@jlink , I've created a sample chain that reproduces the issue of "wrong state being used in transformers": #426 (comment)

@jlink
Copy link
Collaborator

jlink commented Dec 18, 2022

I lost overview of all the different topics discussed in this issue. Can it be closed now since #426 has been tackled?

@DirkToewe
Copy link
Contributor Author

Yes the new stateful testing API is awesome! Thank You so much for all the hard work @jlink and @vlsi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants