Sort test methods for predictability #293

Merged
merged 9 commits into from Feb 1, 2012

Conversation

Projects
None yet
6 participants
Contributor

jglick commented Aug 26, 2011

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 Aug 26, 2011

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
Owner

dsaff commented Sep 8, 2011

Considering...

Owner

dsaff commented Sep 9, 2011

"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.

Contributor

jglick commented Sep 9, 2011

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.

Owner

dsaff commented Sep 12, 2011

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.

Contributor

matthewfarwell commented Sep 14, 2011

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.

Contributor

stefanbirkner commented Sep 14, 2011

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

Owner

dsaff commented Sep 14, 2011

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?

Contributor

jglick commented Sep 19, 2011

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.

Owner

dsaff commented Sep 19, 2011

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.

Contributor

jglick commented Sep 19, 2011

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.)

Owner

dsaff commented Sep 19, 2011

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.

Contributor

jglick commented Sep 19, 2011

No external bytecode parsing library is needed. Stay tuned.

Contributor

Tibor17 commented Sep 21, 2011

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.

Contributor

jglick commented Sep 21, 2011

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.

Contributor

Tibor17 commented Sep 21, 2011

@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.

Contributor

Tibor17 commented Sep 21, 2011

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

dsaff commented on b40493d Sep 21, 2011

No, let's not do this.

Owner

dsaff commented Sep 21, 2011

Jesse,

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

Owner

dsaff commented Sep 21, 2011

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?

@dsaff dsaff commented on an outdated diff Jan 31, 2012

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

dsaff Jan 31, 2012

Owner

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

Owner

dsaff commented Jan 31, 2012

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

Contributor

jglick commented Jan 31, 2012

Ready as far as I am concerned.

Contributor

matthewfarwell commented Jan 31, 2012

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

Contributor

jglick commented Jan 31, 2012

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

Contributor

matthewfarwell commented Jan 31, 2012

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 added a commit that referenced this pull request Feb 1, 2012

@dsaff dsaff Merge pull request #293 from jglick/master
Sort test methods for predictability
4f92c3c

@dsaff dsaff merged commit 4f92c3c into junit-team:master Feb 1, 2012

Owner

dsaff commented Feb 1, 2012

@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 Mar 21, 2012

src/main/java/org/junit/internal/MethodSorter.java
+ /**
+ * 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

marcphilipp Mar 21, 2012

Owner

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

@jglick

jglick Mar 21, 2012

Contributor

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