Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ordering, Orderable and @OrderWith #1130

Merged
merged 29 commits into from
Jul 30, 2018
Merged

Conversation

kcooney
Copy link
Member

@kcooney kcooney commented Apr 25, 2015

These APIs allow arbitrary ordering of tests, including randomization.

These APIs allow arbitrary ordernig of tests, including randomization.
@dsaff
Copy link
Member

dsaff commented Apr 27, 2015

Woah, nice!

Beginning to dig in. Thanks for the work here.

*
* @since 4.13
*/
public final class GenericOrdering extends Ordering {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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
Copy link
Member

dsaff commented Apr 27, 2015

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
Copy link
Member

dsaff commented Apr 27, 2015

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

@kcooney
Copy link
Member Author

kcooney commented Apr 27, 2015

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
Copy link
Member Author

kcooney commented Apr 27, 2015

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
Copy link
Member

dsaff commented Apr 29, 2015

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
Copy link
Member Author

kcooney commented Apr 29, 2015

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
Copy link
Member Author

kcooney commented Apr 30, 2015

@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
Copy link
Member

dsaff commented Apr 30, 2015

(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
Copy link
Member Author

kcooney commented Apr 30, 2015

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


@Override
public void apply(Object runner) {
if (runner instanceof Sortable) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@dsaff
Copy link
Member

dsaff commented May 1, 2015

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
Copy link
Member Author

kcooney commented May 1, 2015

@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
Copy link
Member Author

kcooney commented May 1, 2015

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 kcooney modified the milestone: 4.13 Aug 6, 2017
@kcooney kcooney removed the 4.13 label Aug 6, 2017
@kcooney
Copy link
Member Author

kcooney commented May 17, 2018

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

@marcphilipp
Copy link
Member

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

/**
* An ordering that internally uses a {@link Comparator}.
*/
class ComparsionBasedOrdering extends Ordering {
Copy link
Member

Choose a reason for hiding this comment

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

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

/**
* Creates an {@link Ordering} from the given factory.
*
* @param factoryClass class to use to create the ordering
Copy link
Member

Choose a reason for hiding this comment

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

There's no factoryClass parameter.

* compare() method--but the Liskov substitution principle demands that
* we obey the general contract of Orderable. Luckily, it's trivial to
* implement.
*/
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
}

boolean validateOrderingIsCorrect() {
Copy link
Member

Choose a reason for hiding this comment

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

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

@kcooney
Copy link
Member Author

kcooney commented Jun 1, 2018

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
Copy link
Member Author

kcooney commented Jun 1, 2018

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
Copy link
Member Author

kcooney commented Jun 1, 2018

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
Copy link
Member Author

kcooney commented Jun 20, 2018

@marcphilipp do my changes and comments address your concerns?

Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

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

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

@kcooney kcooney merged commit 02c3280 into junit-team:master Jul 30, 2018
aristotle0x01 pushed a commit to aristotle0x01/junit4 that referenced this pull request Jun 27, 2022
* Add Ordering, Orderable and @OrderWith.

These APIs allow arbitrary ordering of tests, including randomization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants