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

Add ActionSequenceArbitrary#sequences(Arbitrary<? extends List<? extends Action<M>>>) for dependent actions #300

Open
vlsi opened this issue Feb 4, 2022 · 14 comments

Comments

@vlsi
Copy link
Contributor

vlsi commented Feb 4, 2022

Testing Problem

See #134 (comment)
The case is that input data is hierarchical, and certain states are known to be invalid.
For instance, I know "table without columns" can't be created in the database, so I would like to avoid generating that state during testing.
At the same time, having action classes for add table , add column, remove column, remove table seems to be helpful for code deduplication.

In other words, what I want is to ensure that add table is always accompanied with add column action.
Then, there's a limit on the number of columns in a single table (e.g. OracleDB table can have up to 1000 columns), so I want to have 1000 columns as an edge case. In other words, the edge case would be add table, then repeat 1000 times (add column, update column).

Note, that CompositeAction(add table, add column) can't work since its precondition can't be implemented properly.
add column verifies that the table with given name exists, and the verification should be performed only after add table is executed.

Suggested Solution

Implement ActionSequenceArbitrary#sequences(Arbitrary<? extends List<? extends Action<M>>>), so the users can
return multiple actions in sequence

@Provide
fun testActions() =
    Arbitraries.sequences(
        frequencyOf(
            1 to listOf(just(`remove all tables`())),
            4 to listOf(just(`add table`()), just(`add column to table`())), // <-- here are the actions that are generated paired
...

Discussion

Alternative options could be
a) a Markov chain generator instead of frequencyOf. E.g. users would provide a matrix of weights, so add column would be the only continuation after add table.
A fixed weight matrix would allow jqwik to figure out edge cases automatically (it would know which arbitraries can ever be created just like in frequencyOf)

b) Something like Stream#iterate(T seed, UnaryOperator f).
It looks like the current net.jqwik.api.Arbitraries#lazy(Supplier<Arbitrary<T>>) can be used for a stateful generation, however, lazy does not seem to support edge cases

@jlink
Copy link
Collaborator

jlink commented Feb 5, 2022

@vlsi Interesting use case.

I'm wondering if

Note, that CompositeAction(add table, add column) can't work since its precondition can't be implemented properly.
add column verifies that the table with given name exists, and the verification should be performed only after add table is executed.

is really true, because at first glance it looks like the simplest way to satisfy the need.
What if the composite action's precondition is equal to the first action's precondition, and the subsequent actions' preconditions are just a trigger to execute or not execute the code in their run() methods respectively. Shrinking should - I assume - always happen for the full composite action, shouldn't it?

The markov chain solution also sounds interesting b/c it would be more generic. I think it deserves an issue of its own.

@vlsi
Copy link
Contributor Author

vlsi commented Feb 5, 2022

What if the composite action's precondition is equal to the first action's precondition, and the subsequent actions' preconditions are just a trigger to execute or not execute the code in their run() methods respectively.

That is what I tried in my workaround, however, it did not work because jqwik protects from keeping state in precondition method (jqwik creates new object).

For now, I am looking into implementing my own SequentialActionSequence / RandomActionGenerator so they would either allow keeping state (e.g. they would call precondition in SequentialActionSequence#initialRun, or they would even allow deriving values from the model, so I no longer need to map indices to names, and the test code would be easier.


Yet another issue is that I am willing to discard "incompatible" actions during the shrinking phase.
As far as I understand, the current behavior is that jqwik would just stop executing actions as soon as it faces the first action that is not applicable. On the other hand, I am fine if excluding one of the actions requires removing another one, and I would like to keep repeatedRun running even if some of the actions no longer apply.

@jlink
Copy link
Collaborator

jlink commented Feb 6, 2022

Yet another issue is that I am willing to discard "incompatible" actions during the shrinking phase.
As far as I understand, the current behavior is that jqwik would just stop executing actions as soon as it faces the first action that is not applicable. On the other hand, I am fine if excluding one of the actions requires removing another one, and I would like to keep repeatedRun running even if some of the actions no longer apply.

If I remember correctly, actions will be removed during shrinking if their precondition does not hold, but I may be wrong. If I am wrong, I think this could be fixed.

@jlink
Copy link
Collaborator

jlink commented Feb 6, 2022

Here's an implementation of CompositeAction. The demo code is here:

class CompositeAction<T> implements Action<T> {

	private final Action<T> first;
	private final Action<T> second;

	public CompositeAction(Action<T> first, Action<T> second) {
		this.first = first;
		this.second = second;
	}

	@Override
	public boolean precondition(T state) {
		return first.precondition(state);
	}

	@Override
	public T run(T state) {
		T stateAfterFirst = first.run(state);
		if (second.precondition(stateAfterFirst)) {
			return second.run(stateAfterFirst);
		} else {
			return stateAfterFirst;
		}
	}

	@Override
	public String toString() {
		return String.format("[%s then %s]", first, second);
	}
}

It seems to work for me, but I might have overlooked something.

It's also an open point of discussion if the failure of a subsequent precondition should exclude the composite action from being considered or not.

@vlsi
Copy link
Contributor Author

vlsi commented Feb 6, 2022

It seems to work for me, but I might have overlooked something.

See https://github.com/jlink/jqwik/blob/cc952102e3209a20c63e9b1cb6a7c3007a028b91/engine/src/main/java/net/jqwik/engine/properties/stateful/SequentialActionSequence.java#L69-L73

It terminates execution as soon as any precondition fails.

@jlink
Copy link
Collaborator

jlink commented Feb 7, 2022

It terminates execution as soon as any precondition fails.

Yes, but this code is not executed during shrinking. AFAIR repeated runs are currently only used in tests to verify that a shrunk sequence produces the expected result model. The code you quote makes sure that the same sequence (not a shrunk one) will use the same sequence of actions (or fail) on a second, third etc run.

You can see how actions sequences are shrunk in:

@jlink
Copy link
Collaborator

jlink commented Sep 30, 2022

@vlsi Are composite actions still relevant in the context of the new stateful properties approach? If so, feel free to reopen the issue. The implementation should still be rather straightforward.

@jlink jlink closed this as completed Sep 30, 2022
@vlsi
Copy link
Contributor Author

vlsi commented Sep 30, 2022

I think the issue is still relevant. I do not think the new API provides a possibility to generate dependent chains.

@jlink
Copy link
Collaborator

jlink commented Sep 30, 2022

Yes it does, that was the whole point.

A simple chain example to generate regular expressions:

class RegexChainExample {

	@Property(tries = 10)
	void generateABPlusC(@ForAll("abplusc") Chain<String> regex) {
		String string = null;
		for (String value : regex) {
			string = value;
		}
		System.out.println(string);
		assertThat(string).matches("ab+c");
	}

	@Provide
	Arbitrary<Chain<String>> abplusc() {
		return Chain.startWith(() -> "")
					.addTransformation(Transformation.when(String::isEmpty).provide(just(s -> s + "a")))
					.addTransformation(Transformation.<String>when(s -> s.endsWith("a")).provide(just(s -> s + "b")))
					.addTransformation(Transformation.<String>when(s -> s.endsWith("b")).provide(
						 frequency(
							 Tuple.of(5, s -> s + "b"),
							 Tuple.of(1, s -> s + "c")
						 )
					 ))
					.addTransformation(Transformation.<String>when(s -> s.endsWith("c")).provide(just(Transformer.endOfChain())))
					.infinite().dontShrink();
	}

}

addTransformation(..) can also take a weight if you want to mess with probabilities.

Action.Dependent is the equivalent for action chains.

@vlsi
Copy link
Contributor Author

vlsi commented Sep 30, 2022

I know I can use addTransformation, and I know I can use Transformation.when, however, it does not seem to provide a way to glue several actions together.

Here's a sample case from the issue description:

4 to listOf(just(`add table`()), just(`add column to table`())), // <-- here are the actions that are generated paired

In other words, if I generate add table action, I want it to be accompanied with other actions like add column to table.

Well, technically speaking, I can implement a state machine in the state: add table action might change the state like .lastTable=...; .nextAction=ADD_COLUMN;, however, coding state-machines is error-prone.

@jlink
Copy link
Collaborator

jlink commented Sep 30, 2022

In other words, if I generate add table action, I want it to be accompanied with other actions like add column to table

So it's about always doing those together? You'll never want to just addTable and then do something completely different?

@vlsi
Copy link
Contributor Author

vlsi commented Sep 30, 2022

So it's about always doing those together?

Well, a table can't be created without a column, so it just makes no sense to create a table without columns.
For instance, if I generate 1000 tables, it is unlikely that jqwik will issue "add column" for all the tables. It is likely there will be at least one table without a column which is useless from the testing perspective.

After I create the table as a package of actions (add table, add column, etc), I still want the other add column would be scheduled for the table. However, I just don't want to spend much time generating sequences that are invalid.

@jlink
Copy link
Collaborator

jlink commented Sep 30, 2022

OK. There's value in that. I reopen.

@jlink jlink reopened this Sep 30, 2022
@vlsi
Copy link
Contributor Author

vlsi commented Sep 30, 2022

For now, I use a single combine(`add table`(), `add columns`(), ...), so it more-or-less works, however, it looks more like a workaround to me.

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

2 participants