naver / fixture-monkey

Let Fixture Monkey generate test instances including edge cases automatically
https://naver.github.io/fixture-monkey
Apache License 2.0
560 stars 89 forks source link

Fix when to apply JdkVariantOptions #992

Closed ctmanjak closed 3 months ago

ctmanjak commented 3 months ago

Summary

Fix when to apply JdkVariantOptions in FixtureMonkeyOptionsBuilder.

(Optional): Description

Fix an issue where JdkVariantOptions.apply has a higher priority than options set by the user because it is in the build method of FixtureMonkeyOptionsBuilder.

How Has This Been Tested?

Use existing tests.

seongahjo commented 3 months ago

Fix an issue where JdkVariantOptions.apply has a higher priority than options set by the user

I do know when to override the options in JdkVariantOptions

Could you show the example?

ctmanjak commented 3 months ago

@seongahjo

private static final FixtureMonkey SUT = FixtureMonkey.builder()
    .objectIntrospector(ConstructorPropertiesArbitraryIntrospector.INSTANCE)
    .defaultNotNull(true)
    .pushAssignableTypeObjectPropertyGenerator(
        SealedClass.class,
        new InterfaceObjectPropertyGenerator<>(
            List.of(
                SealedClassImpl.class
            )
        )
    )
    .build();

private static sealed class SealedClass permits SealedClassImpl, SealedClassImpl2 {
}

private static final class SealedClassImpl extends SealedClass {
}

private static final class SealedClassImpl2 extends SealedClass {
}

If you declared it in the same way as above.

@RepeatedTest(TEST_COUNT)
void sampleSealedClass() {
    // when
    SealedClass actual = SUT.giveMeOne(SealedClass.class);

    then(actual).isInstanceOf(SealedClassImpl.class);
}

About half of these tests will fail.

스크린샷 2024-06-17 오후 8 05 31

because the ObjectPropertyGenerator applied by JdkVariantOptions has a higher priority than the one you added.

seongahjo commented 3 months ago

@ctmanjak If so, it is better to add the test (like sampleSealedClass you have made )with the modified option to illustrate the case and prevent the regression.

ctmanjak commented 3 months ago

~@seongahjo I think I'll need the SealedClassTestSpecs from #991 when writing test code, should I combine the two PRs?~

nvm. It's merged 😅