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
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
44b64df
Add Ordering, Orderable and @OrderWith.
kcooney Mar 28, 2015
e68c015
Remove unreachable code
kcooney Apr 25, 2015
4770fff
Handle @RunWith in RunnerBuilder
kcooney Apr 30, 2015
a19a3ab
Change Sorter to no longer extend Ordering.
kcooney Apr 30, 2015
82e019f
Revert "Change Sorter to no longer extend Ordering."
kcooney May 1, 2015
f19f035
Replace Ordering.order(List) with Ordering.orderDescription(Description)
kcooney May 1, 2015
8390bc7
Rename GenericOrdering to GeneralOrdering
kcooney Jan 7, 2017
6e38776
Merge branch 'master' into ordering
kcooney Jan 7, 2017
1ba37d2
Add Ordering.Context so Orderings can use the Description to get
kcooney Jan 7, 2017
62ca6a0
Rename parameters in applyOrdering() and Sorter.apply() from "runner"…
kcooney Jan 7, 2017
753842d
Check ordering correctness in Ordering.
kcooney Jan 7, 2017
ea71fa4
Pass Ordering.Context in constructor when reflectively creating insta…
kcooney Jan 7, 2017
121744f
Remove unnecessary call to unmodifableCollection()
kcooney Jan 7, 2017
9d71b2f
Remove use of ReflectiveOperationException
kcooney Jan 8, 2017
5a7186b
Merge branch 'master' into ordering
kcooney May 15, 2017
2f9d21a
Fix javadoc for orderWith()
kcooney May 15, 2017
c6de86d
Minor formatting fixes
kcooney May 15, 2017
5a3f954
Add Ordering.Factory.
kcooney May 18, 2017
f2f6131
Merge branch 'master' into ordering
kcooney May 18, 2017
4ce52c1
Add missing @since Javadoc for Ordering methods
kcooney May 18, 2017
d8a1ee6
Merge branch 'master' into ordering
kcooney May 26, 2017
bfbad94
Minor formatting fix.
kcooney Jun 27, 2017
9fb4772
Merge branch 'master' into ordering
kcooney Aug 7, 2017
78ee8c6
Rename ComparsionBasedOrdering to ComparatorBasedOrdering; fix Javadoc
kcooney Jun 1, 2018
9d1e2aa
Rename GeneralOrdering to Orderer and make it no longer implement Ord…
kcooney Jun 1, 2018
550654a
Add OrderingRequest, to ensure orderWith() orders once
kcooney Jun 2, 2018
b2ce86a
Introduce MemoizingRequest
kcooney Jun 2, 2018
ca3e040
Sort tests in AllManipulationTests
kcooney Jun 2, 2018
3e6d464
Removed Comparators.reverse(); it's broken and the JDK provides a bet…
kcooney Jun 2, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/main/java/junit/framework/JUnit4TestAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
import org.junit.runner.Runner;
import org.junit.runner.manipulation.Filter;
import org.junit.runner.manipulation.Filterable;
import org.junit.runner.manipulation.Orderer;
import org.junit.runner.manipulation.InvalidOrderingException;
import org.junit.runner.manipulation.NoTestsRemainException;
import org.junit.runner.manipulation.Sortable;
import org.junit.runner.manipulation.Orderable;
import org.junit.runner.manipulation.Sorter;

/**
Expand All @@ -23,7 +25,7 @@ public static Test suite() {
}
</pre>
*/
public class JUnit4TestAdapter implements Test, Filterable, Sortable, Describable {
public class JUnit4TestAdapter implements Test, Filterable, Orderable, Describable {
private final Class<?> fNewTestClass;

private final Runner fRunner;
Expand Down Expand Up @@ -93,4 +95,13 @@ public void filter(Filter filter) throws NoTestsRemainException {
public void sort(Sorter sorter) {
sorter.apply(fRunner);
}

/**
* {@inheritDoc}
*
* @since 4.13
*/
public void order(Orderer orderer) throws InvalidOrderingException {
orderer.apply(fRunner);
}
}
23 changes: 3 additions & 20 deletions src/main/java/org/junit/internal/requests/ClassRequest.java
Original file line number Diff line number Diff line change
@@ -1,25 +1,18 @@
package org.junit.internal.requests;

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import org.junit.internal.builders.AllDefaultPossibilitiesBuilder;
import org.junit.internal.builders.SuiteMethodBuilder;
import org.junit.runner.Request;
import org.junit.runner.Runner;
import org.junit.runners.model.RunnerBuilder;

public class ClassRequest extends Request {
private final Lock runnerLock = new ReentrantLock();

public class ClassRequest extends MemoizingRequest {
/*
* We have to use the f prefix, because IntelliJ's JUnit4IdeaTestRunner uses
* reflection to access this field. See
* https://github.com/junit-team/junit4/issues/960
*/
private final Class<?> fTestClass;
private final boolean canUseSuiteMethod;
private volatile Runner runner;

public ClassRequest(Class<?> testClass, boolean canUseSuiteMethod) {
this.fTestClass = testClass;
Expand All @@ -31,18 +24,8 @@ public ClassRequest(Class<?> testClass) {
}

@Override
public Runner getRunner() {
if (runner == null) {
runnerLock.lock();
try {
if (runner == null) {
runner = new CustomAllDefaultPossibilitiesBuilder().safeRunnerForClass(fTestClass);
}
} finally {
runnerLock.unlock();
}
}
return runner;
protected Runner createRunner() {
return new CustomAllDefaultPossibilitiesBuilder().safeRunnerForClass(fTestClass);
}

private class CustomAllDefaultPossibilitiesBuilder extends AllDefaultPossibilitiesBuilder {
Expand Down
30 changes: 30 additions & 0 deletions src/main/java/org/junit/internal/requests/MemoizingRequest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.junit.internal.requests;

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import org.junit.runner.Request;
import org.junit.runner.Runner;

abstract class MemoizingRequest extends Request {
private final Lock runnerLock = new ReentrantLock();
private volatile Runner runner;

@Override
public final Runner getRunner() {
if (runner == null) {
runnerLock.lock();
try {
if (runner == null) {
runner = createRunner();
}
} finally {
runnerLock.unlock();
}
}
return runner;
}

/** Creates the {@link Runner} to return from {@link #getRunner()}. Called at most once. */
protected abstract Runner createRunner();
}
29 changes: 29 additions & 0 deletions src/main/java/org/junit/internal/requests/OrderingRequest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.junit.internal.requests;

import org.junit.internal.runners.ErrorReportingRunner;
import org.junit.runner.Request;
import org.junit.runner.Runner;
import org.junit.runner.manipulation.InvalidOrderingException;
import org.junit.runner.manipulation.Ordering;

/** @since 4.13 */
public class OrderingRequest extends MemoizingRequest {
private final Request request;
private final Ordering ordering;

public OrderingRequest(Request request, Ordering ordering) {
this.request = request;
this.ordering = ordering;
}

@Override
protected Runner createRunner() {
Runner runner = request.getRunner();
try {
ordering.apply(runner);
} catch (InvalidOrderingException e) {
return new ErrorReportingRunner(ordering.getClass(), e);
}
return runner;
}
}
22 changes: 19 additions & 3 deletions src/main/java/org/junit/internal/runners/JUnit38ClassRunner.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.junit.internal.runners;

import java.lang.annotation.Annotation;
import java.lang.reflect.Method;

import junit.extensions.TestDecorator;
import junit.framework.AssertionFailedError;
import junit.framework.Test;
Expand All @@ -12,15 +15,16 @@
import org.junit.runner.Runner;
import org.junit.runner.manipulation.Filter;
import org.junit.runner.manipulation.Filterable;
import org.junit.runner.manipulation.Orderer;
import org.junit.runner.manipulation.InvalidOrderingException;
import org.junit.runner.manipulation.NoTestsRemainException;
import org.junit.runner.manipulation.Orderable;
import org.junit.runner.manipulation.Sortable;
import org.junit.runner.manipulation.Sorter;
import org.junit.runner.notification.Failure;
import org.junit.runner.notification.RunNotifier;
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;

public class JUnit38ClassRunner extends Runner implements Filterable, Sortable {
public class JUnit38ClassRunner extends Runner implements Filterable, Orderable {
private static final class OldTestClassAdaptingListener implements
TestListener {
private final RunNotifier notifier;
Expand Down Expand Up @@ -170,6 +174,18 @@ public void sort(Sorter sorter) {
}
}

/**
* {@inheritDoc}
*
* @since 4.13
*/
public void order(Orderer orderer) throws InvalidOrderingException {
if (getTest() instanceof Orderable) {
Orderable adapter = (Orderable) getTest();
adapter.order(orderer);
}
}

private void setTest(Test test) {
this.test = test;
}
Expand Down
26 changes: 26 additions & 0 deletions src/main/java/org/junit/runner/OrderWith.java
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>&#064;OrderWith</code> or extends a class annotated
* with <code>&#064;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 {
Copy link
Member

Choose a reason for hiding this comment

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

A couple musings here (which I may not finish all at once)

  1. I'd be strongly tempted to make this an annotated static field instead/in-addition. Why? Well, consider the case (which I've written several times in the past) where you try to run the tests from the fastest to the slowest, based on a record of previous runtimes kept in a file. It probably makes sense to allow the location of the file to be specified by the test writer. I think that with the current pull request, this would be 100% impossible. One COULD imagine a different spec of this annotation where what's returned is not an Ordering, but a functional interface that takes a Description and returns an Ordering, so that one could write a Fastest Ordering that looks for a @TestTimesFile annotation on the Description to know what file to use, BUT...

It might be so much easier to instead just say:

@OrderWith public static Ordering ordering = new FastestOrdering("/tmp/test-times.txt");

or some such.

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 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 apply() is called. If the location of the file should be specified by the test writer, then your Ordering could use a custom annotation.

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 Description when you create an ordering is that an ordering applies to all children. Luckily, you can get a Description from a Runner so a custom Ordering do a different algorithm based on the Description inside of Ordering.apply(). I could be convinced that Ordering.order() should also take in the parent Description.

Copy link
Member

Choose a reason for hiding this comment

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

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 would you be fine with allowing a static method annotated with OrderWith as an additional way of specifying an ordering, and document whether the static method overrides the annotation?

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 alternatively, as I suggested, we could add the parent Description to the parameters passed to Ordering.order(), which should handle your use case.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
of the class annotated with @OrderWith. This will handle David's use-case of " run the tests from the fastest to the slowest, based on a record of previous runtimes kept in a file". The filename could
either be specified as an annotation on the class annotated with @OrderWith or it could be
derived from the class name of the class annotated with @OrderWith

/**
* Gets a class that extends {@link Ordering}. The class must have a public no-arg constructor.
*/
Class<? extends Ordering.Factory> value();
}
42 changes: 36 additions & 6 deletions src/main/java/org/junit/runner/Request.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
import org.junit.internal.builders.AllDefaultPossibilitiesBuilder;
import org.junit.internal.requests.ClassRequest;
import org.junit.internal.requests.FilterRequest;
import org.junit.internal.requests.OrderingRequest;
import org.junit.internal.requests.SortingRequest;
import org.junit.internal.runners.ErrorReportingRunner;
import org.junit.runner.manipulation.Filter;
import org.junit.runner.manipulation.Ordering;
import org.junit.runners.model.InitializationError;

/**
Expand Down Expand Up @@ -151,15 +153,15 @@ public Request filterWith(Description desiredDescription) {
* For example, here is code to run a test suite in alphabetical order:
* <pre>
* private static Comparator&lt;Description&gt; forward() {
* return new Comparator&lt;Description&gt;() {
* public int compare(Description o1, Description o2) {
* return o1.getDisplayName().compareTo(o2.getDisplayName());
* }
* };
* return new Comparator&lt;Description&gt;() {
* public int compare(Description o1, Description o2) {
* return o1.getDisplayName().compareTo(o2.getDisplayName());
* }
* };
* }
*
* public static main() {
* new JUnitCore().run(Request.aClass(AllTests.class).sortWith(forward()));
* new JUnitCore().run(Request.aClass(AllTests.class).sortWith(forward()));
* }
* </pre>
*
Expand All @@ -169,4 +171,32 @@ public Request filterWith(Description desiredDescription) {
public Request sortWith(Comparator<Description> comparator) {
return new SortingRequest(this, comparator);
}

/**
* Returns a Request whose Tests can be run in a certain order, defined by
* <code>ordering</code>
* <p>
* For example, here is code to run a test suite in reverse order:
* <pre>
* private static Ordering reverse() {
* return new Ordering() {
* public List&lt;Description&gt; orderItems(Collection&lt;Description&gt; descriptions) {
* List&lt;Description&gt; ordered = new ArrayList&lt;&gt;(descriptions);
* Collections.reverse(ordered);
* return ordered;
* }
* }
* }
*
* public static main() {
* new JUnitCore().run(Request.aClass(AllTests.class).orderWith(reverse()));
* }
* </pre>
*
* @return a Request with ordered Tests
* @since 4.13
*/
public Request orderWith(Ordering ordering) {
return new OrderingRequest(this, ordering);
}
}
27 changes: 27 additions & 0 deletions src/main/java/org/junit/runner/manipulation/Alphanumeric.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package org.junit.runner.manipulation;

import java.util.Comparator;

import org.junit.runner.Description;

/**
* A sorter that orders tests alphanumerically by test name.
*
* @since 4.13
*/
public final class Alphanumeric extends Sorter implements Ordering.Factory {

public Alphanumeric() {
super(COMPARATOR);
}

public Ordering create(Context context) {
return this;
}

private static final Comparator<Description> COMPARATOR = new Comparator<Description>() {
public int compare(Description o1, Description o2) {
return o1.getDisplayName().compareTo(o2.getDisplayName());
}
};
}
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);
}
}
21 changes: 21 additions & 0 deletions src/main/java/org/junit/runner/manipulation/Orderable.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
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>orderer</code>
*
* @throws InvalidOrderingException if orderer does something invalid (like remove or add
* children)
*/
void order(Orderer orderer) throws InvalidOrderingException;
}
Loading