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

Enum comparison with String is not working with recursive assertion #2616

Closed
vimalbera92 opened this issue May 18, 2022 · 18 comments
Closed
Assignees
Labels
theme: recursive comparison An issue related to the recursive comparison
Milestone

Comments

@vimalbera92
Copy link

vimalbera92 commented May 18, 2022

Summary

Comparison of enum with string fails with recursive assertion and strictTypeChecking as false. I would expect below code to be run successfully.

actual and expected objects and their fields were compared field by field recursively even if they were not of the same type - as per error, actual and expected values are same (CAR in below example) and test case should pass.

Example

public class TestEnumWithString {

    @Test
    public void testEnumWithString() {
        assertThat(Vehicle.CAR).usingRecursiveComparison().isEqualTo("CAR");
    }
}

enum Vehicle {
    CAR
}
@scordio scordio added the theme: recursive comparison An issue related to the recursive comparison label May 18, 2022
@joel-costigliola
Copy link
Member

@vimalbera92, thanks for reporting this. Which version are you using?

@vimalbera92
Copy link
Author

@joel-costigliola - I'm using latest version 3.22.0.

@joel-costigliola
Copy link
Member

joel-costigliola commented May 19, 2022

The behavior is as expected, strictTypeChecking as false applies for comparing objects, not primitives.

We are discussing with the team the pros and cons of adding an option that would allow to compare enum with string.

In the meantime you can register comparators per field or type to tweak the behavior as you want, see https://assertj.github.io/doc/#assertj-core-recursive-comparison-comparators.

@joel-costigliola joel-costigliola added this to the 3.24.0 milestone May 19, 2022
@scordio scordio added the status: team discussion An issue we'd like to discuss as a team to make progress label May 19, 2022
@vimalbera92
Copy link
Author

@joel-costigliola Thanks for quick response and workaround.

I think it's not a good idea to include all primitive types to check against string but definitely worth enum comparison with string but I'll leave that to assertj team to decide and agree upon.

@joel-costigliola
Copy link
Member

Closing this issue as we can register specific comparison/equals for types or fields which I think cover the use case.
Happy to reopen if that turns our not to be true.

@armandino
Copy link
Contributor

@joel-costigliola, apologies for resurrecting an old issue. I wanted to add another vote for supporting enum/string comparison as it's a fairly common use case. Currently this requires withEqualsForFields, as suggested above, which can get a little verbose if there are many enums:

assertThat(entity)
        .usingRecursiveComparison()
        .withEqualsForFields((String s, PhoneType t) -> PhoneType.valueOf(s) == t, "phones.phoneType")
        .withEqualsForFields((String s, AddressType t) -> AddressType.valueOf(s) == t, "addresses.addressType")
        // etc..
        .isEqualTo(dto);

It would be nice if this was supported OOTB via an option, e.g.:

assertThat(entity)
        .usingRecursiveComparison()
        .withEnumStringComparison()
        .isEqualTo(dto);

@scordio scordio reopened this Apr 8, 2023
@scordio
Copy link
Member

scordio commented Apr 8, 2023

What about withEnumNameComparison? Mostly to refer to Enum::name

@joel-costigliola
Copy link
Member

joel-costigliola commented Apr 8, 2023

What about a name like comparingEnumsByName():

assertThat(entity).usingRecursiveComparison()
                  .comparingEnumsByName()
                  .isEqualTo(dto);

@scordio scordio modified the milestones: 3.24.0, 3.25.0 Apr 8, 2023
@armandino
Copy link
Contributor

What do you think about including other types, in addition to enums? e.g.

class Person {
    UUID id;
    int age;
    Gender gender;
    boolean isActive;
}

class PersonDto {
    String id;
    String age;
    String gender;
    String isActive;
}

I believe it's a pretty common use case where one side of the comparison has String representations of certain types. Including such an option would make usingRecursiveComparison() more versatile and tests more concise. Maybe something like allowToStringComparison()?

@joel-costigliola
Copy link
Member

joel-costigliola commented Apr 8, 2023

The challenge here would be that the root objects, myPerson and myPersonDto would be compared by their string representation, this is equivalent to assertThat(myPerson.toString()).isEqualTo(myPersonDto.toString()).

Note that you can actually achieve comparing all fields by string using withEqualsForFieldsMatchingRegexes((o1, o2) -> o1.toString().equals(o2.toString()), ".+"):

    PersonEntity person = new PersonEntity();
    person.id = UUID.randomUUID();
    person.age = 20;
    person.isActive = true;

    PersonDto personDto = new PersonDto();
    personDto.id = person.id.toString();
    personDto.age = "20";
    personDto.isActive = "true";

    assertThat(person).usingRecursiveComparison()
                      .withEqualsForFieldsMatchingRegexes((o1, o2) -> o1.toString().equals(o2.toString()), ".+")
                      .isEqualTo(personDto);

This means that any objects that are not root will be compared by string, note the regex requires the field not to be empty (empty field = root object).

@armandino
Copy link
Contributor

@joel-costigliola I haven't used withEqualsForFieldsMatchingRegexes() before. This looks very handy. Thank you for the example!

Do you see any issues if the String-comparison option was limited to enums and primitive/wrapper types?

@joel-costigliola
Copy link
Member

I see one issue if we go that way, it won't be clear what the scope of the String-comparison is.
An alternative could be to add something like comparingByToString(Class... types) where one could specify the list of types to compare with toString(), need discussing with the team to see how they feel about it.

@joel-costigliola joel-costigliola self-assigned this Apr 11, 2023
@joel-costigliola joel-costigliola removed the status: team discussion An issue we'd like to discuss as a team to make progress label Apr 17, 2023
@joel-costigliola
Copy link
Member

joel-costigliola commented Apr 17, 2023

We will add comparingEnumsAsString, this addresses the enum case, the other ones can be with withEqualsForFieldsMatchingRegexes if given the proper regex.

@joel-costigliola
Copy link
Member

actually we will stick with withEnumStringComparison which reflects more that we are going to compare an enum to a string.

@armandino
Copy link
Contributor

@joel-costigliola sounds good. I see it's assigned to you, but if help is needed with a PR, I can give it a try.

@joel-costigliola
Copy link
Member

I'm almost done but thanks for the offer @armandino !

@NewbieProger
Copy link

Where did this method goes?

@scordio
Copy link
Member

scordio commented Sep 22, 2023

@NewbieProger this will be available in 3.25.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: recursive comparison An issue related to the recursive comparison
Projects
None yet
Development

No branches or pull requests

5 participants