From 06d3bfce732eb594f14e416359aa8be5b6bfbd4b Mon Sep 17 00:00:00 2001 From: Joel Costigliola Date: Fri, 19 Mar 2021 22:29:01 +1300 Subject: [PATCH] Make IterableDiff to always compare actual elements to expected elements and not the the other way around in case the comparison is not symmetrical. This occurs with the recursive comparison when the types being compared are not the same and the actual type has less fields than the expected type (the recursive comparison ignores fields non defined in actual type) Fixes #2147 --- .../assertj/core/internal/IterableDiff.java | 48 +++++++++++---- .../core/internal/IterableDiff_Test.java | 59 ++++++++++++++++++- 2 files changed, 94 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/assertj/core/internal/IterableDiff.java b/src/main/java/org/assertj/core/internal/IterableDiff.java index 5254c605c0..768c4cff58 100644 --- a/src/main/java/org/assertj/core/internal/IterableDiff.java +++ b/src/main/java/org/assertj/core/internal/IterableDiff.java @@ -29,9 +29,9 @@ class IterableDiff { IterableDiff(Iterable actual, Iterable expected, ComparisonStrategy comparisonStrategy) { this.comparisonStrategy = comparisonStrategy; // return the elements in actual that are not in expected: actual - expected - this.unexpected = subtract(actual, expected); + this.unexpected = unexpectedActualElements(actual, expected); // return the elements in expected that are not in actual: expected - actual - this.missing = subtract(expected, actual); + this.missing = missingActualElements(actual, expected); } static IterableDiff diff(Iterable actual, Iterable expected, ComparisonStrategy comparisonStrategy) { @@ -46,27 +46,51 @@ boolean differencesFound() { * Returns the list of elements in the first iterable that are not in the second, i.e. first - second * * @param the element type - * @param first the list we want to subtract from - * @param second the list to subtract + * @param actual the list we want to subtract from + * @param expected the list to subtract * @return the list of elements in the first iterable that are not in the second, i.e. first - second */ - private List subtract(Iterable first, Iterable second) { + private List unexpectedActualElements(Iterable actual, Iterable expected) { List missingInFirst = new ArrayList<>(); // use a copy to deal correctly with potential duplicates - List copyOfSecond = newArrayList(second); - for (Object elementInFirst : first) { - if (iterableContains(copyOfSecond, elementInFirst)) { + List copyOfExpected = newArrayList(expected); + for (Object elementInActual : actual) { + if (isActualElementInExpected(elementInActual, copyOfExpected)) { // remove the element otherwise a duplicate would be found in the case if there is one in actual - iterablesRemoveFirst(copyOfSecond, elementInFirst); + iterablesRemoveFirst(copyOfExpected, elementInActual); } else { - missingInFirst.add(elementInFirst); + missingInFirst.add(elementInActual); } } return unmodifiableList(missingInFirst); } - private boolean iterableContains(Iterable actual, Object value) { - return comparisonStrategy.iterableContains(actual, value); + private boolean isActualElementInExpected(Object elementInActual, List copyOfExpected) { + // the order of comparisonStrategy.areEqual is important if element comparison is not symmetrical, we must compare actual to + // expected but not expected to actual, for ex recursive comparison where: + // - actual element is PersonDto, expected a list of Person + // - Person has more fields than PersonDto => comparing PersonDto to Person is ok as it looks at PersonDto fields only, + // --- the opposite always fails as the reference fields are Person fields and PersonDto does not have all of them. + return copyOfExpected.stream().anyMatch(expectedElement -> comparisonStrategy.areEqual(elementInActual, expectedElement)); + } + + private List missingActualElements(Iterable actual, Iterable expected) { + List missingInExpected = new ArrayList<>(); + // use a copy to deal correctly with potential duplicates + List copyOfActual = newArrayList(actual); + for (Object expectedElement : expected) { + if (iterableContains(copyOfActual, expectedElement)) { + // remove the element otherwise a duplicate would be found in the case if there is one in actual + iterablesRemoveFirst(copyOfActual, expectedElement); + } else { + missingInExpected.add(expectedElement); + } + } + return unmodifiableList(missingInExpected); + } + + private boolean iterableContains(Iterable actual, Object expectedElement) { + return comparisonStrategy.iterableContains(actual, expectedElement); } private void iterablesRemoveFirst(Iterable actual, Object value) { diff --git a/src/test/java/org/assertj/core/internal/IterableDiff_Test.java b/src/test/java/org/assertj/core/internal/IterableDiff_Test.java index b33b57890d..ca2241d5c5 100644 --- a/src/test/java/org/assertj/core/internal/IterableDiff_Test.java +++ b/src/test/java/org/assertj/core/internal/IterableDiff_Test.java @@ -14,6 +14,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.internal.IterableDiff.diff; +import static org.assertj.core.util.Lists.list; import static org.assertj.core.util.Lists.newArrayList; import java.util.List; @@ -105,7 +106,7 @@ void should_not_report_any_differences_between_two_case_sensitive_iterables_acco expected = newArrayList("A", "B", "C", "D"); // WHEN IterableDiff diff = diff(actual, expected, comparisonStrategy); - // THEN + // THEN6 assertThatNoDiff(diff); } @@ -147,4 +148,60 @@ private static void assertThatNoDiff(IterableDiff diff) { assertThat(diff.unexpected).isEmpty(); } + // issue #2147 + @Test + void should_work_when_comparison_strategy_is_not_symmetrical() { + // GIVEN + Address address1 = new Address(12, "xyz", "abc", "432432", "asdsa"); + Address address2 = new Address(13, "xyzx", "abcds", "32432432", "asdsdfsa"); + Address address3 = new Address(14, "xyzsa", "axbc", "4sd32432", "asdsfsda"); + List addressDtoList = list(AddressDto.from(address1), AddressDto.from(address2), AddressDto.from(address3)); + // WHEN/THEN + assertThat(addressDtoList).usingRecursiveComparison() + .isEqualTo(list(address1, address2, address3)); + assertThat(addressDtoList).asList() + .usingRecursiveFieldByFieldElementComparator() + .containsExactly(address1, address2, address3); + } + + static class Address { + int id; + String randomProperty; + String name; + String secondName; + String street; + String postCode; + + public Address(int id, String name, String secondName, String street, String postCode) { + this.id = id; + this.name = name; + this.secondName = secondName; + this.street = street; + this.postCode = postCode; + } + + } + + @SuppressWarnings("unused") + static class AddressDto { + private final int id; + private final String name; + private final String secondName; + private final String street; + private final String postCode; + + public AddressDto(int id, String name, String secondName, String street, String postCode) { + this.id = id; + this.name = name; + this.secondName = secondName; + this.street = street; + this.postCode = postCode; + } + + static AddressDto from(Address le) { + return new AddressDto(le.id, le.name, le.secondName, le.street, le.postCode); + } + + } + }