Skip to content

Commit

Permalink
Track visited values and their comparison differences so that they ca…
Browse files Browse the repository at this point in the history
…n be reused.

Fix #2954 a case where values comparison differences were needed multiple times but the previous values tracking would incorrectly ignore them.
  • Loading branch information
joel-costigliola committed Apr 11, 2023
1 parent 7d2df9c commit 8379cac
Show file tree
Hide file tree
Showing 9 changed files with 314 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,22 @@ public DualValue(FieldLocation fieldLocation, Object actualFieldValue, Object ex
public boolean equals(Object other) {
if (!(other instanceof DualValue)) return false;
DualValue that = (DualValue) other;
// it is critical to compare by reference when tracking visited dual values.
// see should_fix_1854_minimal_test for an explanation
// TODO add field location check?
return actual == that.actual && expected == that.expected;
return actual == that.actual && expected == that.expected && fieldLocation.equals(that.fieldLocation);
}

/**
* If we want to detect potential cycles in the recursive comparison, we need to check if an object has already been visited.
* <p>
* We must ignore the {@link FieldLocation} otherwise we would not find cycles. Let's take for example a {@code Person} class
* with a neighbor field. We have a cycle between the person instance and its neighbor instance, ex: Jack has Tim as neighbor
* and vice versa, when we navigate to Tim we find that its neighbor is Jack, we have already visited it but the location is
* different, Jack is both the root object and root.neighbor.neighbor (Jack=root, Tim=root.neighbor and Tim.neighbor=Jack)
*
* @param dualValue the {@link DualValue} to compare
* @return true if dual values references the same values (ignoring the field location)
*/
public boolean sameValues(DualValue dualValue) {
return actual == dualValue.actual && expected == dualValue.expected;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public int hashCode() {

@Override
public String toString() {
return String.format("FieldLocation [pathToUseInRules=%s, decomposedPath=%s]", pathToUseInRules, decomposedPath);
return String.format("<%s>", pathToUseInRules);
}

public String shortDescription() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@

/**
* Based on {@link DeepDifference} but takes a {@link RecursiveComparisonConfiguration}, {@link DeepDifference}
* being itself based on the deep equals implementation of https://github.com/jdereg/java-util
* being itself based on the deep equals implementation of <a href="https://github.com/jdereg/java-util">https://github.com/jdereg/java-util</a>
*
* @author John DeRegnaucourt (john@cedarsoftware.com)
* @author Pascal Schumacher
Expand All @@ -69,23 +69,28 @@ public class RecursiveComparisonDifferenceCalculator {

private static class ComparisonState {
// Not using a Set as we want to precisely track visited values, a set would remove duplicates
List<DualValue> visitedDualValues;
VisitedDualValues visitedDualValues;
List<ComparisonDifference> differences = new ArrayList<>();
DualValueDeque dualValuesToCompare;
RecursiveComparisonConfiguration recursiveComparisonConfiguration;

public ComparisonState(List<DualValue> visited, RecursiveComparisonConfiguration recursiveComparisonConfiguration) {
this.visitedDualValues = visited;
public ComparisonState(VisitedDualValues visitedDualValues,
RecursiveComparisonConfiguration recursiveComparisonConfiguration) {
this.visitedDualValues = visitedDualValues;
this.dualValuesToCompare = new DualValueDeque(recursiveComparisonConfiguration);
this.recursiveComparisonConfiguration = recursiveComparisonConfiguration;
}

void addDifference(DualValue dualValue) {
differences.add(new ComparisonDifference(dualValue, null, getCustomErrorMessage(dualValue)));
addDifference(dualValue, null);
}

void addDifference(DualValue dualValue, String description) {
differences.add(new ComparisonDifference(dualValue, description, getCustomErrorMessage(dualValue)));
String customErrorMessage = getCustomErrorMessage(dualValue);
ComparisonDifference comparisonDifference = new ComparisonDifference(dualValue, description, customErrorMessage);
differences.add(comparisonDifference);
// track the difference for the given dual values, in case we visit the same dual values again
visitedDualValues.registerComparisonDifference(dualValue, comparisonDifference);
}

void addKeyDifference(DualValue parentDualValue, Object actualKey, Object expectedKey) {
Expand All @@ -102,26 +107,19 @@ public boolean hasDualValuesToCompare() {
}

public DualValue pickDualValueToCompare() {
final DualValue dualValue = dualValuesToCompare.removeFirst();
if (dualValue.hasPotentialCyclingValues()) {
// visited dual values are here to avoid cycle, java types don't have cycle, there is no need to track them.
// moreover this would make should_fix_1854_minimal_test to fail (see the test for a detailed explanation)
visitedDualValues.add(dualValue);
}
return dualValue;
return dualValuesToCompare.removeFirst();
}

private void registerForComparison(DualValue dualValue) {
if (!visitedDualValues.contains(dualValue)) dualValuesToCompare.addFirst(dualValue);
dualValuesToCompare.addFirst(dualValue);
}

private void initDualValuesToCompare(Object actual, Object expected, FieldLocation nodeLocation) {
DualValue dualValue = new DualValue(nodeLocation, actual, expected);
boolean mustCompareNodesRecursively = mustCompareNodesRecursively(dualValue);
if (dualValue.hasNoNullValues() && mustCompareNodesRecursively) {
// disregard the equals method and start comparing fields
// TODO should fail if actual and expected don't have the same fields to compare (taking into account ignored/compared
// fields)
// TODO should fail if actual and expected don't have the same fields (taking into account ignored/compared fields)
Set<String> actualChildrenNodeNamesToCompare = recursiveComparisonConfiguration.getActualChildrenNodeNamesToCompare(dualValue);
if (!actualChildrenNodeNamesToCompare.isEmpty()) {
// fields to ignore are evaluated when adding their corresponding dualValues to dualValuesToCompare which filters
Expand All @@ -134,30 +132,20 @@ private void initDualValuesToCompare(Object actual, Object expected, FieldLocati
Object expectedChildNodeValue = recursiveComparisonConfiguration.getValue(actualChildNodeName, expected);
DualValue childNodeDualValue = new DualValue(nodeLocation.field(actualChildNodeName), actualChildNodeValue,
expectedChildNodeValue);
dualValuesToCompare.addFirst(childNodeDualValue);
registerForComparison(childNodeDualValue);
}
} else {
dualValuesToCompare.addFirst(dualValue);
registerForComparison(dualValue);
}
} else {
dualValuesToCompare.addFirst(dualValue);
registerForComparison(dualValue);
}
} else {
dualValuesToCompare.addFirst(dualValue);
registerForComparison(dualValue);
}
// We need to remove already visited nodes pair to avoid infinite recursion in case parent -> set{child} with child having
// a reference back to its parent but only for complex types can have cycle, this is not the case for primitive or enums.
// It occurs for unordered collection where we compare all possible combination of the collection elements recursively.
// --
// remove visited values one by one, DualValue.equals correctly compare respective actual and expected fields by reference
visitedDualValues.forEach(visitedDualValue -> dualValuesToCompare.stream()
.filter(dualValueToCompare -> dualValueToCompare.equals(visitedDualValue))
.findFirst()
.ifPresent(dualValuesToCompare::remove));
}

private boolean mustCompareNodesRecursively(DualValue dualValue) {

return !recursiveComparisonConfiguration.hasCustomComparator(dualValue)
&& !shouldHonorEquals(dualValue, recursiveComparisonConfiguration)
&& dualValue.hasNoContainerValues();
Expand Down Expand Up @@ -203,20 +191,36 @@ public List<ComparisonDifference> determineDifferences(Object actual, Object exp
if (recursiveComparisonConfiguration.isInStrictTypeCheckingMode() && expectedTypeIsNotSubtypeOfActualType(actual, expected)) {
return list(expectedAndActualTypeDifference(actual, expected));
}
List<DualValue> visited = list();
return determineDifferences(actual, expected, rootFieldLocation(), visited, recursiveComparisonConfiguration);
return determineDifferences(actual, expected, rootFieldLocation(), new VisitedDualValues(), recursiveComparisonConfiguration);
}

// TODO keep track of ignored fields in an RecursiveComparisonExecution class ?

private static List<ComparisonDifference> determineDifferences(Object actual, Object expected, FieldLocation fieldLocation,
List<DualValue> visited,
VisitedDualValues visitedDualValues,
RecursiveComparisonConfiguration recursiveComparisonConfiguration) {
ComparisonState comparisonState = new ComparisonState(visited, recursiveComparisonConfiguration);
ComparisonState comparisonState = new ComparisonState(visitedDualValues, recursiveComparisonConfiguration);
comparisonState.initDualValuesToCompare(actual, expected, fieldLocation);

while (comparisonState.hasDualValuesToCompare()) {

final DualValue dualValue = comparisonState.pickDualValueToCompare();
// if we have already visited the dual value, no need to compute the comparison differences again, this also avoid cycles
Optional<List<ComparisonDifference>> comparisonDifferences = comparisonState.visitedDualValues.registeredComparisonDifferencesOf(dualValue);
if (comparisonDifferences.isPresent()) {
if (!comparisonDifferences.get().isEmpty()) {
comparisonState.addDifference(dualValue, "already visited node but now location is: " + dualValue.fieldLocation);
}
continue;
}

// first time we evaluate this dual value, perform the usual recursive comparison from there

if (dualValue.hasPotentialCyclingValues()) {
// visited dual values are tracked to avoid cycle, java types don't have cycle => no need to keep track of them.
// moreover this would make should_fix_1854_minimal_test to fail (see the test for a detailed explanation)
comparisonState.visitedDualValues.registerVisitedDualValue(dualValue);
}

final Object actualFieldValue = dualValue.actual;
final Object expectedFieldValue = dualValue.expected;
Expand Down Expand Up @@ -767,11 +771,11 @@ private static boolean areEqualUsingComparator(final Object actual, final Object
// this occurs when comparing field of different types, Person.id is an int and PersonDto.id is a long
// TODO maybe we should let the exception bubble up?
// assertion will fail with the current behavior and report other diff so it might be better to keep things this way
System.out.println(format("WARNING: Comparator was not suited to compare '%s' field values:%n" +
"- actual field value : %s%n" +
"- expected field value: %s%n" +
"- comparator used : %s",
fieldName, actual, expected, comparator));
System.out.printf("WARNING: Comparator was not suited to compare '%s' field values:%n" +
"- actual field value : %s%n" +
"- expected field value: %s%n" +
"- comparator used : %s%n",
fieldName, actual, expected, comparator);
return false;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* 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-2023 the original author or authors.
*/
package org.assertj.core.api.recursive.comparison;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import static java.lang.String.format;

class VisitedDualValues {

private List<VisitedDualValue> dualValues;

VisitedDualValues() {
this.dualValues = new ArrayList<>();
}

void registerVisitedDualValue(DualValue dualValue) {
this.dualValues.add(new VisitedDualValue(dualValue));
}

void registerComparisonDifference(DualValue dualValue, ComparisonDifference comparisonDifference) {
this.dualValues.stream()
// register difference on dual values agnostic of location, to take care of values visited several times
.filter(visitedDualValue -> visitedDualValue.dualValue.sameValues(dualValue))
.findFirst()
.ifPresent(visitedDualValue -> visitedDualValue.comparisonDifferences.add(comparisonDifference));
}

Optional<List<ComparisonDifference>> registeredComparisonDifferencesOf(DualValue dualValue) {
return this.dualValues.stream()
// use sameValues to get already visited dual values with different location
.filter(visitedDualValue -> visitedDualValue.dualValue.sameValues(dualValue))
.findFirst()
.map(visitedDualValue -> visitedDualValue.comparisonDifferences);
}

private static class VisitedDualValue {
DualValue dualValue;
List<ComparisonDifference> comparisonDifferences;

VisitedDualValue(DualValue dualValue) {
this.dualValue = dualValue;
this.comparisonDifferences = new ArrayList<>();
}

@Override
public String toString() {
return format("VisitedDualValue[dualValue=%s, comparisonDifferences=%s]", this.dualValue, this.comparisonDifferences);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void toString_should_succeed() {
// WHEN
String result = underTest.toString();
// THEN
then(result).isEqualTo("FieldLocation [pathToUseInRules=location, decomposedPath=[location]]");
then(result).isEqualTo("<location>");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* 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-2023 the original author or authors.
*/
package org.assertj.core.api.recursive.comparison;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.recursive.comparison.FieldLocation.rootFieldLocation;
import static org.assertj.core.util.Lists.list;
import static org.junit.jupiter.params.provider.Arguments.arguments;

import java.util.List;
import java.util.stream.Stream;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

class DualValue_sameValues_Test {

private static final List<String> PATH = list("foo", "bar");

@ParameterizedTest
@MethodSource
void sameValues_should_return_true_when_dual_values_refer_to_the_same_instances(DualValue dualValue1, DualValue dualValue2) {
assertThat(dualValue1.sameValues(dualValue2)).isTrue();
}

static Stream<Arguments> sameValues_should_return_true_when_dual_values_refer_to_the_same_instances() {
Object value1 = new Object();
Object value2 = new Object();
DualValue dualValue1 = new DualValue(rootFieldLocation(), value1, value2);
DualValue dualValue2 = new DualValue(rootFieldLocation(), value1, value2);
DualValue dualValue3 = new DualValue(rootFieldLocation().field("foo"), value1, value2);
return Stream.of(arguments(dualValue1, dualValue2),
arguments(dualValue1, dualValue1),
arguments(dualValue2, dualValue1),
arguments(dualValue1, dualValue3),
arguments(dualValue1, dualValue3));
}

@ParameterizedTest
@MethodSource
void sameValues_should_return_false_when_dual_values_refer_to_different_instances(DualValue dualValue1, DualValue dualValue2) {
assertThat(dualValue1.sameValues(dualValue2)).isFalse();
}

static Stream<Arguments> sameValues_should_return_false_when_dual_values_refer_to_different_instances() {
Object value1 = new Object();
Object value2 = new Object();
Object value3 = new Object();
DualValue dualValue1 = new DualValue(rootFieldLocation(), value1, value2);
DualValue dualValue2 = new DualValue(rootFieldLocation(), value1, value3);
DualValue dualValue3 = new DualValue(rootFieldLocation().field("foo"), value1, value3);
DualValue dualValue4 = new DualValue(rootFieldLocation(), new Object(), value2);
return Stream.of(arguments(dualValue1, dualValue2),
arguments(dualValue2, dualValue1),
arguments(dualValue1, dualValue3),
arguments(dualValue1, dualValue4));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ void should_not_treat_Path_as_Iterable_to_avoid_infinite_recursion() {

// issue #2434
@Test
void should_treat_class_cast_expection_as_comparison_difference_when_comparing_lists() {
void should_treat_class_cast_exception_as_comparison_difference_when_comparing_lists() {
// GIVEN
Wrapper a = new Wrapper(Double.MAX_VALUE);
Wrapper b = new Wrapper(Integer.MAX_VALUE);
Expand All @@ -396,7 +396,7 @@ void should_treat_class_cast_expection_as_comparison_difference_when_comparing_l
}

@Test
void should_report_class_cast_expection_as_comparison_difference() {
void should_report_class_cast_exception_as_comparison_difference() {
// GIVEN
Wrapper actual = new Wrapper(1.0);
Wrapper expected = new Wrapper(5);
Expand All @@ -407,7 +407,7 @@ void should_report_class_cast_expection_as_comparison_difference() {
}

@Test
void should_treat_class_cast_expection_as_comparison_difference_when_comparing_lists_with_specific_equals() {
void should_treat_class_cast_exception_as_comparison_difference_when_comparing_lists_with_specific_equals() {
// GIVEN
Wrapper a = new Wrapper(1.001);
Wrapper b = new Wrapper(1);
Expand All @@ -421,7 +421,7 @@ void should_treat_class_cast_expection_as_comparison_difference_when_comparing_l
}

@Test
void should_treat_class_cast_expection_as_comparison_difference() {
void should_treat_class_cast_exception_as_comparison_difference() {
// GIVEN
Wrapper a = new Wrapper(Double.MAX_VALUE);
Wrapper b = new Wrapper(Integer.MAX_VALUE);
Expand Down
Loading

0 comments on commit 8379cac

Please sign in to comment.