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

Generic types cannot be customized #33

Closed
TWiStErRob opened this issue Apr 21, 2017 · 10 comments
Closed

Generic types cannot be customized #33

TWiStErRob opened this issue Apr 21, 2017 · 10 comments

Comments

@TWiStErRob
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

TWiStErRob commented Apr 21, 2017

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
Copy link
Collaborator

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
Copy link
Contributor Author

TWiStErRob commented Apr 21, 2017

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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!)

richkeenan added a commit that referenced this issue May 3, 2017
… name as it was causing a bug when overriding generic types. Fixes #33
richkeenan added a commit that referenced this issue May 9, 2017
… name as it was causing a bug when overriding generic types. Fixes #33 (#34)
@TWiStErRob
Copy link
Contributor Author

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

@richkeenan
Copy link
Collaborator

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

richkeenan added a commit to richkeenan/jfixture that referenced this issue May 11, 2020
… name as it was causing a bug when overriding generic types. Fixes FlexTradeUKLtd#33 (FlexTradeUKLtd#34)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants