Add Ordering, Orderable and @OrderWith

[kcooney]

These APIs allow arbitrary ordering of tests, including randomization.

[dsaff]

Woah, nice!

Beginning to dig in. Thanks for the work here.

[dsaff]

Name and comment here is a bit confusing. I might call it DelegatingOrdering?

[kcooney]

@dsaff I would rather not call it DelegatingOrdering. It is specifically an Ordering that is known not to be a Sorter, and some of the core code depends on that. I'm open to other names and improved doc, of course.

[dsaff]

Another thing potentially worth considering sooner-rather-than-later: I imagine that the request will come down the road for a --sort_with annotation to complement the existing --filter_with. If a class has OrderWith, and the command line has --order_with, who wins? (I assume we just say "command line" and move on, but if you have another idea?)

[dsaff]

Holding off on digging for further feedback while top-level issues settle down. Thanks again!

[kcooney]

Do we thing that we have many users that use the JUnit command line flags to run tests where it makes sense to add --sort_with? I suspect most users run tests via an IDE, Ant, Maven or Gradle. Each of those would have their own way of adding a sort, and can do so via Request.orderWith(). If someone wanted us to support this in the command line, it wouldn't be much work, but I'd rather not do it in the same pull as the pull will get too large.

[kcooney]

I do appreciate the feedback, and I'm open to looking for a more general concept there. I'm just pushing back because @OrderWith(MyOrdering.class) is extremely simple and self-documenting, while some of the APIs you've proposed are less simple. Many people have asked us for a way to specify rules via an annotation, because having a field was "too much code" or because "fields imply state", and while I don't always agree with them, it's happened enough times that I'm always trying to ask myself "is this the simplest and easiest to use API for this concept?"

[dsaff]

A couple non-exhaustive thoughts:

1. I'm throwing out a bunch of alternatives not necessarily because I consider them obviously better, but because I think we'll be better off to have explicitly considered and rejected (or accepted) them; hopefully that comes across.

2. I had missed that this is plugged in through ParentRunner, not through the top-level RunnerBuilders.
   a) On the one hand, that reduces the scope of the change, and also makes it more likely that systems that use custom RunnerBuilders (android test system, for example) will also work with this change.
   b) On the other, it means that whether putting @OrderWith on a class will actually apply the Ordering depends on whether the class is a subclass of ParentRunner; at the very least, we should probably add documentation explicitly to BlockJUnit4Runner, JUnit3ClassRunner, and Suite to say that they "observe @OrderWith", with a link explaining what that means, and add documentation to OrderWith saying that you can only depend on its documented behavior if it is attached to a class that promises to "observe "@OrderWith".

3. One thing I need to let simmer for a while is the effect of this change on people just learning xTest-style testing. JUnit has always been opinionated about what it makes easy and what it makes possible.

Put another way, I'd hate to give credence to the narrative "JUnit used to irrationally make it hard to order your tests so that they worked, but with 4.13, they added OrderWith, so now you can go ahead and write order-dependent tests, no more problems."

(There's no action item on #3, just a warning that I may need to let this process a day or two before I'm ready to pull the trigger.)

[kcooney]

For the third point, although I agree that writing tests that depend on a specific ordering is a bad idea, we've been asked about this a number of times (as well as supporting shuffling tests) that we should consider some way to allow this, without baking into JUnit the particular strategy.

If you want to explore the RunRule route, could you create a pull request with an outline of what you had in mind?

When I have some time, I'll explore splitting Ordering and Sorter so you can see how that changes things.

I wonder if we could attach @OrderWith at the RunnerBuilder level.

One other concern: the way @OrderWith is implemented bothers me a bit, because the ordering is done the first time someone asks for the children, and that can happen as a result of a number of different actions. In particular, if a suite that uses @OrderWith includes a test class annotated with @OrderWith, it's unclear which will be applied first. I don't know if there's another hook that can be used to do ordering at a deterministic time. Perhaps doing the work in RunnerBuilder would solve that

[kcooney]

@dsaff I added two commits.

The first one moves the handling of @RunWith to RunnerBuilder.

The second changes Sorter to no longer extend Ordering. I added this so you get an idea of the complexities that I had before I decided that Sorter should extend Ordering. Note that with these changes, you can't say @OrderWith(MySort.class)

[dsaff]

(I think you meant "moves the handling of @OrderWith to RunnerBuilder").

Now that I see it, I think that RunnerBuilder is one step too far up to move this: now it's possible to write a suite that doesn't respect @RunWith on its children (by replacing AllDefaultPossibilitiesBuilder), but not possible to not respect @OrderWith. I'm still not sure about making @OrderWith be on the same level of the call tree as @RunWith, but I'm pretty sure it doesn't make sense to put it above. What do you think?

Re: Sorter doesn't extend Ordering: Note that now we have Sorter with a void apply(Object) method, and Ordering with a void apply(Object) method. One could create a [choosing random word] FooBox interface that has just this apply method, and cause OrderWith to accept any subclass of FooBox, thereby (I think) avoiding the need to wrap in many cases.

However, at that point, we're awfully close (again) to RunRule, since FooBox basically can do anything it wants to the Runner it's passed, not just ordering: it could certainly filter, and we might even be able to finagle it to inject listeners or rules. So we'd either have to decide to arbitrarily hamstring OrderWith, or accept that very clever people will use OrderWith to do non-ordering things, or give it a different name at that point.

I'm again tempted to go the fully generic route, since (a) we're almost there already, and (b) given the rather big (in percentage terms) addition here to top-level API, I'd feel much better about it if the features unlocked including more than ordering, which (as I've mentioned above) I'm worried may prove a mixed blessing.

Don't feel a need to go change the code just yet; I'd love to hear your thoughts on the above (unless changing the code is the easiest way to do that). Thanks again!

And I can work on a fork that shows what RunRule might look like, although today isn't necessarily looking conducive.

[kcooney]

@dsaff I actually think that RunnerBuilder is the right way to handle @OrderWith. A runner indicates that it can be sorted via Sortable. @OrderWith is a simple way to order any runner that is Sortable or Orderable without going trough Request.

I don't see any problems with third-party subclasses of AllDefaultPossibilitiesBuilder not being able to turn off the @OrderWith behavior. Could you give an example of a problem? And is that problem worse than the complexity of the Javadoc we would need on @OrderWith to describe when it would be ignored?

I'm sorry, but I still don't quite follow how your RunRule proposal would work. Can you whip up a quick implementation and point us to a branch on your github repo that has the changes? It's much easier to talk about actual code.

[kcooney]

This was some of the ugly code that caused me to want to make Sorter an Ordering.

[dsaff]

We currently allow Suites to ignore RunWith on their children, for example:

http://www.patterntesting.com/release/apidocs/src-html/patterntesting/runtime/junit/internal/SmokeBuilder.html

This facility may be under-documented, but it already exists, and is used.

As one extension that I think we'd want to allow, imagine a BackAndForthSuite runner that first runs its children in their native order, and then runs them again in the precise opposite direction, in order to rout out any order-dependent tests. I'll need to play with the code to see whether such a thing would be possible with the current implementation.

[kcooney]

@dsaff if you wanted to disable @OrderWith you could do something similar. You would wrap the runner with another runner that didn't implement Sortable or Orderable

[kcooney]

The current implementatiion makes sure the reordered children includes all original children exactly once. I could allow running tests multiple times, but I am not sure that we should.

[kcooney]

@marcphilipp had another ping on #1194 ... Should we get this pull submitted, find another solution, or close that issue?

[marcphilipp]

@kcooney Thanks for the ping! I think this PR looks promising, but there are still few things to be sorted out.

Currently Orderable extends Sortable. This forces every runner that wants to implement Orderable to implement Sortable as well. Did you do it this way so Sorter could extend Ordering?

Why do we need GeneralOrdering instead of? I locally replaced it with Ordering and deleted the class and all the tests were green.

[marcphilipp]

There's a typo in the class name. How about ComparatorBasedOrdering?

[marcphilipp]

There's no factoryClass parameter.

[marcphilipp]

Since this method is protected, I'm wondering whether we shouldn't just throw an UnsupportedOperationException. Thoughts?

[kcooney]

Since it's trivial to implement, and Ordering defines the method, I think we should implement it. A future subclass or Sorter could decide to call this method.

[marcphilipp]

Can you please add a comment to document what this method is used for?

[kcooney]

Currently Orderable extends Sortable. This forces every runner that wants to implement
Orderable to implement Sortable as well. Did you do it this way so Sorter could extend Ordering?

Yes, that's why it was done. I'm not too worked about forcing Sortable runners to implement Orderable for the following resons

1. If you implement Orderable, you need to be able to arbitrarily order children. If you could do that, you can sort the children. In fact, an Orderable runner could implement Sortable by using the Sorter to sort the Description objects and then use their Orderable implementation to reorder the children.
2. Many runners don't implement Sortable directly; they implement it by extending ParentRunner. Those runners will automatically implement Orderable with this change

Why do we need GeneralOrdering

If you need to do a sort, calling Sortable.sort(sorter) is much more efficient than calling Orderable.order(sorter). GeneralOrdering exists so that developers call the more-efficient method.

[kcooney]

Oh, and Sorter extends Ordering so OrderWith can work with both. I didn't want two annotations; one is simpler and it doesn't make sense to both order and sort your tests.

[kcooney]

Why do we need GeneralOrdering

I renamed GeneralOrdering to Orderer and it no longer implements Ordering. Now Orderer has the logic for doing the ordering, calling the Ordering to order the items and then validating that there are no duplicates or removed items. I think it's much simpler than what I had before, though I'm not excited about the naming.

[kcooney]

@marcphilipp do my changes and comments address your concerns?

[marcphilipp]

Yep, I think that looks good now, thanks! 👍