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

RecursiveComparisonAssert: improvement for comparing field with different types in actual and expected objects. #3165

Open
pcuriel opened this issue Sep 1, 2023 · 6 comments · May be fixed by #3179
Labels
theme: recursive comparison An issue related to the recursive comparison

Comments

@pcuriel
Copy link

pcuriel commented Sep 1, 2023

When using RecursiveComparisonAssert to compare objects of different Classes (e.g., Foo and FooDto), we may have the same fields but with different types (e.g., LocalTime and String).

In these cases, comparison will fail by default.

We can use withEqualsForType, withComparatorForType, withEqualsForFields and withComparatorForFields methods to customize comparison.

For instance, given:

class Foo {
  LocalTime time;
}

class FooDto {
  String time;
}

We can do any of these:

assertThat(foo)
  .usingRecursiveComparison()
  .withEqualsForType((BiPredicate<LocalTime, Object>) (time, timeStr) -> time.equals(LocalTime.parse((String) timeStr)), LocalTime.class)
  .isEqualTo(fooDto);

assertThat(foo)
  .usingRecursiveComparison()
  .withComparatorForType((Comparator<Object>) (time, timeStr) -> ((LocalTime) time).compareTo(LocalTime.parse((String) timeStr)), LocalTime.class)
  .isEqualTo(fooDto);

Assertions.assertThat(foo)
  .usingRecursiveComparison()
  .withEqualsForFields((time, timeStr) -> time.equals(LocalTime.parse((String) timeStr)), "time")
  // Also working as:
  // .withEqualsForFields((BiPredicate<LocalTime, String>) (time, timeStr) -> time.equals(LocalTime.parse(timeStr)), "time")
  .isEqualTo(fooDto);

assertThat(foo)
  .usingRecursiveComparison()
  .withComparatorForFields((time, timeStr) -> ((LocalTime) time).compareTo(LocalTime.parse((String) timeStr)), "time")
  .isEqualTo(fooDto);

The "ForFields" methods just require casting the values (or the BiPredicate in the withEquals case), so they look OK.

However, the "ForType" methods look to hacky, requiring an ugly double casting. I addition, while registering a withComparatorForType using LocalTime may work well (to assert Foo against FooDto), registering it using String (to assert FooDto against Foo) will most likely break equality tests for other fields in the class.

To solve these issues, would it be feasible to add two-type versions of these methods?

public <T, U> SELF withEqualsForTypes(BiPredicate<? super T, ? super U> equals, Class<T> typeInActual, Class<U> typeInExpected)

// BiComparator should also be defined
public <T, U> SELF withComparatorForType(BiComparator<? super T, ? super U> comparator, Class<T> typeInActual, Class<U> typeInExpected)
@joel-costigliola joel-costigliola added the theme: recursive comparison An issue related to the recursive comparison label Sep 4, 2023
@joel-costigliola
Copy link
Member

@pcuriel could you show us on an example what the use of these new methods would look like compared to the existing API ?
I'm not opposed to this addition, I want to see the goodness with my own eyes before 😉

@pcuriel
Copy link
Author

pcuriel commented Sep 5, 2023

For sure.

Continuing with my example above. If we have these:

class Foo {
  LocalTime time;
}

class FooDto {
  String time;
}

With the new methods, the idea is that instead of this:

assertThat(foo)
  .usingRecursiveComparison()
  .withEqualsForType((BiPredicate<LocalTime, Object>) (time, timeStr) -> time.equals(LocalTime.parse((String) timeStr)), LocalTime.class)
  .isEqualTo(fooDto);

We would be able to do this:

assertThat(foo)
  .usingRecursiveComparison()
  .withEqualsForTypes((time, timeStr) -> time.equals(LocalTime.parse(timeStr)), LocalTime.class, String.class)
  .isEqualTo(fooDto);




In addition, if we have something like this:

class Bar {
  LocalTime time;
  LocalDate date;
}

class BarDto {
  String time;
  String date;
}

And we want to assert that BarDto is equal to Bar, using "withEqualsForType" does not work:

DateTimeFormatter dateFormatter = DateTimeFormatter.ofPattern("yyyy/MM/dd");
DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm");

assertThat(barDto)
    .usingRecursiveComparison()
    // This is overridden...
    .withEqualsForType((BiPredicate<String, Object>) (dateStr, date) -> dateStr.equals(dateFormatter.format((LocalDate) date)), String.class)
    // when registering this
    .withEqualsForType((BiPredicate<String, Object>) (timeStr, time) -> timeStr.equals(timeFormatter.format((LocalTime) time)), String.class)
    .isEqualTo(bar);

While "withEqualsForTypes" should be OK, because one is registered with String & LocalDate, whereas the other is registered with String & LocalTime:

    ...
    .withEqualsForTypes((dateStr, date) -> dateStr.equals(dateFormatter.format(date)), String.class, LocalDate.class)
    .withEqualsForTypes((timeStr, time) -> timeStr.equals(timeFormatter.format(time)), String.class, LocalTime.class)
    ...

With objects so simple as the ones used here it is true that we can just resort to using "withEqualsForFields" as explained in my initial post.
But with more complex objects, where a given type is used in several fields, I think having the ability to define both actual and expected types may come handy.

@joel-costigliola
Copy link
Member

Yeah, fair enough, would have time contributing this one ?

@pcuriel
Copy link
Author

pcuriel commented Sep 12, 2023

Currently I don't have much time, but I'll try to work on it if it's still pending in a few weeks.

@amodolo
Copy link
Contributor

amodolo commented Sep 12, 2023

I can take it if you would

@joel-costigliola
Copy link
Member

@amodolo thanks !

@amodolo amodolo linked a pull request Sep 15, 2023 that will close this issue
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

Successfully merging a pull request may close this issue.

3 participants