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

support for heterogeneous arbitrary configurators in base #493

Closed
SimY4 opened this issue Jun 21, 2023 · 9 comments
Closed

support for heterogeneous arbitrary configurators in base #493

SimY4 opened this issue Jun 21, 2023 · 9 comments

Comments

@SimY4
Copy link

SimY4 commented Jun 21, 2023

Testing Problem

The current implementation of ArbitraryConfiguratorBase makes it super easy to configure a single arbitrary type using multiple annotation types but not vice versa where you might want to configure multiple arbitraries with a single annotation. i.e.

Suppose I have two arbitraries: Arbitrary<Foo> and Arbitrary<Bar> as I have a single annotation rule that can be applied to both. Unfortunately, due to erasure, there's no way to define two configure methods in base to satisfy its current contract:

class FooBarConfigurator extends ArbitraryConfiguratorBase {
  Arbitrary<Foo> configure(Arbitrary<Foo> arb, SomeAnnotation ann) { return arb; }

  Arbitrary<Bar> configure(Arbitrary<Bar> arb, SomeAnnotation ann) { return arb; } // no good since signatures collide
}

Suggested Solution

I would be keen to explore if jqwik could expand its search for potential config method signatures to include methods with random postfixes. i.e. say this would still be picked up by base as valid:

class FooBarConfigurator extends ArbitraryConfiguratorBase {
  Arbitrary<Foo> configureFoo(Arbitrary<Foo> arb, SomeAnnotation ann) { return arb; }

  Arbitrary<Bar> configureBar(Arbitrary<Bar> arb, SomeAnnotation ann) { return arb; }
}
@jlink
Copy link
Collaborator

jlink commented Jun 21, 2023

The feature would probably be rather easy to implement. For judging if it's worthwhile I wonder why just registering two separate configurators doesn't work for you?

class FooConfigurator extends ArbitraryConfiguratorBase {
  Arbitrary<Foo> configure(Arbitrary<Foo> arb, SomeAnnotation ann) { return arb; }
}
class BarConfigurator extends ArbitraryConfiguratorBase {
  Arbitrary<Bar> configure(Arbitrary<Bar> arb, SomeAnnotation ann) { return arb; }
}

If the configurators share code use a common superclass or extract the shared functionality it into a helper.

I may be missing something in the way you apply configurators.

@SimY4
Copy link
Author

SimY4 commented Jun 21, 2023

Makes sence.

It's just the volume in my case. I have >20 that can be configured by a single annotation (like wrapper types on top of String) that basically configures a string inside).

@jlink
Copy link
Collaborator

jlink commented Jun 22, 2023

Had a look at the implementation of ArbitraryConfiguratorBase. The enhancement is possible. Just not as straightforward as I initially thought due to some shortcuts I took.

It's on the list. Just not at the top of it.

@jlink
Copy link
Collaborator

jlink commented Aug 8, 2023

@SimY4 The current snapshot 1.8.0-SNAPSHOT should support this feature. Maybe you can check if it fits your needs.

@SimY4
Copy link
Author

SimY4 commented Aug 8, 2023

Thank you @jlink . I only glanced through the changes on GitHub on my mobile, so could be totally wrong here. I think the code is still vulnerable to configuring the wrong thing if configuration annotation is applicable to different types. I.e. if I have @Odd applicable to Long and BigInteger it can incorrectly substitute them.

@jlink
Copy link
Collaborator

jlink commented Aug 9, 2023

I thought it was covered, but checking shows that you're right.

@jlink
Copy link
Collaborator

jlink commented Aug 9, 2023

Actually the code was already correct, but the presence of the Kotlin module could mess up annotation interpretation in Java classes and tests :-/

So, the following code - taken from jqwiks documentation examples - works:

public class OddConfigurator extends ArbitraryConfiguratorBase {

	public Arbitrary<Integer> configureInteger(Arbitrary<Integer> arbitrary, Odd odd) {
		return arbitrary.filter(number  -> Math.abs(number % 2) == 1);
	}

	public Arbitrary<BigInteger> configureBigInteger(Arbitrary<BigInteger> arbitrary, Odd odd) {
		return arbitrary.filter(number  -> {
            return number.mod(BigInteger.valueOf(2)).compareTo(BigInteger.ZERO) != 0;
        });
	}
}

class OddProperties {

	@Property @Report(Reporting.GENERATED)
	boolean oddIntegersOnly(@ForAll @Odd int aNumber) {
		return Math.abs(aNumber % 2) == 1;
	}

	@Property @Report(Reporting.GENERATED)
	boolean oddBigIntegersOnly(@ForAll @Odd BigInteger aNumber) {
		return Math.abs(aNumber.longValueExact() % 2) == 1;
	}
}

What I left out here is the code for the annotation and the registration of the configurator.

@jlink
Copy link
Collaborator

jlink commented Aug 9, 2023

I redeployed 1.8.0-SNAPSHOT with a fixed Kotlin module.

@jlink
Copy link
Collaborator

jlink commented Aug 11, 2023

@jlink jlink closed this as completed Aug 11, 2023
@jlink jlink removed the in progress label Aug 11, 2023
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