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

equals method should not be overriden on interfaces #103

Closed
akutschera opened this issue Aug 17, 2024 · 6 comments · Fixed by #104
Closed

equals method should not be overriden on interfaces #103

akutschera opened this issue Aug 17, 2024 · 6 comments · Fixed by #104
Labels

Comments

@akutschera
Copy link
Collaborator

While working on a fix for #102 I noticed that the following test fails:

    @RepeatedTest(10)
    void issue103() {
        var sut = Fixture.fixture().create(Closeable.class);

        assertThat(sut).isEqualTo(sut);
    }

This fails for interfaces and abstract classes because we create a proxy that returns a random value for any called method.

You have to run it multiple times, because the result of equals is a random boolean, so for some created fixtures it's true, for others it's false.
If we create random values for this, we violate the Java contract for equals and hashCode. This might lead to unexpected behaviour.
I believe this should be fixed before the fix for #102 can be merged.

@akutschera akutschera added the bug label Aug 17, 2024
@akutschera
Copy link
Collaborator Author

I propose that when we construct a proxy for interfaces or abstract classes, we only fixture abstract methods. Everything else should be forwarded to the existing (parent) classes.
That has the advantage that the fixtured objects are closer to real-life instances, even though we will have to write a bit more code in the ProxyInvocationHandler.

@Nylle
Copy link
Owner

Nylle commented Aug 19, 2024

Only fixturing the abstract methods should not be hard. This can easily be reflected - maybe the SpecimenType already has this? (I'm not sure.)
If we can rely on the "existing parent class" (I don't know what this would be on an interface), I'd agree to your proposal.

Edit: yes, SpecimenType has a helper for abstract, but it only applies to the type itself - to analyse methods we'd have to write a little something.

@akutschera
Copy link
Collaborator Author

For interfaces I want to keep the default methods. If we call these instead of fixturing, we should be fine. Maybe in the future there will be a need to fixture these, but that still needs to be separated from fixturing equals or hashCode.
Besides, we do not overwrite existing methods anywhere else so not doing that in abstract classes or interfaces would actually make JavaFixture more predictable (as in: less surprising).

@akutschera
Copy link
Collaborator Author

The commits above should fix that. As a bonus, when concrete methods throw an exception, the caller will see it.

@Nylle
Copy link
Owner

Nylle commented Aug 22, 2024

(Will be closed as resolved with the next release.)

@Nylle Nylle reopened this Aug 22, 2024
Nylle added a commit that referenced this issue Aug 25, 2024
- Bugfix: Sets can now contain more than one element (#102)
- Bugfix: The `equals`-method will no longer be proxied for abstract specimen (#103)
- Feature: Collections will now contain unique elements instead of a number of copies of the same instance
- Experimental: Failures to fixture an object will now contain more information about what was attempted and what went wrong
@jk-idealo
Copy link
Collaborator

Fixed since version 2.11.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants