Assertions.assertIterableEquals overflows the stack

[sormuras]

Steps to reproduce

The following code:

var expected = List.of(Path.of("1"));
var actual = List.of(Path.of("1"));
System.out.println(expected);
System.out.println(actual);
assertIterableEquals(expected, actual);

Yields:

[1]
[1]
java.lang.StackOverflowError
	at java.base/java.util.ArrayList.grow(ArrayList.java:243)
	at java.base/java.util.ArrayList.add(ArrayList.java:453)
	at java.base/java.util.ArrayList.add(ArrayList.java:466)
	at java.base/sun.nio.fs.WindowsPath.initOffsets(WindowsPath.java:649)
	at java.base/sun.nio.fs.WindowsPath.getNameCount(WindowsPath.java:660)
	at java.base/java.nio.file.Path$1.hasNext(Path.java:923)
	at org.junit.jupiter.api.AssertIterableEquals.assertIterableEquals(AssertIterableEquals.java:62)
	at org.junit.jupiter.api.AssertIterableEquals.assertIterableElementsEqual(AssertIterableEquals.java:82)
	at org.junit.jupiter.api.AssertIterableEquals.assertIterableEquals(AssertIterableEquals.java:72)
	at org.junit.jupiter.api.AssertIterableEquals.assertIterableElementsEqual(AssertIterableEquals.java:82)
	at org.junit.jupiter.api.AssertIterableEquals.assertIterableEquals(AssertIterableEquals.java:72)
	[...]

Deliverables

• Provide fix and test for comparing Iterable<? extends Iterable<?>> instances.

[jbduncan]

Oh wow, that's a really insidious bug! Thank you very much for finding and fixing it @sormuras. 👏

[jbduncan]

I wonder if this bug could have been caught earlier with mutation testing. 🤔

[sormuras]

I wonder if this bug could have been caught earlier with mutation testing. 🤔

Interesting thought. Perhaps also a candidate for property-based testing ... @jlink?

[jlink]

@sormuras Can you point me to the test? I'll make it a simple property and see if it catches the bug.

[sormuras]

junit5/junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertIterableEqualsAssertionsTests.java

Lines 446 to 452 in 0e39392

	@Test
	// https://github.com/junit-team/junit5/issues/2157
	void assertIterableEqualsWithListOfPath() {
	var expected = listOf(Path.of("1"));
	var actual = listOf(Path.of("1"));
	assertDoesNotThrow(() -> assertIterableEquals(expected, actual));
	}

And the one below -- which is expected to fail. With and with the fix.

Edit: Revert this if-clause

https://github.com/junit-team/junit5/blob/master/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertIterableEquals.java#L67-L69

back to

			if (expectedElement == actualElement) {
				continue;
			}

This will show the StackOverflowError again.

[jlink]

I could detect the bug with the following rather straightforward property:

@Property
<T> void assertingEqualityOfTwoListsShouldNeverThrowException(@ForAll List<T> expected, @ForAll List<T> actual) {
	try {
		assertIterableEquals(expected, actual);
	} catch (AssertionError ignore) {}
}

I had to register a default arbitrary provider for java.nio.Path because there is currently none in jqwik. But that could be fixable, of course. I'm wondering, however, what's so special about Path.

There's an additional drawback: Shrinking does not produce something meaningful in this setup. That means that the falsifying sample is very large and hard to identify.

[marcphilipp]

The special thing about path is that its iterator returns paths whose iterator will return instances that are equal to themselves. For instance, the following Groovy snippet

import java.nio.file.Paths
def path = Paths.get("bar");
println path.iterator().next().equals(path)
println path.iterator().next().is(path)

prints

true
false

The code did not account for this kind of recursiveness.

[jlink]

That's some strange kind of behaviour indeed. Nothing in Iterator's contract that would exclude that, though.