From 035eac999d506c61e1ccfe70f19960a9312e8e7c Mon Sep 17 00:00:00 2001 From: Thomas Zub Date: Sun, 19 Jun 2022 15:12:33 +0200 Subject: [PATCH] Track already visited pairs of Iterables to avoid stack overflow Resolves #2915. Co-authored-by: Thomas Zub Co-authored-by: Marc Philipp --- .../jupiter/api/AssertIterableEquals.java | 88 +++++++++++++++++-- .../AssertIterableEqualsAssertionsTests.java | 15 ++++ 2 files changed, 94 insertions(+), 9 deletions(-) diff --git a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertIterableEquals.java b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertIterableEquals.java index a5f1641dfd03..c0edc0825335 100644 --- a/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertIterableEquals.java +++ b/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertIterableEquals.java @@ -19,6 +19,8 @@ import java.util.ArrayDeque; import java.util.Deque; import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; @@ -49,6 +51,11 @@ static void assertIterableEquals(Iterable expected, Iterable actual, Suppl private static void assertIterableEquals(Iterable expected, Iterable actual, Deque indexes, Object messageOrSupplier) { + assertIterableEquals(expected, actual, indexes, messageOrSupplier, new LinkedHashMap<>()); + } + + private static void assertIterableEquals(Iterable expected, Iterable actual, Deque indexes, + Object messageOrSupplier, Map investigatedElements) { if (expected == actual) { return; @@ -60,28 +67,61 @@ private static void assertIterableEquals(Iterable expected, Iterable actua int processed = 0; while (expectedIterator.hasNext() && actualIterator.hasNext()) { - processed++; Object expectedElement = expectedIterator.next(); Object actualElement = actualIterator.next(); - if (Objects.equals(expectedElement, actualElement)) { - continue; - } + indexes.addLast(processed); + + assertIterableElementsEqual(expectedElement, actualElement, indexes, messageOrSupplier, + investigatedElements); - indexes.addLast(processed - 1); - assertIterableElementsEqual(expectedElement, actualElement, indexes, messageOrSupplier); indexes.removeLast(); + processed++; } assertIteratorsAreEmpty(expectedIterator, actualIterator, processed, indexes, messageOrSupplier); } private static void assertIterableElementsEqual(Object expected, Object actual, Deque indexes, - Object messageOrSupplier) { + Object messageOrSupplier, Map investigatedElements) { + + // If both are equal, we don't need to check recursively. + if (Objects.equals(expected, actual)) { + return; + } + + // If both are iterables, we need to check whether they contain the same elements. if (expected instanceof Iterable && actual instanceof Iterable) { - assertIterableEquals((Iterable) expected, (Iterable) actual, indexes, messageOrSupplier); + + Pair pair = new Pair(expected, actual); + + // Before comparing their elements, we check whether we have already checked this pair. + Status status = investigatedElements.get(pair); + + // If we've already determined that both contain the same elements, we don't need to check them again. + if (status == Status.CONTAIN_SAME_ELEMENTS) { + return; + } + + // If the pair is already under investigation, we fail to avoid infinite recursion. + if (status == Status.UNDER_INVESTIGATION) { + indexes.removeLast(); + failIterablesNotEqual(expected, actual, indexes, messageOrSupplier); + } + + // Otherwise, we put the pair under investigation and recurse. + investigatedElements.put(pair, Status.UNDER_INVESTIGATION); + + assertIterableEquals((Iterable) expected, (Iterable) actual, indexes, messageOrSupplier, + investigatedElements); + + // If we reach this point, we've checked that the two iterables contain the same elements so we store this information + // in case we come across the same pair again. + investigatedElements.put(pair, Status.CONTAIN_SAME_ELEMENTS); } - else if (!Objects.equals(expected, actual)) { + + // Otherwise, they are neither equal nor iterables, so we fail. + else { assertIterablesNotNull(expected, actual, indexes, messageOrSupplier); failIterablesNotEqual(expected, actual, indexes, messageOrSupplier); } @@ -131,4 +171,34 @@ private static void failIterablesNotEqual(Object expected, Object actual, Deque< fail(prefix + message); } + private final static class Pair { + private final Object left; + private final Object right; + + public Pair(Object left, Object right) { + this.left = left; + this.right = right; + } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + Pair that = (Pair) o; + return Objects.equals(this.left, that.left) // + && Objects.equals(this.right, that.right); + } + + @Override + public int hashCode() { + return Objects.hash(left, right); + } + } + + private enum Status { + UNDER_INVESTIGATION, CONTAIN_SAME_ELEMENTS + } + } diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertIterableEqualsAssertionsTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertIterableEqualsAssertionsTests.java index a68d86643869..dc3d10b3e250 100644 --- a/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertIterableEqualsAssertionsTests.java +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertIterableEqualsAssertionsTests.java @@ -83,6 +83,7 @@ void assertIterableEqualsNestedIterablesWithIntegers() { listOf(listOf(1), listOf(2), listOf(listOf(3, listOf(4))))); assertIterableEquals(setOf(setOf(1), setOf(2), setOf(setOf(3, setOf(4)))), setOf(setOf(1), setOf(2), setOf(setOf(3, setOf(4))))); + assertIterableEquals(listOf(listOf(1), listOf(listOf(1))), setOf(setOf(1), setOf(setOf(1)))); } @Test @@ -460,4 +461,18 @@ void assertIterableEqualsThrowsStackOverflowErrorForInterlockedRecursiveStructur assertThrows(StackOverflowError.class, () -> assertIterableEquals(expected, actual)); } + @Test + // https://github.com/junit-team/junit5/issues/2915 + void assertIterableEqualsWithDifferentListOfPath() { + try { + var expected = listOf(Path.of("1").resolve("2")); + var actual = listOf(Path.of("1").resolve("3")); + assertIterableEquals(expected, actual); + expectAssertionFailedError(); + } + catch (AssertionFailedError ex) { + assertMessageEquals(ex, "iterable contents differ at index [0][1], expected: <2> but was: <3>"); + } + } + }