Make IgnoreException "attempts" (and other related hard coded values) configurable

[adam-waldenberg]

Testing Problem

With some pesky implementations ignoreExceptons can hit it's hard coded limit very easily.

Suggested Solution

This should probably be configurable as it's easy to accidentally hit under certain conditions:

jqwik/engine/src/main/java/net/jqwik/engine/properties/arbitraries/randomized/IgnoreExceptionGenerator.java

Line 27 in b736d79

for (int i = 0; i < 10000; i++) {

There are probably a few other places similar ones like this one that should also be configurable:

jqwik/engine/src/main/java/net/jqwik/engine/properties/arbitraries/randomized/CollectGenerator.java

Line 22 in b736d79

for (int i = 0; i < 10000; i++) {

Discussion

Not sure if its more appropriate to make these properties or some other method, but some property/config value is probably appropriate.

[jlink]

My first take is that if a generator attempt fails that often one should look for a different approach to generate those values. But that view is probably too narrow.

Since the number of necessary attempts is case specific, a case specific configuration would be required. There is indeed one place where this already exists: https://jqwik.net/docs/1.7.2/javadoc/net/jqwik/api/Arbitrary.html#filter(int,java.util.function.Predicate)

Would something like that fit your needs? E.g.

val int maxMisses = 1000000;
val ignoringArbitrary = myArbitrary.ignoreException(maxMisses, MyExceptionType.class);

What are the other places you're referring to?

[adam-waldenberg]

The other ones I found so far are these,

jqwik/api/src/main/java/net/jqwik/api/Arbitrary.java

Line 211 in b736d79

return filter(10000, filterPredicate);

jqwik/engine/src/main/java/net/jqwik/engine/properties/arbitraries/exhaustive/ExhaustiveGenerators.java

Line 48 in b736d79

return exhaustiveGenerator.filter(l -> checkUniquenessOfValues(uniquenessExtractors, l), 10000);

jqwik/engine/src/main/java/net/jqwik/engine/properties/arbitraries/exhaustive/ExhaustiveGenerators.java

Line 76 in b736d79

.filter(s -> UniquenessChecker.checkUniquenessOfValues(featureExtractors, s), 10000)

jqwik/engine/src/main/java/net/jqwik/engine/properties/arbitraries/exhaustive/IgnoreExceptionExhaustiveGenerator.java

Line 46 in b736d79

for (int i = 0; i < 10000; i++) {

Unsure about this one,

jqwik/engine/src/main/java/net/jqwik/engine/properties/arbitraries/randomized/ContainerGenerator.java

Line 50 in b736d79

this.maxAttempts = Math.min(10000, Math.max(1000, maxUniqueElements * 10));

But any hard-coded integers that affect/limit generation should probably be configurable. Not only the defaut, but, as you also said, probably per-case with some of these as well. Then again, one could argue that in a specific test case you can almost always limit/change/catch the generated values to avoid these hits. In that case, maybe a common setable property for this would be enough? - just to be able to change that default.

[adam-waldenberg]

My first take is that if a generator attempt fails that often one should look for a different approach to generate those values. But that view is probably too narrow.

Yes, there most definitely is always a way around it - and limiting the number of hits from the test is exactly how I solved it for now.

Since the number of necessary attempts is case specific, a case specific configuration would be required. There is indeed one place where this already exists: https://jqwik.net/docs/1.7.2/javadoc/net/jqwik/api/Arbitrary.html#filter(int,java.util.function.Predicate)

Would something like that fit your needs? E.g.

val int maxMisses = 1000000;
val ignoringArbitrary = myArbitrary.ignoreException(maxMisses, MyExceptionType.class);

What are the other places you're referring to?

As a per-case setting, something like that is probably more than enough. But that wouldn't work with, say a @Provide method within a @Property test that has a ignoreException?

[jlink]

As a per-case setting, something like that is probably more than enough. But that wouldn't work with, say a @Provide method within a @Property test that has a ignoreException

It wouldn't without introducing another annotation attribute.

My tendency towards not making everything configurable is mostly founded in a generic principle: Every configuration parameter comes with a cost, which is usually underestimated. Thus, I'm happy to add an optional parameter if a concrete need has been shown - as it is now the case for ignoreException.
As for introducing a global config parameter for all filtering-like functionality, this needs a lot more proof of usefulness than I can currently see.

[adam-waldenberg]

I absolutely don't agree.

There is a saying - and that's; constants are evil and that is especially true with frameworks. Introducing them without an easy way of changing them is generally not a good thing. It is also considered an anti-pattern when inlined like this.

Having a default is absolutely fine. But having a hard-coded constant that's not easily changeable that fundamentally affects the way the framework functions? - not so much.

[jlink]

We'll have to agree to disagree then. Each exposed config value extends the API surface of a library, which comes with costs - maintenance costs being the most prominent. Each extension should pull its weight, otherwise maintenance to keep up backwards compatibility will badly influence a lib's ability to change.

[adam-waldenberg]

If its something that changes behavior and needs continuous maintenance - yes I would agree. But in this case it's just a property value to get rid of a hard-coded magic number that keeps repeating in the code. It's not a good idea to force a rigid implementation. Where exactly does this 10000 number come from and how have you defined it? How do we know 1000, 5000, 500 or 20000 isn't better?

[jlink]

I don’t know the best value, but I know a useful one which works for the initially know use cases. The problem with exposing the value for configuration is that it’s an implementation detail. If I change the implementation the meaning of the value may change or the value may disappear completely. Having to keep it around for compatibility is a burden.

[adam-waldenberg]

As long as they are in there as they are right now they have the potential to cause issues. If this is solved some other way in the future and the property value goes away because it's no longer needed - great.

But yes, getting rid of these rather than botching it with a property (if it's actually doable) obviously sounds a lot better. I don't think it is though? I guess @Provide methods would be required to provide unique values for that to even be an option?

[jlink]

Arbitrary.ignoreException(maxThrows, exceptionType) and
Arbitrary.ignoreExceptions(maxThrows, exceptionTypes)

are available in 1.7.3-SNAPSHOT.

[adam-waldenberg]

Cool. Thank you.