Skip to content

Commit

Permalink
Fix unintended exceptions in assertLinesMatch
Browse files Browse the repository at this point in the history
This commit adds tests for fast-forward lines surrounded by spaces.
This commit also adds a test for limited, non-terminal fast-forward exceeding
actual lines.

Prior to this commit, isFastForwardLine and parseFastForwardLimit currently
didn't match each other in how they handle spaces on the outside of the
fast-forward markers.

> isFastForwardLine allows them and will detect " >> 2 >> " as a
> fast-forward line, but parseFastForwardLimit does not handle the spaces
> correctly. It only trims the line that substring(int, int) is invoked on
> and does not do so when determining the end index based on the string
> length.
> This can lead to an IndexOutOfBoundsException being thrown for strings
> with three or more surrounding spaces, like "  >> 2 >> ". Strings with
> one or two surround spaces, like " >> 2 >> ", are treated like an
> unlimited fast-forward.

Now, a trimmed line is passed to parseFastForwardLimit before any further
usage. This implementation is analogous to the implementation in
isFastForwardLine that reassigns the trimmed line to the parameter.

Also, an NoSuchElementException is prevented when fast-forward exceeds
the number of actual lines.
  • Loading branch information
MaisiKoleni authored and marcphilipp committed Sep 22, 2021
1 parent 248ac22 commit 68b08a7
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ GitHub.
[[release-notes-5.8.1-junit-jupiter]]
=== JUnit Jupiter

==== Bug Fixes

* `assertLinesMatch()` in `Assertions` no longer fails with a `NoSuchElementException` if
a limited fast-forward followed by at least one more expected line exceeds the remaining
actual lines.
* `assertLinesMatch()` in `Assertions` now handles fast-forwards with leading and trailing
spaces correctly and no longer throws an `IndexOutOfBoundsException`.

==== New Features and Improvements

* `JAVA_18` has been added to the `JRE` enum for use with JRE-based execution conditions.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,10 @@ void assertLinesMatchWithFastForward() {
// fast-forward marker found in expected line: fast-forward actual line...
if (isFastForwardLine(expectedLine)) {
int fastForwardLimit = parseFastForwardLimit(expectedLine);
int actualRemaining = actualDeque.size();

// trivial case: fast-forward marker was in last expected line
if (expectedDeque.isEmpty()) {
int actualRemaining = actualDeque.size();
// no limit given or perfect match? we're done.
if (fastForwardLimit == Integer.MAX_VALUE || fastForwardLimit == actualRemaining) {
return;
Expand All @@ -150,6 +150,10 @@ void assertLinesMatchWithFastForward() {

// fast-forward limit was given: use it
if (fastForwardLimit != Integer.MAX_VALUE) {
if (actualRemaining < fastForwardLimit) {
fail("fast-forward(%d) error: not enough actual lines remaining (%s)", fastForwardLimit,
actualRemaining);
}
// fast-forward now: actualDeque.pop(fastForwardLimit)
for (int i = 0; i < fastForwardLimit; i++) {
actualDeque.pop();
Expand Down Expand Up @@ -202,7 +206,8 @@ static boolean isFastForwardLine(String line) {
}

static int parseFastForwardLimit(String fastForwardLine) {
String text = fastForwardLine.trim().substring(2, fastForwardLine.length() - 2).trim();
fastForwardLine = fastForwardLine.trim();
String text = fastForwardLine.substring(2, fastForwardLine.length() - 2).trim();
try {
int limit = Integer.parseInt(text);
condition(limit > 0, () -> format("fast-forward(%d) limit must be greater than zero", limit));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,19 @@ void assertLinesMatchUsingFastForwardMarkerWithTooHighLimitFails() {
assertError(error, "terminal fast-forward(100) error: fast-forward(2) expected", expected, actual);
}

@Test
void assertLinesMatchUsingFastForwardMarkerWithTooHighLimitAndFollowingLineFails() {
/*
* It is important here that the line counts are expected <= actual, that the
* fast-forward exceeds the available actual lines and that it is not a
* terminal fast-forward.
*/
var expected = List.of("first line", ">> 3 >>", "not present");
var actual = List.of("first line", "first skipped", "second skipped");
var error = assertThrows(AssertionFailedError.class, () -> assertLinesMatch(expected, actual));
assertError(error, "fast-forward(3) error: not enough actual lines remaining (2)", expected, actual);
}

@Test
void assertLinesMatchUsingFastForwardMarkerWithoutMatchingNextLineFails() {
var expected = List.of("first line", ">> fails, because next line is >>", "not present");
Expand All @@ -187,7 +200,8 @@ void assertLinesMatchIsFastForwardLine() {
() -> assertTrue(isFastForwardLine(">> stacktrace >>")),
() -> assertTrue(isFastForwardLine(">> single line, non Integer.parse()-able comment >>")),
() -> assertTrue(isFastForwardLine(">>9>>")), () -> assertTrue(isFastForwardLine(">> 9 >>")),
() -> assertTrue(isFastForwardLine(">> -9 >>")));
() -> assertTrue(isFastForwardLine(">> -9 >>")), () -> assertTrue(isFastForwardLine(" >> 9 >> ")),
() -> assertTrue(isFastForwardLine(" >> 9 >> ")));
}

@Test
Expand All @@ -198,7 +212,9 @@ void assertLinesMatchParseFastForwardLimit() {
() -> assertEquals(Integer.MAX_VALUE, parseFastForwardLimit(">> stacktrace >>")),
() -> assertEquals(Integer.MAX_VALUE, parseFastForwardLimit(">> non Integer.parse()-able comment >>")),
() -> assertEquals(9, parseFastForwardLimit(">>9>>")),
() -> assertEquals(9, parseFastForwardLimit(">> 9 >>")));
() -> assertEquals(9, parseFastForwardLimit(">> 9 >>")),
() -> assertEquals(9, parseFastForwardLimit(" >> 9 >> ")),
() -> assertEquals(9, parseFastForwardLimit(" >> 9 >> ")));
Throwable error = assertThrows(PreconditionViolationException.class, () -> parseFastForwardLimit(">>0>>"));
assertMessageEquals(error, "fast-forward(0) limit must be greater than zero");
error = assertThrows(PreconditionViolationException.class, () -> parseFastForwardLimit(">>-1>>"));
Expand Down

0 comments on commit 68b08a7

Please sign in to comment.