Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Sort test methods for predictability #293

Merged
merged 9 commits into from

6 participants

@jglick

The order of Class.get(Declared)Methods was never guaranteed to match source order, and apparently could always have been affected by certain conditions relating to the sequence of symbol resolution in the Sun/Oracle VM; but a VM change in JDK 7 (soon to be backported to a JDK 6 update) makes the order quite random from run to run.

This randomness has been reported to break JUnit-based tests in Eclipse, NetBeans, and Lucene sources.

While "correct" test suites/classes should pass or fail regardless of the order of test cases/methods within them, a minority unintentionally rely on the source order to pass. It is better for these to fail predictably, for easy diagnosis and fixing by developers, than to fail randomly - especially if the random failures are manifested on particular JVM versions but not on the diagnoser's JVM.

Jesse Glick added some commits
Jesse Glick Should explicitly specify source=1.5.
JDK 7 javac warns if you do not; Ant inserts it for you but warns.
3448af3
Jesse Glick Preditably sort (test) methods in a class.
The JVM does not guarantee any order.
Visible here as an occasional failure in ParentRunnerTest.useChildHarvester on JDK 7 prior to fix.
cb69050
@dsaff
Owner

Considering...

@dsaff
Owner

"JUnit does not have a defined order for test methods" has been a principle for so long that I want to be sure we think long and hard about whether and how to modify it.

@jglick

The principle seems to be unrecognized by many (most?) users, since the actual behavior on typical VMs has been to follow the source order, which has come to look like a principle - one which is being broken for existing JUnit releases.

https://hg.netbeans.org/core-main/raw-file/default/nbjunit/src/org/netbeans/junit/MethodOrder.java hacks around with the VM method order and would be rendered inoperable by this patch if accepted. However it demonstrates that it is possible to run unmodified test suites with different method ordering policies to look for unintentional order dependencies: natural order; alphabetic, forward or reverse; randomized; or randomized according to a specified seed (inserted into the failure message to help reproduce subtle problems). In order to be useful, the policy needs to be set externally to the test sources (system property or environment variable), not via a test runner as has been suggested, and must work for 3.x TestCases.

It would be possible to implement a policy which would use the bytecode order - identical to source order with javac at least - using a minimalist bytecode scan (no external library dependency). This would essentially enforce the behavior of older Sun VMs (and perhaps others), thus being most compatible.

But I still think a guaranteed alpha order is best: it is transparent, and minimizes the chance that a test will pass for one person yet fail for another... or worse, fail in some continuous builds without related source changes. Really any order would be preferable which is a function of your classes, the JUnit version, and perhaps well-defined system properties.

@dsaff
Owner

Jesse,

Kent and I are planning to talk offline about this in the next couple of days. Get back to you soon.

One concern I have with the current code is that for pre-JDK7 users, upgrading to this patched version will switch them from one stable sort to a different stable sort, which might cause consternation. Or perhaps that's inevitable, and having it happen at an expected point of instability (a library upgrade) is a benefit.

@matthewfarwell

One possible solution would be to introduce a @TestSortOrder annotation which allows the users to specify the order. Would this be an acceptable solution?

I must admit I'd like the option to be able to have my tests run in a particular order (that is not sorted). For instance, I have some tests which test the same code, but with different data sets, one small and one large. I'd like the small data set to run first, especially if I'm in Eclipse.

@stefanbirkner
Collaborator

The Maven Surefire Plugin already supports run order. Maybe its code is helpful.

@dsaff
Owner

I think the way to go is with a pluggable ordering for reflectively-accessed members, with three built-in options:

  • JDK_DEFAULT: whatever order your current JDK uses
  • ALPHA: the ordering defined here
  • RANDOM: explicitly random

Jesse, this is a bigger job. Are you interested in pursuing it?

@jglick

It is my understanding that the relevant changes to HotSpot are planned for a JDK 6 backport, which will affect a lot more people sooner, and yes I think it would be preferable for a change in behavior to occur with a library upgrade which is usually managed; many Java projects merely specify a minimum Java version, so a (minor or major) JDK upgrade occurs outside of version control.

Of course people using VMs unrelated to HotSpot will perceive any behavioral change differently, but that is part of the point - by moving to a VM-independent guaranteed ordering you minimize platform-dependent variation.

Regarding an annotation to specify the order - you could already do this today, in 3.x using a suite method and in 4.x using a custom runner. But this would be appropriate only for cases where you intentionally rely on test order. Most suites affected by the problem are not intentionally relying on order, and the problem is hard-to-reproduce failures in large bodies of existing code.

Regarding switchable ordering - I think this is fine so long as the switch is defined externally to Java sources, perhaps in a system property, for reasons mentioned earlier: it is not desirable to modify thousands of source files in a large existing source base, whereas setting a system property is straightforward to do in an Ant script, Maven POM, direct IDE test runner, etc. (Anyway an annotation would be undesirable for code using only the 3.x API and thus potentially -source 1.4.)

As to the default setting, I think alpha order is the best choice because of portability and predictability; with developers being able to switch to VM order as an interim compatibility mode, or random order for diagnosis (either locally or in a CI job). Obviously the release notes would need to discuss this.

The other mode I can think of would be bytecode order, requiring a minimal bytecode parse, which I believe would be simple enough to include in JUnit sources (though I have not yet tried to prototype it); this is useful insofar as your compiler follows source order.

As to pluggability (not just switchability), this seems like overkill; is there any other expected ordering? Otherwise we would be introducing a new SPI with no foreseeable implementations.

Depending on what policy is decided upon, I can try to do an implementation.

@dsaff
Owner

After more thought, I'm leaning back toward just accepting the original pull request. Strictly speaking, moving from "undefined" to "defined" can't break the actual spec. And the first time JUnit core takes a system property, I'd rather it be for something that absolutely had to be done that way.

Will open for discussion on the list to see if anyone wants to take the other side.

@jglick

Even if an unconfigurable standard order is to be preferred, bytecode order (as determined by a parse) is an option that should be considered - it would be most compatible, least likely to be surprising to users, and most pleasant for reviewing test results. The main objection is that this might differ from source order; I doubt any existing compiler reorders declared methods, but I also doubt the JLS prohibits it. (Sorting by line number attribute, when available, is another option.)

@dsaff
Owner

Jesse,

I'd rather not introduce a dependency from JUnit core on a bytecode parsing library. I've mailed the list, and look forward to feedback.

@jglick

No external bytecode parsing library is needed. Stay tuned.

@Tibor17

Can you explain why we want to sort the methods in a lexical order of their names, and/or to keep the order specified by the class file?
As far as I know, the order of methods retrieved by java.lang.Class#getDeclaredMethods() may not be guaranteed.
Therefore I sounds, to me at least, to take over an idea which exists in TestNG on Test annotation as follows:

@Test(dependsOnMethods = { "serverStartedOk" })

What you think of it?

Of course the developer has to identify top parent test cases (in test inheritance as well), circularity, and a real appearance of test case specified by 'dependsOnMethods' before going to start the first test case.

This solution would definitely declare predictability and an ability to configure the meaning of the order.

@jglick

An annotation or other facility to specify a particular ordering between test cases is fine, but that is orthogonal to this issue: that for existing bodies of test code, random ordering of Class.getDeclaredMethods can lead to hard-to-reproduce failures in suites which unintentionally relied on source order, especially during JDK upgrades.

@Tibor17

@jglick
Ok i think i understand now.
I have to remark on what would happen if the JUnit behavior is going to change and what would be the backwards compatibility.

Basically, now the order of test cases in a test is not guaranteed, however the order of tests in suite is.
This means that coding the test against this restriction in test in past was wrong decision done by a developer who made such decision. This means that after the submit of this feature, the developer's tests may start to fail even on the same JVM, which we can argue against him that he already took a wrong decision before.

Although the ordering of test cases on miscellaneous JVM vendors will behave same, the idea of a strict ordering is against the traditionally good experience. The reason is that the developer who requires this feature is going to use it only because of sharing static context; And that's bad. Basically, the bug is then on side of JUnit design that every test case starts with a new instance of test, however same type. Therefore the developer is forced to use static (global) references.
If the instance if the test was only one, then there would not be a problem. Again solved in TestNG.

The names of test methods may change after some refactoring, and thus this order may change as well (if sorting the Strings in natural order - lexical order). This would be again unpredictable from the point of view of making mistakes. Suppose that the lexical order of test cases would not be well considered by the developer. If the developer shares some global references across test cases (methods), then the refactoring might be a factor of failure exactly same as it was without this feature improvement.

We should think about risk especially the risk, however the idea was initially good. Some weeks ago I was also roughly thinking in my mind about how it could be if we had methods ordering, but it was just my pleasure without influences to the status of current tests. Maybe we should talk more about it next time.

@Tibor17

Bytecode analysis is, sorry for that, but inherently a dirty solution.
I don't like!

@dsaff
Owner

No, let's not do this.

@dsaff
Owner

Jesse,

that's a great trick on the bytecode order, but I think simplicity should win the day.

@dsaff
Owner

The thread at http://tech.groups.yahoo.com/group/junit/message/23685 suggests sorting on String#hashCode. I like this as a compromise: the ordering is deterministic, and stable, but not easily gameable: you can't just say test00runThisFirst, test01RunThisNext, etc. Jesse, what do you think?

src/test/java/org/junit/internal/MethodSorterTest.java
@@ -0,0 +1,34 @@
+package org.junit.internal;
+
+import java.util.Arrays;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+public class MethodSorterTest {
+
+ @Test public void getDeclaredMethods() throws Exception {
+ assertEquals("[void epsilon(), void beta(int[][]), java.lang.Object alpha(int,double,java.lang.Thread), void delta(), int gamma(), void gamma(boolean)]", declaredMethods(Dummy.class));
+ class Super {
@dsaff Owner
dsaff added a note

Can we split out Super and Sub to live next to Dummy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dsaff
Owner

Finally ready to pull, just one change in the tests. Let me know if this has fallen off your priority queue

@jglick

Ready as far as I am concerned.

@matthewfarwell

What was the final decision on the default sort order? Is it hashcode or alpha?

@jglick

The code in the pull request sorts via hash code, i.e. predictable even across platforms but not accidentally "gamed".

@matthewfarwell

So there isn't a method for specifying the sorter then? I must admit I still prefer the test method name sort order, just for the least surprise factor. I completely agree that if the user's tests are order dependent, they are broken, but this doesn't give me a quick way to 'fix' them in the short term.

The other thing that occurs is that it's harder to find a particular test (for instance in the JUnit view in Eclipse, because the names aren't in any (human) predictable order.

If it's final, can we at least add a @SortWith, as was suggested by David in http://tech.groups.yahoo.com/group/junit/message/23693.

@dsaff dsaff merged commit 4f92c3c into from
@dsaff
Owner

@matthewfarwell, custom sorting would certainly make sense as a feature request, but this change handles the primary problem, which was irreproducibility.

@marcphilipp marcphilipp commented on the diff
src/main/java/org/junit/internal/MethodSorter.java
((9 lines not shown))
+ /**
+ * Gets declared methods of a class in a predictable order.
+ * Using the JVM order is unwise since the Java platform does not
+ * specify any particular order, and in fact JDK 7 returns a more or less
+ * random order; well-written test code would not assume any order, but some
+ * does, and a predictable failure is better than a random failure on
+ * certain platforms. Uses an unspecified but deterministic order.
+ * @param clazz a class
+ * @return same as {@link Class#getDeclaredMethods} but sorted
+ * @see <a href="http://bugs.sun.com/view_bug.do?bug_id=7023180">JDK
+ * (non-)bug #7023180</a>
+ */
+ public static Method[] getDeclaredMethods(Class<?> clazz) {
+ Method[] methods = clazz.getDeclaredMethods();
+ Arrays.sort(methods, new Comparator<Method>() {
+ @Override public int compare(Method m1, Method m2) {
@marcphilipp Collaborator

This does not compile in Eclipse because @Override is not allowed on interface implementations.

@jglick
jglick added a note

Indeed JDK 6 javac compiles it (using -source 5) but JDK 5 javac does not; bug in JDK 5 which ecj perhaps copied. I submitted: https://github.com/KentBeck/junit/pull/403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 26, 2011
  1. Should explicitly specify source=1.5.

    Jesse Glick authored
    JDK 7 javac warns if you do not; Ant inserts it for you but warns.
  2. Preditably sort (test) methods in a class.

    Jesse Glick authored
    The JVM does not guarantee any order.
    Visible here as an occasional failure in ParentRunnerTest.useChildHarvester on JDK 7 prior to fix.
Commits on Sep 20, 2011
  1. Use bytecode order of methods when possible.

    Jesse Glick authored
Commits on Sep 22, 2011
  1. Switching to method sort based on hash code - deterministic but hard …

    Jesse Glick authored
    …to abuse.
Commits on Jan 31, 2012
  1. Using MethodSorter here too, just in case it matters.

    Jesse Glick authored
This page is out of date. Refresh to see the latest.
View
1  build.xml
@@ -68,6 +68,7 @@
debug="on"
classpath="@{classpath}"
includeantruntime="false"
+ source="1.5"
target="1.5"
>
<compilerarg value="-Xlint:unchecked" />
View
3  src/main/java/junit/framework/TestSuite.java
@@ -10,6 +10,7 @@
import java.util.Enumeration;
import java.util.List;
import java.util.Vector;
+import org.junit.internal.MethodSorter;
/**
* <p>A <code>TestSuite</code> is a <code>Composite</code> of Tests.
@@ -146,7 +147,7 @@ private void addTestsFromTestCase(final Class<?> theClass) {
Class<?> superClass= theClass;
List<String> names= new ArrayList<String>();
while (Test.class.isAssignableFrom(superClass)) {
- for (Method each : superClass.getDeclaredMethods())
+ for (Method each : MethodSorter.getDeclaredMethods(superClass))
addTestMethod(each, names, theClass);
superClass= superClass.getSuperclass();
}
View
35 src/main/java/org/junit/internal/MethodSorter.java
@@ -0,0 +1,35 @@
+package org.junit.internal;
+
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.Comparator;
+
+public class MethodSorter {
+
+ /**
+ * Gets declared methods of a class in a predictable order.
+ * Using the JVM order is unwise since the Java platform does not
+ * specify any particular order, and in fact JDK 7 returns a more or less
+ * random order; well-written test code would not assume any order, but some
+ * does, and a predictable failure is better than a random failure on
+ * certain platforms. Uses an unspecified but deterministic order.
+ * @param clazz a class
+ * @return same as {@link Class#getDeclaredMethods} but sorted
+ * @see <a href="http://bugs.sun.com/view_bug.do?bug_id=7023180">JDK
+ * (non-)bug #7023180</a>
+ */
+ public static Method[] getDeclaredMethods(Class<?> clazz) {
+ Method[] methods = clazz.getDeclaredMethods();
+ Arrays.sort(methods, new Comparator<Method>() {
+ @Override public int compare(Method m1, Method m2) {
@marcphilipp Collaborator

This does not compile in Eclipse because @Override is not allowed on interface implementations.

@jglick
jglick added a note

Indeed JDK 6 javac compiles it (using -source 5) but JDK 5 javac does not; bug in JDK 5 which ecj perhaps copied. I submitted: https://github.com/KentBeck/junit/pull/403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ int i1 = m1.getName().hashCode();
+ int i2 = m2.getName().hashCode();
+ return i1 != i2 ? i1 - i2 : m1.toString().compareTo(m2.toString());
+ }
+ });
+ return methods;
+ }
+
+ private MethodSorter() {}
+
+}
View
3  src/main/java/org/junit/internal/matchers/TypeSafeMatcher.java
@@ -3,6 +3,7 @@
import java.lang.reflect.Method;
import org.hamcrest.BaseMatcher;
+import org.junit.internal.MethodSorter;
/**
* Convenient base class for Matchers that require a non-null value of a specific type.
@@ -26,7 +27,7 @@ protected TypeSafeMatcher() {
private static Class<?> findExpectedType(Class<?> fromClass) {
for (Class<?> c = fromClass; c != Object.class; c = c.getSuperclass()) {
- for (Method method : c.getDeclaredMethods()) {
+ for (Method method : MethodSorter.getDeclaredMethods(c)) {
if (isMatchesSafelyMethod(method)) {
return method.getParameterTypes()[0];
}
View
3  src/main/java/org/junit/internal/runners/TestClass.java
@@ -11,6 +11,7 @@
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
+import org.junit.internal.MethodSorter;
import org.junit.runners.BlockJUnit4ClassRunner;
/**
@@ -41,7 +42,7 @@ public TestClass(Class<?> klass) {
public List<Method> getAnnotatedMethods(Class<? extends Annotation> annotationClass) {
List<Method> results= new ArrayList<Method>();
for (Class<?> eachClass : getSuperClasses(fClass)) {
- Method[] methods= eachClass.getDeclaredMethods();
+ Method[] methods= MethodSorter.getDeclaredMethods(eachClass);
for (Method eachMethod : methods) {
Annotation annotation= eachMethod.getAnnotation(annotationClass);
if (annotation != null && ! isShadowed(eachMethod, results))
View
3  src/main/java/org/junit/runners/model/TestClass.java
@@ -14,6 +14,7 @@
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
+import org.junit.internal.MethodSorter;
/**
* Wraps a class to be run, providing method validation and annotation searching
@@ -38,7 +39,7 @@ public TestClass(Class<?> klass) {
"Test class can only have one constructor");
for (Class<?> eachClass : getSuperClasses(fClass)) {
- for (Method eachMethod : eachClass.getDeclaredMethods())
+ for (Method eachMethod : MethodSorter.getDeclaredMethods(eachClass))
addToAnnotationLists(new FrameworkMethod(eachMethod),
fMethodsForAnnotations);
for (Field eachField : eachClass.getDeclaredFields())
View
34 src/test/java/org/junit/internal/MethodSorterTest.java
@@ -0,0 +1,34 @@
+package org.junit.internal;
+
+import java.util.Arrays;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+public class MethodSorterTest {
+
+ @Test public void getDeclaredMethods() throws Exception {
+ assertEquals("[void epsilon(), void beta(int[][]), java.lang.Object alpha(int,double,java.lang.Thread), void delta(), int gamma(), void gamma(boolean)]", declaredMethods(Dummy.class));
+ assertEquals("[void testOne()]", declaredMethods(Super.class));
+ assertEquals("[void testTwo()]", declaredMethods(Sub.class));
+ }
+
+ private static String declaredMethods(Class<?> c) {
+ return Arrays.toString(MethodSorter.getDeclaredMethods(c)).replace(c.getName() + '.', "");
+ }
+
+ private static class Dummy {
+ Object alpha(int i, double d, Thread t) {return null;}
+ void beta(int[][] x) {}
+ int gamma() {return 0;}
+ void gamma(boolean b) {}
+ void delta() {}
+ void epsilon() {}
+ }
+ private static class Super {
+ void testOne() {}
+ }
+ private static class Sub extends Super {
+ void testTwo() {}
+ }
+
+}
View
2  src/test/java/org/junit/tests/running/classes/ParentRunnerTest.java
@@ -33,7 +33,7 @@ public void apple() {
}
@Test
- public void banana() {
+ public void /* must hash-sort after "apple" */Banana() {
log+= "banana ";
}
}
Something went wrong with that request. Please try again.