SortMethodsWith allows the user to choose the order of execution of the methods within a test class #386

Merged
merged 3 commits into from Apr 9, 2012

Projects

None yet

7 participants

@matthewfarwell

The default order of execution of JUnit tests within a class
is deterministic but not predictable. Before 4.11, the
behaviour was to run the test methods in byte code order,
which pre-Java 7 was mostly predictable. Java 7 (and some
previous versions), does not guaranteee the order of execution,
which can change from run to run, so a deterministic sort was introduced.
As a rule, test method execution should be independent of
one another. However, there may be a number of dependent
tests either through error or by design. This class
allows the user to specify the order of execution of test methods.

There are four possibilities:

MethodSorters.DEFAULT: the default value, deterministic, but not predictable
MethodSorters.JVM: the order in which the tests are returned by the JVM, i.e. there is no sorting done
MethodSorters.NAME_ASC: sorted in order of method name, ascending
MethodSorters.NAME_DESC: sorter in order of method name, descending

This is an enhancement to the solution for #293 Sort test methods for predictability, in order that users can temporarily 'fix' their broken tests, from https://github.com/KentBeck/junit/pull/293#issuecomment-2095777

@matthewfarwell matthewfarwell SortMethodsWith allows the user to choose the order of execution of t…
…he methods within a test class.

The default order of execution of JUnit tests within a class
is deterministic but not predictable. Before 4.11, the
behaviour was to run the test methods in byte code order,
which pre-Java 7 was mostly predictable. Java 7 (and some
previous versions), does not guaranteee the order of execution,
which can change from run to run, so a deterministic sort was introduced.
As a rule, test method execution should be independent of
one another. However, there may be a number of dependent
tests either through error or by design. This class
allows the user to specify the order of execution of test methods.

There are four possibilities:

MethodSorters.DEFAULT: the default value, deterministic, but not predictable
MethodSorters.JVM: the order in which the tests are returned by the JVM, i.e. there is no sorting done
MethodSorters.NAME_ASC: sorted in order of method name, ascending
MethodSorters.NAME_DESC: sorter in order of method name, descending
c610a49
@kcooney

I suggest inlining this method; I personally don't think it improves readability of the code. I might be convinced that a method like compare(Method, Method) would improve readability

In fact, I created it for the DEFAULT as well, but forgot to factor it.

@kcooney

style-nit: indentation is a bit inconsistent here and other places. This should line up with line 37 (only indented two spaces). Line 43 should only be indented two spaces.

@kcooney

style-nit: For whatever reason, the code in JUnit doesn't tend to have a space before assignment operators. Could we make these changes consistent? Thanks.

@kcooney

Curious: why do you use Method.toString() here, but Method.getName() for the comparison in NAME_ASC?

In fact, it wasn't me who created the initial deterministic sorter (which was created to stop people gaming the system).

@kcooney

Personally, I don't like abbreviations. Most Java developers I know use IDEs that do completion, so I would call this NAME_ASCENDING or just LEXICOGRAPHIC

Changed to NAME_ASCENDING & NAME_DESCENDING.

@kcooney

You could just do:

return NAME_ASC(m2, m1); // reverse of METHOD_ASC

I added -1 to make it more explicit.

@kcooney

guarantee -> guarantee

I would suggest:

"The order of execution is not guaranteed for Java 7 (and some previous versions), and can even change from run to run, so the order of execution was changed to be deterministic."

@kcooney

I suggest:

"It is recommended that test methods be written so that they are independent of the order that they are executed."

@kcooney

Personally I don't think it's useful to list the possibilities and their meanings in the class Javadoc. Just reference the place where the reference sorter implementations live.

@kcooney

Why do you need these defined here? You could define them in MethodSorters as private static constants.

True, for all except DEFAULT, which needs to be referenced from MethodSorter.

@kcooney

Hmm... this API prevents users from providing their own method sorter implementations. I worry we would go down a dark road where JUnit would have to implement and support every possible way of sorting methods.

How about this:

public interface MethodSorter {
  Comparable<Method> create();
}

Then you can define value like this:

Class<? extends MethodSorter> value();

(not I didn't specify a default; I'm not sure why that would be needed)

We would require that the values passed to SortMethodWith define a public no-arg constructor.

You can still have one place to define the sorters:

public class MethodSorters {
  public static class DefaultMethodSorter implements MethodSorter {

I agree that it prevents users from defining their own method sorter, but that was deliberate on my part. I thought that given the usual opposition to people being able to order tests, it wouldn't be approved. I am, however, open to change it, but I think it should be David's decision. I'll send an email.

@kcooney

I wonder how often developers would want to use this. If we allow developers to define their own MethodSorter implementations, then I vote to remove this; it's trivial to implement (see below)

NAME_DESCENDING is there for completeness.

@kcooney

when can comparator be null?

When we don't want to sort, i.e. for the JVM. We don't actually want to reorder at all.

@kcooney

How about "Defines common {@link MethodSorter} implementations." ?

@kcooney

"Sorts the test methods by the method name, in lexicographic order."

(please make simular changes to the other Javadoc for these enums)

@kcooney

indentation is off here and elsewhere in this file

@kcooney
JUnit member

Thanks for the contribution!

@jglick

Consider adding a mode BYTECODE which would match what users normally expect - order of methods as produced by javac - but without relying on the nondeterminism of Class.getDeclaredMethods. My b40493d (amended with 785a33b) shows that this is not particularly hard.

@dsaff
JUnit member

@matthewfarwell, are you working on responses to the requests here? Thanks.

@matthewfarwell

Yes, but give me a couple of days.

@matthewfarwell

@kcooney Thanks for the feedback Kevin (Cooney). I've pushed another commit with changes.

@matthewfarwell

@dsaff I've updated the code, could you please review what's been done.

There are two things for you to decide:

1) Do you wish to create a standard interface for MethodSorter that anyone can implement, or leave it as I've written it? (see https://github.com/KentBeck/junit/pull/386#commitcomment-1011880)
2) Do you want to add a BYTECODE as well, which would mean bytecode parsing? (see https://github.com/KentBeck/junit/pull/386#issuecomment-4286764)

If all is OK, I'll do the merge with master and resubmit the pull.

@dsaff dsaff and 1 other commented on an outdated diff Apr 6, 2012
src/main/java/org/junit/SortMethodsWith.java
+ * <br/>
+ * <br/>
+ * The default order of execution of JUnit tests within a class is deterministic but not predictable.
+ * The order of execution is not guaranteed for Java 7 (and some previous versions), and can even change
+ * from run to run, so the order of execution was changed to be deterministic (in JUnit 4.11)
+ * <br/>
+ * It is recommended that test methods be written so that they are independent of the order that they are executed.
+ * However, there may be a number of dependent tests either through error or by design.
+ * This class allows the user to specify the order of execution of test methods.
+ * <br/>
+ * For possibilities, see {@link MethodSorters}
+ *
+ * Here is an example:
+ *
+ * <pre>
+ * &#064;SortMethodsWith(MethodSorters.NAME_ASC)
@dsaff
dsaff Apr 6, 2012

NAME_ASCENDING, right?

@matthewfarwell
matthewfarwell Apr 6, 2012

Yep, you're right. Changed.

@dsaff dsaff and 1 other commented on an outdated diff Apr 6, 2012
src/test/java/org/junit/internal/MethodSorterTest.java
@@ -24,11 +78,22 @@ void gamma(boolean b) {}
void delta() {}
void epsilon() {}
}
- private static class Super {
- void testOne() {}
+
+ @Test public void testNameAsc() {
+ assertEquals("[java.lang.Object alpha(int,double,java.lang.Thread), void beta(int[][]), void delta(), void epsilon(), int gamma(), void gamma(boolean)]", declaredMethods(DummySortWithNameAsc.class));
@dsaff
dsaff Apr 6, 2012

Can we write these tests in a way that doesn't end up with the lines so long?

@matthewfarwell
matthewfarwell Apr 6, 2012

Refactored to shorten lines, and reduce duplication at the same time.

@dsaff
JUnit member

Thanks for the work here, @matthewfarwell,

1) We already have a Sorter interface, although we could use a better way to use it declaratively, which is a subject for a different pull request. Two general-purpose sorting methods could get very confusing. Instead, just to avoid confusion, maybe could we rename the annotation to @FixMethodOrder?

2) It would take quite a deafening chorus of user feedback to convince me this project should take on the liabilities of the code complexity of the suggested BYTECODE.

A separate question: do we really think anyone will use NAME_DESCENDING?

@matthewfarwell

Annotation renamed to @FixMethodOrder.

Removed NAME_DESCENDING (you're right, noone will ever use this).

Not going to implement BYTECODE.

When people feel this is ready, I'll do a squash and merge with master.

@dsaff dsaff merged commit 560322d into junit-team:master Apr 9, 2012
@dsaff
JUnit member

Went ahead and merged. Thanks for the hard work!

@dsaff
JUnit member

Matthew,

Is it possible this is related to this bug report?

https://github.com/KentBeck/junit/issues/429

@schlm3

@matthewfarwell Your last change to MethodSorters causes a wrong Javadoc-Entry for the JVM-Sorter:

/** Sorts the test methods by the method name, in reverse lexicographic order */
    JVM(null),
@matthewfarwell

@schim3 this has since been fixed elsewhere: It is now:

/** Leaves the test methods in the order returned by the JVM.
* Note that the order from the JVM may vary from run to run
*/

@KedneckInc

I know I'm getting into this discussion way late, but...

I'd like to argue for having the option to run the tests in the order they're specified in the file. It's not that I'm writing tests that depend on that order, but that I put them in a specific order so that low level methods are tested and confirmed working before higher order methods that depend on the low order methods are tested. It makes it much easier for me to fix any issues that crop up, because I can depend on seeing the low order method tests at the beginning of the junit report, and not mixed up in everything else.

Now, I could do that by using the name ascending order, but that leads to artificial additions to test names that have to change every time I want to change the order.

test_001_foo()
test_002_bar()
test_003_baz()

Oops, it turns out 'bar' needs a method tested in 'baz'. Now I not only have to move the methods around, but I have to change their names too? Bleah.

@kcooney
JUnit member

Java doesn't provide a guaranteed way to get the order methods are declared in a class

@KedneckInc
@kcooney
JUnit member

@KedneckInc Have you considered using @RunWith(Enclosed.class)? You can put the "low order" tests in one nested class and the "high order" tests in another.

@bechte

@KedneckInc @kcooney and just not to forget the new HierarchicalContextRunner, which will allow you to create whole hierarchies of test. Normally, you'd test hierarchies with it, but it also fits perfectly for your use-case.

You find this runner on https://github.com/bechte/junit-hierarchicalcontextrunner/wiki and it can be easily added as a maven dependency to your project. Your test will look like this:

@RunWith(HierarchicalContextRunner.class)
public class FeatureTest {
  @Before
  public void setUp() {
    // like before
  }

  // Define a new "context" for higher level details
  public class HigherLevelDetails {
    //... test like normal...
  }

  // Define a new "context" for lower level details
  public class LowerLevelDetails {
    //... test like normal...
  }
}

Give it a try :-) and if you like let me know how it worked out for you.

All the best,

bechte

@KedneckInc

@bechte Thanks for the suggestion! I'm still working out the best way to use it in my scenario, but it's definitely running the low level tests before the high level tests. At the least, that's the way the report is displayed in Eclipse. I've actually got 7 nested classes. I have run into an occasional problem, but I'm not sure if it's the variant of Eclipse I'm using (Spring Tool Suite), the way I'm using the hierarchy, or something else. It's not easily repeatable either, so no report on it at this time. One thing I need to try is getting the methods within the classes sorted alphabetically so that they happen in the same order every time. Just to keep me from getting lost when I run another test and the method results have rearranged ... again.

Can I use the

@FixMethodOrder(MethodSorters.NAME_ASCENDING)

attribute on each of the sub classes? Having it at the outermost class doesn't seem to affect the nested classes. At least, thats what I think I'm seeing.

Just to make things interesting, I've been using a Perl script to generate the java test code from a table that describes all the tests, their expected outputs, and the inputs to use for each test. That's working pretty good. With over a hundred tests, having the test code automatically generated seemed a good idea. At least that way I could be certain that all the tests are written identically.

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