FlexTradeUKLtd / jfixture

JFixture is an open source library based on the popular .NET library, AutoFixture
MIT License
105 stars 22 forks source link

Generic types cannot be customized #33

Closed TWiStErRob closed 7 years ago

TWiStErRob commented 7 years ago

How can I make this test pass?

public class GenericListTest {

    @Fixture Container fixture;

    private final JFixture jFixture = new JFixture();

    @Before
    public void setUp() {
        jFixture.behaviours().add(new TracingBehaviour(new DebugTracingStrategy(), System.out));
        FixtureAnnotations.initFixtures(this, jFixture);
    }

    @Test
    public void defaultBehaviorIsToCreateListOfThreeItems() {
        Assert.assertThat(fixture.otherList, Matchers.hasSize(3));
        Assert.assertThat(fixture.list, Matchers.hasSize(3));
    }

    @Test
    public void customizedBehaviorWantingANullField() {
        jFixture.customise().sameInstance(new com.google.common.reflect.TypeToken<List<Contained>>(){}.getType(), null);
        jFixture.customise().sameInstance(new SpecimenType<List<Contained>>(){}, null);
        jFixture.customise().sameInstance(SpecimenType.of(new com.google.common.reflect.TypeToken<List<Contained>>(){}.getType()), null);
        // what to put here to make this work?

        fixture = jFixture.create(Container.class);

        Assert.assertThat(fixture.otherList, Matchers.hasSize(3));
        Assert.assertThat(fixture.list, Matchers.nullValue());
    }

    static class Container {
        final List<Contained> list;
        final List<String> otherList;

        Container(List<Contained> list, List<String> otherList) {
            this.list = list;
            this.otherList = otherList;
        }
    }
    static class Contained {
        final String name;

        Contained(String name) {
            this.name = name;
        }
    }
}

it fails with:

java.lang.AssertionError: Expected: null but: was <[GenericListTest$Contained@79b4d0f, GenericListTest$Contained@6b2fad11, GenericListTest$Contained@79698539]>

because:

GenericTypeCollection.equals fails on nameTypeMap comparison which leads to CustomBuilder.create skip the supplier and return NoSpecimen resulting in default behavior.

richkeenan commented 7 years ago

Hi @TWiStErRob,

Good spot on this - definitely looks like a bug to me. Your second line,

 jFixture.customise().sameInstance(new SpecimenType<List<Contained>>(){}, null);

should work.

The CustomBuilder<T> class checks the type of the request against the type used in the customisation and if they're equal it returns the value used in the customisation. In your example that equality check is returning false even though both types are SpecimenType<List<Contained>>.

The problem seems to be with the way the SpecimenType equality works because it checks the underlying (non-generic) types are equal, in this case List, then it checks the types of the generic arguments, in this case Contained and that's returning false. I'll have a look into why and hopefully get a PR in soon

TWiStErRob commented 7 years ago

Thanks for the quick reply.

Hint: when I debugged the equality was failing because one of the maps had "E" -> Contained and the other one had "" -> Contained entries.

richkeenan commented 7 years ago

Yea that's exactly it - the equality check is looking at the name and the type. If I remove the check on the name it works and all existing tests pass. I want to make sure I understand why it works/why I had that check in there in the first place, but I reckon this should be ok

TWiStErRob commented 7 years ago

How about Map<String, Integer> being different from Map<Integer, String>? In that case you need the K and V mapping, because the map is unordered, you can't just a.values().equals(b.values()).

richkeenan commented 7 years ago

The original code used Lombok to auto generate the equals and hashcode which looked at all the fields to do that. I de-lomboked ages ago and kept the default implementation.

Unfortunately the implementation in GenericTypeCollection does this and it's a bit silly because 2 of the 3 fields in that class and just derived from the other (which is the arg passed to the constructor.) By changing the equals/hashcode to just use the original value it works as expected.

I'll make that change now and include your test case to avoid regressions. Thanks!

TWiStErRob commented 7 years ago

By changing the equals/hashcode to just use the original value it works as expected.

As I see then GenericType's equals will be in the way. Because getName() was E for one and "" for the other. That difference will be still there and Arrays.equals will be false.

richkeenan commented 7 years ago

Yea I'm thinking of not using GenericType's equals, but doing a specific check, e.g.

if (underlying.length != that.underlying.length) return false;

for (int i = 0; i < underlying.length; i++) {
    if (!underlying[i].getType().equals(that.underlying[i].getType())) return false;
}

return true;

What do you think?

TWiStErRob commented 7 years ago

Yes, I see that would work. It keeps order and compares what we're interested in, given that what getType() returns implements equals that way. For example SpecimenType handles other known Types, but the equals is not symmetric: specType.equals(otherType) != otherType.equals(specType). To mitigate this, I think you need to equals on getType().getTypeName(). (Note: Class does not have an equals method!)

TWiStErRob commented 7 years ago

@richkeenan can you please release this? (also README.md version is out of date)

richkeenan commented 7 years ago

That's done. Sorry for the delay, 2.7.1 should be up on Maven Central once it's synced with Nexus.

Going forward I don't plan on actively maintaining this project because I don't use it (or Java) and it's always a struggle to back into developing it! I'll update the readme to make that clear too. Thanks so much for your support and raising issues that you've found and I'll obviously accept PRs for things you want in the library that I like.

Cheers, Rich