Skip to content

Commit

Permalink
Avoid a potentially expensive reflection-based loop in assertArrayEqu…
Browse files Browse the repository at this point in the history 
…als, in the usual case where the arrays are in fact exactly equal.
  • Loading branch information
@eamonnmcmanus
eamonnmcmanus committed May 22, 2014
1 parent 950702c commit 8f59230
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/main/java/org/junit/internal/ComparisonCriteria.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.junit.internal;

import java.lang.reflect.Array;
import java.util.Arrays;

import org.junit.Assert;

Expand All @@ -24,7 +25,11 @@ public abstract class ComparisonCriteria {
*/
public void arrayEquals(String message, Object expecteds, Object actuals)
throws ArrayComparisonFailure {
if (expecteds == actuals) {
if (expecteds == actuals
|| Arrays.deepEquals(new Object[] {expecteds}, new Object[] {actuals})) {
// The reflection-based loop below is potentially very slow, especially for primitive
// arrays. The deepEquals check allows us to circumvent it in the usual case where
// the arrays are exactly equal.
return;
}
String header = message == null ? "" : message + ": ";
Expand Down

3 comments on commit 8f59230

@dmak
Copy link

@dmak dmak commented on 8f59230 Feb 2, 2015

Choose a reason for hiding this comment

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

This change is for good, but I think it breaks the philosophy of ComparisonCriteria class, that should delegate the comparison of each element array to assertElementsEqual(). Consider the following example:

I have array elements that implement Object.equals(Object) and another specific method isEqualTo(Object). Assume that elmt1.equals(elmt2) == true but elmt1.isEqualTo(elmt2) == false. Now I cannot implement this strategy that utilizes isEqualTo() based on ComparisonCriteria class:

public class MyComparisonCriteria extends ComparisonCriteria {
    @Override
    protected void assertElementsEqual(Object expected, Object actual) {
        assertTrue("Elements " + expected + " and " + actual + " differ", ((Element) expected).isEqualTo(actual));
    }
}

I also think that this optimization is preliminary. The user of ComparisonCriteria should better perform this check, in this case, Assert.internalArrayEquals(String message, Object expecteds, Object actuals):

if (Arrays.deepEquals(new Object[] {expecteds}, new Object[] {actuals})) { return; }
new ExactComparisonCriteria().arrayEquals(message, expecteds, actuals);

@kcooney
Copy link
Member

@kcooney kcooney commented on 8f59230 Feb 2, 2015

Choose a reason for hiding this comment

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

ComparisonCriteria is an internal class. If you want to implement your own comparison code, use Hamcrest or Truth.

@dmak
Copy link

@dmak dmak commented on 8f59230 Feb 4, 2015

Choose a reason for hiding this comment

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

I mean, it's absolutely fine that ComparisonCriteria is implemented that way. I just wanted to underline that from my point of view the API contract is a bit vague as it assumes that assertElementsEqual() will be called to compare elements of the array, but due to the optimization it will not. Treat my message as a general comment, not as issue.

Actually, taboo to use internal classes is generally OK, unless somebody implements the extension for jUnit to build more powerful framework.

Please sign in to comment.