Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #803: improve error message for assertArrayEquals() when multi-dimensional arrays have different lengths #1054

Closed
wants to merge 2 commits into from
Closed

Fix for #803: improve error message for assertArrayEquals() when multi-dimensional arrays have different lengths #1054

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef34f49
Make clearer *which* (nested) arrays have unequal length.
Stephan202 Dec 27, 2014
36edd32
Incorporate feedback on PR #1054.
Stephan202 Dec 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/main/java/org/junit/internal/ComparisonCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ public abstract class ComparisonCriteria {
*/
public void arrayEquals(String message, Object expecteds, Object actuals)
throws ArrayComparisonFailure {
arrayEquals(message, expecteds, actuals, true);
}

private void arrayEquals(String message, Object expecteds, Object actuals, boolean outer)
throws ArrayComparisonFailure {
if (expecteds == actuals
|| Arrays.deepEquals(new Object[] {expecteds}, new Object[] {actuals})) {
// The reflection-based loop below is potentially very slow, especially for primitive
Expand All @@ -35,18 +40,20 @@ public void arrayEquals(String message, Object expecteds, Object actuals)
String header = message == null ? "" : message + ": ";

int expectedsLength = assertArraysAreSameLength(expecteds,
actuals, header);
actuals, outer ? header : "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves a comment. Something like "Only include the user-provided message in the outer exception"


for (int i = 0; i < expectedsLength; i++) {
Object expected = Array.get(expecteds, i);
Object actual = Array.get(actuals, i);

if (isArray(expected) && isArray(actual)) {
try {
arrayEquals(message, expected, actual);
arrayEquals(message, expected, actual, false);
} catch (ArrayComparisonFailure e) {
e.addDimension(i);
throw e;
} catch (AssertionError e) {
throw new ArrayComparisonFailure(header, e, i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here: // array lengths differed

}
} else {
try {
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/org/junit/tests/assertion/AssertionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,26 @@ public void multiDimensionalArraysAreNotEqualNoMessage() {
}
}

@Test
public void multiDimensionalArraysDifferentLengthMessage() {
try {
assertArrayEquals("message", new Object[][]{{true, true}, {false, false}}, new Object[][]{{true, true}, {false}});
fail();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling fail() throws an AssertionError, so you'll get a confusing error message if we ever get here. When testing code that should throw an AssertionError it's best to use this pattern:

try {
    callThatShouldThrowAssertionError();
} catch (AssertionError expected) {
    // optional: make assertions on expected here
    return;
}
fail("Expected AssertionError to be thrown");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will do. (I copied this pattern from the surrounding tests. Will only fix those relevant to this PR, if that's okay with you; I prefer minimal patches.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stephan202 fine to only make this fix for the tests you added. I'll try to find time to commit a change to fix the others.

} catch (AssertionError exception) {
assertEquals("message: arrays first differed at element [1]; array lengths differed, expected.length=2 actual.length=1", exception.getMessage());
}
}

@Test
public void multiDimensionalArraysDifferentLengthNoMessage() {
try {
assertArrayEquals(new Object[][]{{true, true}, {false, false}}, new Object[][]{{true, true}, {false}});
fail();
} catch (AssertionError exception) {
assertEquals("arrays first differed at element [1]; array lengths differed, expected.length=2 actual.length=1", exception.getMessage());
}
}

@Test
public void arraysWithNullElementEqual() {
Object[] objects1 = new Object[]{null};
Expand Down