quicktheories / QuickTheories

Property based testing for Java 8
Apache License 2.0
505 stars 51 forks source link

Add method to support dependent generators in forAll #34

Closed doctau closed 6 years ago

doctau commented 6 years ago

This isn't meant to be committed as-is, it's a work in progress but wanted to get some feedback on it, especially if you think it's a good idea.

While writing https://github.com/ncredinburgh/QuickTheories/pull/33, I wondered why the ListDSLTest hard-coded the numbers 2 and 1 for the length of the lists rather than being generated too. I suspect the answer is at least partially that there isn't an elegant way to generate a value, generate a dependent value, and then use both in the test.

This code adds an equivalent to asWithPrecursor() with a Gen-producing function, as flatMap() is to map(). What do you think of the idea of this? Also, "withPrecursorGen" could do with a better name.

If this seems useful, I can add it to the other TheoryBuilders too. In the size-between tests generating (min, extra) and adding them in both the list generator and the check isn't as nice as it could be. I should be able to make it so you can chain withPrecursorGen, so it could be something like:

.forAll(integers().between(0, 10))
.withPrecursorGen(min -> integers().between(0, 10).map(n -> n + min))
.withPrecursorGen((min, max) -> lists().of(integers().all()).ofSizeBetween(min, max))
.check((min, extra, l) -> l.size() >= min && l.size() <= max);
hcoles commented 6 years ago

Thanks for the PR, writing the response below was quite helpful in thinking about what direction to take things in.

The reason the list dsl doesn't accept a Gen of Integers for sizes is history and oversight - it should. Generators didn't use to compose together nicely. Now that they do the api ought to accept them whenever possible. The maps dsl was fixed to do this when it was merged in.

I agree that withPrecursorGen (with a name that was somehow better) would be useful in some circumstances, but I think I'd prefer not to add it as it adds complexity.

The first 0.1x cut of quicktheories had a lot of very complex code spread around the place - it was big relief to get rid of a lot of it in 0.2 and I want to avoid getting back to the situation again.

So, unless there's a compelling case I'd prefer not to expand the api in the theory builders (which requires changes in 4 or more places for consistency, and could require more if the airity is ever increased). In contrast adding flatMap to Gen required 1 line of code.

So I think where we are is we have an easily accessible api for basic operations available via the theorybuilders. This is going to stay fairly static unless there's a really strong use case.

If you want to do something more complex you need to work in terms of Gens. This makes sense as that complex work can then be easily re-used.

doctau commented 6 years ago

Thanks, that makes sense.