-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
Fix for #803: improve error message for assertArrayEquals() when multi-dimensional arrays have different lengths #1054
Conversation
Previously, JUnit's assertion error message would indicate only that some array lengths x and y were unequal, without indicating whether this pertained to the outer array or some nested array. Now, in case of a length mismatch between two nested arrays, Junit will tell at which indices they reside.
} catch (ArrayComparisonFailure e) { | ||
e.addDimension(i); | ||
throw e; | ||
} catch (AssertionError e) { | ||
throw new ArrayComparisonFailure(header, e, i); |
There was a problem hiding this comment.
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
@Stephan202 Thanks for putting this together so quickly! It's possible that changing this message might break someone, so I would prefer that this pull be made against the |
- Added comments explaining edge cases. - Reworked assertion statements in new unit tests.
@junit-team/junit-committers it's possible (though unlikely) that someone is trying to parse the error messages in a way that would be broken by this change. Do you guys feel comfortable including this in 4.13, or should we commit this against the junit5 branch? (yes, I realize that we may decide to skip 4.13, but let's assume for now that we might release 4.13) |
+1 for including this in 4.13 |
I'm OK with including this in 4.13, with the idea that we would back it out
|
+1 for including this in 4.13 |
Sqashed commits and merged into master. Thanks! |
@Stephan202 could you update the 4.13 release notes to mention this change? Thanks again! |
Done :) |
This is an implementation of the suggestion made in #803. Please note that the two new unit tests may make obsolete part of (but certainly not all of) PR #804.