Skip to content

Commit

Permalink
Fixes a regression in field by field recursive comparison where we di…
Browse files Browse the repository at this point in the history
…d not show which actual's class fields were not declared in the expected class thus making the comparison impossible.

Fixes #1158
  • Loading branch information
joel-costigliola committed Feb 14, 2018
1 parent 7f210d5 commit 0f31a22
Show file tree
Hide file tree
Showing 7 changed files with 191 additions and 30 deletions.
Expand Up @@ -60,12 +60,15 @@ private static String describeDifference(Difference difference, Representation r
? representation.unambiguousToStringOf(difference.getOther())
: otherFieldValue;

String additionalInfo = difference.getDescription()
.map(desc -> format("%n- reason : %s", escapePercent(desc)))
.orElse("");
return format("%nPath to difference: <%s>%n" +
"- expected: <%s>%n" +
"- actual : <%s>",
"- actual : <%s>%n" +
"- expected: <%s>" + additionalInfo,
join(difference.getPath()).with("."),
escapePercent(otherFieldValueRepresentation),
escapePercent(actualFieldValueRepresentation));
escapePercent(actualFieldValueRepresentation),
escapePercent(otherFieldValueRepresentation));
}

protected static String escapePercent(String value) {
Expand Down
31 changes: 27 additions & 4 deletions src/main/java/org/assertj/core/internal/DeepDifference.java
Expand Up @@ -12,9 +12,11 @@
*/
package org.assertj.core.internal;

import static java.lang.String.format;
import static org.assertj.core.internal.Objects.getDeclaredFieldsIncludingInherited;
import static org.assertj.core.internal.Objects.propertyOrFieldValuesAreEqual;
import static org.assertj.core.internal.TypeComparators.defaultTypeComparators;
import static org.assertj.core.util.Sets.newHashSet;
import static org.assertj.core.util.Strings.join;
import static org.assertj.core.util.introspection.PropertyOrFieldSupport.COMPARISON;

Expand All @@ -31,6 +33,7 @@
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.SortedMap;
import java.util.SortedSet;
Expand All @@ -47,6 +50,7 @@
*/
public class DeepDifference {

private static final String MISSING_FIELDS = "%s can't be compared to %s as %s does not declare all %s fields, it lacks these:%s";
private static final Map<Class<?>, Boolean> customEquals = new ConcurrentHashMap<>();
private static final Map<Class<?>, Boolean> customHash = new ConcurrentHashMap<>();

Expand Down Expand Up @@ -98,11 +102,17 @@ public static class Difference {
List<String> path;
Object actual;
Object other;
Optional<String> description;

public Difference(List<String> path, Object actual, Object other) {
this(path, actual, other, null);
}

public Difference(List<String> path, Object actual, Object other, String description) {
this.path = path;
this.actual = actual;
this.other = other;
this.description = Optional.ofNullable(description);
}

public List<String> getPath() {
Expand All @@ -117,9 +127,15 @@ public Object getOther() {
return other;
}

public Optional<String> getDescription() {
return description;
}

@Override
public String toString() {
return "Difference [path=" + path + ", actual=" + actual + ", other=" + other + "]";
return description.isPresent()
? format("Difference [path=%s, actual=%s, other=%s, description=%s]", path, actual, other, description.get())
: format("Difference [path=%s, actual=%s, other=%s]", path, actual, other);
}
}

Expand Down Expand Up @@ -150,7 +166,7 @@ public static List<Difference> determineDifferences(Object a, Object b,
TypeComparators comparatorByType) {
// replace null comparators groups by empty one to simplify code afterwards
comparatorByPropertyOrField = comparatorByPropertyOrField == null
? new TreeMap<String, Comparator<?>>()
? new TreeMap<>()
: comparatorByPropertyOrField;
comparatorByType = comparatorByType == null ? defaultTypeComparators() : comparatorByType;
return determineDifferences(a, b, null, comparatorByPropertyOrField, comparatorByType);
Expand Down Expand Up @@ -298,7 +314,14 @@ private static List<Difference> determineDifferences(Object a, Object b, List<St
Set<String> key1FieldsNames = getFieldsNames(getDeclaredFieldsIncludingInherited(key1.getClass()));
Set<String> key2FieldsNames = getFieldsNames(getDeclaredFieldsIncludingInherited(key2.getClass()));
if (!key2FieldsNames.containsAll(key1FieldsNames)) {
differences.add(new Difference(currentPath, key1, key2));
Set<String> key1FieldsNamesNotInKey2 = newHashSet(key1FieldsNames);
key1FieldsNamesNotInKey2.removeAll(key2FieldsNames);
String missingFields = key1FieldsNamesNotInKey2.toString();
String key2ClassName = key2.getClass().getName();
String key1ClassName = key1.getClass().getName();
String missingFieldsDescription = format(MISSING_FIELDS, key1ClassName, key2ClassName, key2.getClass().getSimpleName(),
key1.getClass().getSimpleName(), missingFields);
differences.add(new Difference(currentPath, key1, key2, missingFieldsDescription));
} else {
for (String fieldName : key1FieldsNames) {
List<String> path = new ArrayList<>(currentPath);
Expand Down Expand Up @@ -330,7 +353,7 @@ private static Deque<DualKey> initStack(Object a, Object b, List<String> parentP
TypeComparators comparatorByType) {
Deque<DualKey> stack = new LinkedList<>();
boolean isRootObject = parentPath == null;
List<String> currentPath = isRootObject ? new ArrayList<String>() : parentPath;
List<String> currentPath = isRootObject ? new ArrayList<>() : parentPath;
DualKey basicDualKey = new DualKey(currentPath, a, b);
if (a != null && b != null && !isContainerType(a) && !isContainerType(b)
&& (isRootObject || !hasCustomComparator(basicDualKey, comparatorByPropertyOrField, comparatorByType))) {
Expand Down
31 changes: 31 additions & 0 deletions src/test/java/org/assertj/core/error/Person.java
@@ -0,0 +1,31 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* Copyright 2012-2018 the original author or authors.
*/
package org.assertj.core.error;

class Person {
public String getFirstName() {
return firstName;
}

public String getLastName() {
return lastName;
}

private String firstName, lastName;

public Person(String firstName, String lastName) {
this.firstName = firstName;
this.lastName = lastName;
}

}
43 changes: 43 additions & 0 deletions src/test/java/org/assertj/core/error/PersonDAO.java
@@ -0,0 +1,43 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* Copyright 2012-2018 the original author or authors.
*/
package org.assertj.core.error;

class PersonDAO {

public String getFirstname() {
return firstname;
}

public String getLastname() {
return lastname;
}

public Long getId() {
return id;
}

public Integer getAge() {
return age;
}

private String firstname, lastname;
private Long id;
private Integer age;

public PersonDAO(String firstname, String lastname, Long id, Integer age) {
this.firstname = firstname;
this.lastname = lastname;
this.id = id;
this.age = age;
}
}
Expand Up @@ -18,6 +18,7 @@
import static org.assertj.core.api.Assertions.catchThrowable;
import static org.assertj.core.configuration.ConfigurationProvider.CONFIGURATION_PROVIDER;
import static org.assertj.core.error.ShouldBeEqualByComparingFieldByFieldRecursively.shouldBeEqualByComparingFieldByFieldRecursive;
import static org.assertj.core.internal.DeepDifference.determineDifferences;

import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
Expand All @@ -26,15 +27,17 @@
import java.util.TreeSet;

import org.assertj.core.description.TextDescription;
import org.assertj.core.internal.DeepDifference;
import org.assertj.core.internal.DeepDifference.Difference;
import org.assertj.core.internal.objects.Objects_assertIsEqualToComparingFieldByFieldRecursive_Test.WithCollection;
import org.assertj.core.internal.objects.Objects_assertIsEqualToComparingFieldByFieldRecursive_Test.WithMap;
import org.assertj.core.presentation.Representation;
import org.assertj.core.test.Jedi;
import org.junit.Test;

public class ShouldBeEqualByComparingFieldByFieldRecursively_create_Test {

private static final Representation REPRESENTATION = CONFIGURATION_PROVIDER.representation();

@Test
public void should_throw_assertion_error_rather_than_null_pointer_when_one_nested_member_is_null() {
// GIVEN
Expand All @@ -45,7 +48,7 @@ public void should_throw_assertion_error_rather_than_null_pointer_when_one_neste
Throwable throwable2 = catchThrowable(() -> assertThat(noname).isEqualToComparingFieldByFieldRecursively(yoda));
// THEN
assertThat(throwable1).isInstanceOf(AssertionError.class)
.isNotInstanceOf(NullPointerException.class);
.isNotInstanceOf(NullPointerException.class);
assertThat(throwable2).isInstanceOf(AssertionError.class)
.isNotInstanceOf(NullPointerException.class);
}
Expand All @@ -58,14 +61,14 @@ public void should_use_unambiguous_fields_description_when_standard_description_
withHashSet.collection.add("bar");
withHashSet.collection.add("foo");
withSortedSet.collection.addAll(withHashSet.collection);
List<Difference> differences = DeepDifference.determineDifferences(withHashSet, withSortedSet, null, null);
List<Difference> differences = determineDifferences(withHashSet, withSortedSet, null, null);
// WHEN
// @format:off
String message = shouldBeEqualByComparingFieldByFieldRecursive(withSortedSet,
withHashSet,
differences,
CONFIGURATION_PROVIDER.representation())
.create(new TextDescription("Test"), CONFIGURATION_PROVIDER.representation());
REPRESENTATION)
.create(new TextDescription("Test"), REPRESENTATION);
// @format:on
// THEN
assertThat(message).isEqualTo(format("[Test] %n" +
Expand All @@ -76,8 +79,8 @@ public void should_use_unambiguous_fields_description_when_standard_description_
"when recursively comparing field by field, but found the following difference(s):%n"
+ "%n" +
"Path to difference: <collection>%n" +
"- expected: <[\"bar\", \"foo\"] (TreeSet@%s)>%n" +
"- actual : <[\"bar\", \"foo\"] (LinkedHashSet@%s)>",
"- actual : <[\"bar\", \"foo\"] (LinkedHashSet@%s)>%n" +
"- expected: <[\"bar\", \"foo\"] (TreeSet@%s)>",
toHexString(withSortedSet.collection.hashCode()),
toHexString(withHashSet.collection.hashCode())));
}
Expand All @@ -90,28 +93,61 @@ public void should_use_unambiguous_fields_description_when_standard_description_
withLinkedHashMap.map.put(1L, true);
withLinkedHashMap.map.put(2L, false);
withTreeMap.map.putAll(withLinkedHashMap.map);
List<Difference> differences = DeepDifference.determineDifferences(withLinkedHashMap, withTreeMap, null, null);
List<Difference> differences = determineDifferences(withLinkedHashMap, withTreeMap, null, null);
// WHEN
// @format:off
String message = shouldBeEqualByComparingFieldByFieldRecursive(withTreeMap,
withLinkedHashMap,
differences,
CONFIGURATION_PROVIDER.representation())
.create(new TextDescription("Test"), CONFIGURATION_PROVIDER.representation());
REPRESENTATION)
.create(new TextDescription("Test"), REPRESENTATION);
// @format:on
// THEN
assertThat(message).isEqualTo(format("[Test] %n" +
"Expecting:%n" +
" <WithMap [map={1=true, 2=false}]>%n" +
"to be equal to:%n" +
" <WithMap [map={1=true, 2=false}]>%n" +
"when recursively comparing field by field, but found the following difference(s):%n"
+ "%n" +
"when recursively comparing field by field, but found the following difference(s):%n" +
"%n" +
"Path to difference: <map>%n" +
"- expected: <{1L=true, 2L=false} (TreeMap@%s)>%n" +
"- actual : <{1L=true, 2L=false} (LinkedHashMap@%s)>",
"- actual : <{1L=true, 2L=false} (LinkedHashMap@%s)>%n" +
"- expected: <{1L=true, 2L=false} (TreeMap@%s)>",
toHexString(withTreeMap.map.hashCode()),
toHexString(withLinkedHashMap.map.hashCode())));
}

@Test
public void should_precise_missing_fields_when_actual_does_not_declare_all_expected_fields() {
// GIVEN
Person person = new Person("John", "Doe");
PersonDAO personDAO = new PersonDAO("John", "Doe", 1L, 23);
// THEN
List<Difference> differences = determineDifferences(person, personDAO, null, null);
// WHEN
// @format:off
String message = shouldBeEqualByComparingFieldByFieldRecursive(person,
personDAO,
differences,
REPRESENTATION)
.create(new TextDescription("Test"), REPRESENTATION);
// @format:on
// THEN
String personHash = toHexString(person.hashCode());
String personDAOHash = toHexString(personDAO.hashCode());
assertThat(message).isEqualTo(format("[Test] %n" +
"Expecting:%n" +
" <org.assertj.core.error.Person@%s>%n" +
"to be equal to:%n" +
" <org.assertj.core.error.PersonDAO@%s>%n" +
"when recursively comparing field by field, but found the following difference(s):%n" +
"%n" +
"Path to difference: <>%n" +
"- actual : <org.assertj.core.error.Person@%s>%n" +
"- expected: <org.assertj.core.error.PersonDAO@%s>%n" +
"- reason : org.assertj.core.error.Person can't be compared to org.assertj.core.error.PersonDAO as PersonDAO does not declare all Person fields, it lacks these:[firstName, lastName]",
personHash, personDAOHash,
personHash, personDAOHash));
}

}
4 changes: 4 additions & 0 deletions src/test/java/org/assertj/core/internal/ObjectsBaseTest.java
Expand Up @@ -35,6 +35,10 @@
*/
public class ObjectsBaseTest {

{
Assertions.setRemoveAssertJRelatedElementsFromStackTrace(false);
}

@Rule
public ExpectedException thrown = none();

Expand Down

0 comments on commit 0f31a22

Please sign in to comment.