-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Changes from 2 commits
44b64df
e68c015
4770fff
a19a3ab
82e019f
f19f035
8390bc7
6e38776
1ba37d2
62ca6a0
753842d
ea71fa4
121744f
9d71b2f
5a7186b
2f9d21a
c6de86d
5a3f954
f2f6131
4ce52c1
d8a1ee6
bfbad94
9fb4772
78ee8c6
9d1e2aa
550654a
b2ce86a
ca3e040
3e6d464
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package org.junit.runner; | ||
|
||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Inherited; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
import org.junit.runner.manipulation.Ordering; | ||
|
||
/** | ||
* When a test class is annotated with <code>@OrderWith</code> or extends a class annotated | ||
* with <code>@OrderWith</code>, JUnit will order the tests in the test class (and child | ||
* test classes, if any) using the ordering defined by the {@link Ordering} class. | ||
* | ||
* @since 4.13 | ||
*/ | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.TYPE) | ||
@Inherited | ||
public @interface OrderWith { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple musings here (which I may not finish all at once)
It might be so much easier to instead just say: @OrderWith public static Ordering ordering = new FastestOrdering("/tmp/test-times.txt"); or some such. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsaff I don't see how what you are proposing would be 100% impossible. You could write your own ordering that reads the file the first time I'd hate to add more broiler-plate because of a possibly rare use case. Although I've personally never been bothered by adding a method with an annotation that returns something, other people have objected loudly to this (for example, with Rules). The problem with passing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So my apply() method would cast the Object to a Runner, call getDescription, and then look for the custom annotation in the Description? On balance, I'm not a fan of asking end-extenders to cast things, but could see doing it if there are benefits gained. I can't personally call this use case "possibly rare", since it's occurred in the majority of test-case sorting scenarios I've written (and since test re-ordering was an essential part of my thesis, I may be in the sorry position of having personally seen a healthy percentage of all test-ordering schemes our species have attempted...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsaff would you be fine with allowing a static method annotated with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsaff alternatively, as I suggested, we could add the parent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the long delay, but I hope that we can release JUnit 4.13 in 2017 and I'd like to include this pull. I updated the code to pass a context object that allows you to access the top-level description |
||
/** | ||
* @return a class that extends {@link Ordering} (must have a public no-argument constructor) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Sorry, deleted and re-instated the comment on a different line once I realized what a nightmare having two discussions on the same line of code would be. Alternatively, we could pop the whole conversation out to the general comment thread]
For example, there's not currently a FilterWith annotation, but once we accept this change, that seems to be a natural next square in the matrix to fill in, which would be another top-level change. Also related would be the common request to attach a custom runlistener to a class, ListenWith Just thinking out loud, what if we made a top-level annotation (RunRule? Modify?) which would handle any of these changes? Essentially: public interface RunRule { and then: public class MyTest { Note that: a) This builds on the static field suggestion in (1), but could be orthogonal to it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we would want end users to have to understand that filtering and sorting are applied via this new rule mechanism. They are orthogonal, and I think we should provide the simplest possible API to enable them. I don't see why one couldn't implement What you are proposing sounds like a larger redesign of runners, and one we should consider as part of the runner refactoring being proposed by |
||
*/ | ||
Class<? extends Ordering> value(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package org.junit.runner.manipulation; | ||
|
||
import java.util.Collection; | ||
import java.util.List; | ||
|
||
import org.junit.runner.Description; | ||
|
||
/** | ||
* An {@link Ordering} that is not a {@link Sorter}. | ||
* | ||
* @since 4.13 | ||
*/ | ||
public final class GenericOrdering extends Ordering { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsaff I would rather not call it |
||
private final Ordering delegate; | ||
|
||
GenericOrdering(Ordering delegate) { | ||
this.delegate = delegate; | ||
} | ||
|
||
@Override | ||
public List<Description> order(Collection<Description> siblings) { | ||
return delegate.order(siblings); | ||
} | ||
|
||
@Override | ||
public void apply(Object runner) throws InvalidOrderingException { | ||
/* | ||
* We overwrite apply() to avoid having a GenericOrdering wrap another | ||
* GenericOrdering. | ||
*/ | ||
if (runner instanceof Orderable) { | ||
Orderable orderable = (Orderable) runner; | ||
orderable.order(this); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package org.junit.runner.manipulation; | ||
|
||
/** | ||
* Thrown when an ordering does something invalid (like remove or add children) | ||
* | ||
* @since 4.13 | ||
*/ | ||
public class InvalidOrderingException extends Exception { | ||
private static final long serialVersionUID = 1L; | ||
|
||
public InvalidOrderingException() { | ||
} | ||
|
||
public InvalidOrderingException(String message) { | ||
super(message); | ||
} | ||
|
||
public InvalidOrderingException(String message, Throwable cause) { | ||
super(message, cause); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package org.junit.runner.manipulation; | ||
|
||
/** | ||
* Interface for runners that allow ordering of tests. | ||
* | ||
* <p>Beware of using this interface to cope with order dependencies between tests. | ||
* Tests that are isolated from each other are less expensive to maintain and | ||
* can be run individually. | ||
* | ||
* @since 4.13 | ||
*/ | ||
public interface Orderable extends Sortable { | ||
|
||
/** | ||
* Orders the tests using <code>ordering</code> | ||
* | ||
* @throws InvalidOrderingException if ordering does something invalid (like remove or add children) | ||
*/ | ||
void order(GenericOrdering ordering) throws InvalidOrderingException; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
package org.junit.runner.manipulation; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Random; | ||
|
||
import org.junit.runner.Description; | ||
|
||
/** | ||
* Reorders tests. An {@code Ordering} can reverse the order of tests, sort the | ||
* order or even shuffle the order. | ||
* | ||
* <p>In general you will not need to use a <code>Ordering</code> directly. | ||
* Instead, use {@link org.junit.runner.Request#orderWith(Ordering)}. | ||
* | ||
* @since 4.13 | ||
*/ | ||
public abstract class Ordering { | ||
|
||
/** | ||
* Creates an {@link Ordering} that shuffles the items using the given | ||
* {@link Random} instance. | ||
*/ | ||
public static Ordering shuffledBy(final Random random) { | ||
return new Ordering() { | ||
@Override | ||
public List<Description> order(Collection<Description> siblings) { | ||
List<Description> shuffled = new ArrayList<Description>(siblings); | ||
Collections.shuffle(shuffled, random); | ||
return shuffled; | ||
} | ||
}; | ||
} | ||
|
||
/** | ||
* Creates an {@link Ordering} from the given class. The class must have a public no-argument constructor. | ||
* | ||
* @throws InvalidOrderingException if the instance could not be created | ||
*/ | ||
public static Ordering definedBy(Class<? extends Ordering> orderingClass) | ||
throws InvalidOrderingException { | ||
try { | ||
return orderingClass.newInstance(); | ||
} catch (InstantiationException e) { | ||
throw new InvalidOrderingException("Could not create ordering", e); | ||
} catch (IllegalAccessException e) { | ||
throw new InvalidOrderingException("Could not create ordering", e); | ||
} | ||
} | ||
|
||
/** | ||
* Order the tests in <code>runner</code> using this ordering. | ||
* | ||
* @throws InvalidOrderingException if ordering does something invalid (like remove or add children) | ||
*/ | ||
public void apply(Object runner) throws InvalidOrderingException { | ||
/* | ||
* If the runner is Sortable but not Orderable and this Ordering is a | ||
* Sorter, then the Sorter subclass overrides apply() to apply the sort. | ||
* | ||
* Note that GenericOrdering also overrides apply() to avoid having a | ||
* GenericOrdering wrap another GenericOrdering. | ||
*/ | ||
if (runner instanceof Orderable) { | ||
Orderable orderable = (Orderable) runner; | ||
orderable.order(new GenericOrdering(this)); | ||
} | ||
} | ||
|
||
/** | ||
* Orders the given descriptions (all of which have the same parent). | ||
* | ||
* @param siblings unmodifiable collection of descriptions to order | ||
*/ | ||
public abstract List<Description> order(Collection<Description> siblings); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this take regular Ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
Sorter
extendsOrdering
, but we want to make sure that anyone attempting to do a sort of a runner goes throughOrdering.apply(Object)
. Originally I didn't haveSorter
extendOrdering
, and there the code was more complicated and had more duplication.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the duplication? I can see that one would want to be able to do both @OrderWith(MyOrdering.class) and @OrderWith(MySorter.class), and that's difficult unless one of the classes extends the other. But it looks as though Ordering is essentially a marker interface at this point: most of the code that actually does anything is assuming that all Orderings are either Sorters or GenericOrderings. If you like the RunRule suggestion below, this becomes more of an asset than a liability, but if not, maybe there's some further shuffling that could happen here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not editing earlier comment in case you're currently responding, but "Why the duplication?" is too strong. More to the point: "I see the case of the target type of OrderWith. Are there other places?")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should chat over VC, but I'll try to answer
If
Sorter
doesn't extendOrdering
then we have to write extra code to allow creation of anOrdering
from aSorter
orComparator
(sinceOrdering
is a very generic concept, it seems reasonable that one could have anOrdering
that performs a sort; to do otherwise would require people to choose an API based on the current implementation).Also, as I said earlier, if we allow
order()
to take in anOrdering
and that ordering does a sort, then we'll get worse performance than we would if the caller was smart enough to "know" that they should instead callsort()
(really they shouldn't call either, and instead apply theOrdering
orSorter
to theRunner
).Note that I also made
Orderable
extendSortable
. We could change that if we think it's an extra burden to people that writeOrderable
runners to have to also make themSortable
. If you want to get a hint as to the changes I would need to make ifOrderable
did not extendSortable
, look at the second commit in this pull (the "unreachable code" would be reachable).