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

Extending and Modifying sorting feature of JUnit4. #882

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/main/java/junit/framework/JUnit4TestAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public void filter(Filter filter) throws NoTestsRemainException {
}

public void sort(Sorter sorter) {
sorter.apply(fRunner);
if(sorter!=null)
Copy link
Member

Choose a reason for hiding this comment

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

nit: we put spaces around the "if" keyword, and in new code we try to always use braces for if statements.

This same issue is in a few places

Note that code under org.junit will be migrated to the Google Java Style Guide; see http://google-styleguide.googlecode.com/svn/trunk/javaguide.html

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for not being aware of coding style. Will clean up my code.

Copy link
Member

Choose a reason for hiding this comment

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

why this change? I would rather use some kind of no-op sorter rather than have a special case for null

sorter.apply(fRunner);
}
}
67 changes: 67 additions & 0 deletions src/main/java/org/junit/SortWith.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package org.junit;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.util.Comparator;

import org.junit.runners.Sorters;

/**
* The <code>SortWith</code> Annotation that tells JUnit to execute test methods
* or test classes defined in a test suite in a order specified by users.
* It also supports randomness of test by designating a random sorting method
* to either a test class or a test suite.
*
* <p>
* A sortable test class with its test methods executed in name ascending order looks like:
* <pre>
*
* <b>&#064;RunWith(JUnit4.class)</b>
* <b>&#064;SortWith(Sorters.NAME_ASCENDING)</b>
* public class Example {
* <b>&#064;Test</b>
* public void testMethod1() {
*
* }
*
* <b>&#064;Test</b>
* public void testMethod2() {
*
* }
* }
* </pre>
* <p>
*
* <p>
* A sortable test suite with its test classes executed in name ascending order looks like:
* <pre>
*
* <b>&#064;RunWith(Suite.class)</b>
* <b>&#064;Suite.SuiteClasses({ ClassA.class, ClassB.class })</b>
* <b>&#064;SortWith(Sorters.NAME_ASCENDING)</b>
* public class Example {
*
* }
* </pre>
* <p>
*
* The <code>SortWith</code> Annotation takes one input parameter, see {@link org.junit.runner.manipulation.Sorter}.
* The parameter declares which sorting method JUnit should adopt when
* starts running tests. If no sorting method is specified for the
* Annotation, then by default the default sorting method which compares
* hash codes of names of the two methods or names of two test classes is
* used.
*
* Be aware that using {@link org.junit.runner.Request#sortWith(Comparator)} to manually specify a sorting
* method for JUnit overrides all the declarative sorting methods specified through <code>SortWith</code> Annotation.
*
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface SortWith {

Sorters value() default Sorters.DEFAULT;
Copy link
Member

Choose a reason for hiding this comment

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

Could we take in a class as a value instead? That way, users could add their own sorting implementation instead of having JUnit define all the ways you could sort.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! a good idea. Will change accordingly.


}
5 changes: 5 additions & 0 deletions src/main/java/org/junit/internal/requests/ClassRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import org.junit.internal.builders.AllDefaultPossibilitiesBuilder;
import org.junit.runner.Request;
import org.junit.runner.Runner;
import org.junit.runner.manipulation.Sortable;
import org.junit.runner.manipulation.Sorter;
import org.junit.runners.ParentRunner;

public class ClassRequest extends Request {
private final Object fRunnerLock = new Object();
Expand All @@ -25,6 +28,8 @@ public Runner getRunner() {
synchronized (fRunnerLock) {
if (fRunner == null) {
fRunner = new AllDefaultPossibilitiesBuilder(fCanUseSuiteMethod).safeRunnerForClass(fTestClass);
if(fRunner instanceof Sortable)
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised this change is necessary. We already have support in JUnit for Sortable; why do we need ClassRequest to know about it?

Copy link
Author

Choose a reason for hiding this comment

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

The original Sortable feature only supports method level sorting. New sorting feature supported by @SortWith is able to handle both class level and method level sorting.
The reason why I think it is necessary to let ClassRequest handle sorting is because the class request is executed before sorting request hence if users manually define a sorting method via sorting request, it can override sorting methods defined by @SortWith.

Copy link
Member

Choose a reason for hiding this comment

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

@fshepherd the core JUnit code and runners currently support Sortable for method and class level sorting. That's what the code in ParentRunner does. The ParentRunner class is the base class for class-level runners and method-level runners.

Copy link
Author

Choose a reason for hiding this comment

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

@kcooney thanks for your remind. I have already noticed that. Users can specify sorting method through sorting request. However, that applies to both class-level runner and method level runner. What I extended is that through @SortWith Annotation, user is able to define different sorting strategies for class-level runner and method level runner. For example, a test suite could execute its test case classes in name ascending way while every test case included in that test suite could sort its test methods using another sorting strategy , such as JVM.

Copy link
Member

Choose a reason for hiding this comment

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

@fshepherd my point is that if there are changes to ParentRunner, they should be extremely small, because ParentRunner already supports method and class sorting. Ditto for JUnit4TestAdapter

((Sortable)fRunner).sort(Sorter.ANNOTATED_SORTER);
}
}
}
Expand Down
71 changes: 69 additions & 2 deletions src/main/java/org/junit/runner/manipulation/Sorter.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@
import org.junit.runner.Description;

/**
* A <code>Sorter</code> orders tests. In general you will not need
* to use a <code>Sorter</code> directly. Instead, use {@link org.junit.runner.Request#sortWith(Comparator)}.
* A <code>Sorter</code> orders tests. In general you should specify the Sorter you
* want using <code>SortWith</code> Annotation.
* To use a <code>Sorter</code> directly, use {@link org.junit.runner.Request#sortWith(Comparator)}.
* Be aware, as long as a Sorter is manually specified, all declarative sorting methods defined by
* <code>SortWith</code> Annotation will be overridden.
*
* @since 4.0
*/
Expand All @@ -19,6 +22,59 @@ public int compare(Description o1, Description o2) {
return 0;
}
});

public static final Sorter ANNOTATED_SORTER=null;

/**
* DEFAULT sorting method. It compares hash codes of either names of two test methods or names of two test classes,
* depending on whether it is used for sorting execution order for test methods or sorting execution order for
* test classes defined in a test suite.
*/
public static final Sorter DEFAULT=new Sorter(new Comparator<Description>(){

public int compare(Description o1, Description o2) {
String[] items = getComparisonItems(o1,o2);
int hash1 = items[0].hashCode();
int hash2 = items[1].hashCode();
if (hash1 != hash2) {
return hash1 < hash2 ? -1 : 1;
}
return NAME_ASCENDING.compare(o1, o2);
}

});

/**
* Sorting method based on name ascending order. It compares names of two test methods or names of two test classes,
* depending on whether it is used for sorting execution order for test methods or sorting execution order for
* test classes defined in a test suite.
*/
public static final Sorter NAME_ASCENDING = new Sorter(new Comparator<Description>(){

public int compare(Description o1, Description o2) {
String[] items = getComparisonItems(o1,o2);
final int comparison = items[0].compareTo(items[1]);
if (comparison != 0) {
return comparison;
}
return items[0].toString().compareTo(items[1].toString());
}

});

/**
* Random sorting method. It generates a random double value between 0 and 1 during runtime such that
* a set of test methods or a set of test classes will always run in a random order.
*/
public static final Sorter RANDOM = new Sorter(new Comparator<Description>(){
Copy link
Member

Choose a reason for hiding this comment

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

I very strongly feel that JUnit shouldn't ship with a random sorting implementation. Instead we should allow users to provide their own implementations.

Randomly sorting tests seems like a good idea, but it often isn't

  • if a particular ordering causes the tests to fail, running again will fix them. so the failures are not reproducible
  • if you are making a change to your code, and then run the tests, and you happen to hit an ordering that causes your test to fail, then most likely you'll assume your code changes caused the failure, leading you in the wrong direction
  • related to the previous point, if someone checks in a change, and the next run of your build happens to hit an order that causes a failure, you'll think incorrectly that the change broke the build
  • randomly ordering tests isn't a sort. it's really hard to get a random ordering in a way that won't cause an infinite loop, or won't be slow, or won't cause the ordering to change in the middle of the run

There are ways around some of these

  • you could print out the random seed used, and have a way to specify the seed. but there are a dozen ways one could do each of these, and I'm not sure JUnit should pick which one is best
  • you could have the random seed based on the time of day (so, perhaps it changes daily). but I'm not sure JUnit should decide when the seed should change (or, again, how to specify a seed so you can reproduce)

I'd be more than happy to allow users to specify their own sorting class, and for the documentation to provide a suggested implementation that does random sorting (as long as it gives the above caveats)


public int compare(Description o1, Description o2) {
return Math.random() - 0.5 >0 ? 1 : -1;
Copy link
Member

Choose a reason for hiding this comment

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

This violates the constract of Comparator; see http://docs.oracle.com/javase/7/docs/api/java/util/Comparator.html

}

});

//public static final Sorter JVM
Copy link
Member

Choose a reason for hiding this comment

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

please remove commented out code


private final Comparator<Description> fComparator;

Expand All @@ -45,4 +101,15 @@ public void apply(Object object) {
public int compare(Description o1, Description o2) {
return fComparator.compare(o1, o2);
}

private final static String[] getComparisonItems(Description o1,Description o2){
if(o1.isSuite()){
final String[] items={o1.getClassName(),o2.getClassName()};
return items;
}else{
final String[] items={o1.getMethodName(),o2.getMethodName()};
return items;
}
}

}
33 changes: 28 additions & 5 deletions src/main/java/org/junit/runners/ParentRunner.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.junit.ClassRule;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.SortWith;
import org.junit.internal.AssumptionViolatedException;
import org.junit.internal.runners.model.EachTestNotifier;
import org.junit.internal.runners.statements.RunAfters;
Expand Down Expand Up @@ -384,13 +385,25 @@ public void filter(Filter filter) throws NoTestsRemainException {
}

public void sort(Sorter sorter) {
synchronized (fChildrenLock) {
for (T each : getFilteredChildren()) {
sorter.apply(each);
}
if(sorter==Sorter.ANNOTATED_SORTER){
Copy link
Member

Choose a reason for hiding this comment

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

this is a lot more complicated than the original implementation, so it's hard for me to tell if there a functional changes.

why do we need to special case ANNOTATED_SORTER? couldn't the difference between ANNOTATED_SORTER and the other sorters be put in the implementation of ANNOTATED_SORTER?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also worried that if you need to change the implementation of sort in ParentRunner to get this code to work, do other runners that implement Sortable need to change?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry a bad idea...will rethink the way to organize this piece of code.

List<T> sortedChildren = new ArrayList<T>(getFilteredChildren());
Collections.sort(sortedChildren, comparator(sorter));
Sorter annotatedSorter=getSorter();
Copy link
Member

Choose a reason for hiding this comment

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

nit: we put spaces on both sides of all operators except for comma operator. so there are spaces around '=' and '!='

if(annotatedSorter!=null)
Collections.sort(sortedChildren, comparator(annotatedSorter));
fFilteredChildren = Collections.unmodifiableCollection(sortedChildren);
for (T each : getFilteredChildren()) {
if(each instanceof Sortable)
((Sortable)each).sort(null);
}
}else{
synchronized (fChildrenLock) {
for (T each : getFilteredChildren()) {
sorter.apply(each);
}
List<T> sortedChildren = new ArrayList<T>(getFilteredChildren());
Collections.sort(sortedChildren, comparator(sorter));
fFilteredChildren = Collections.unmodifiableCollection(sortedChildren);
}
}
}

Expand Down Expand Up @@ -436,4 +449,14 @@ public int compare(T o1, T o2) {
public void setScheduler(RunnerScheduler scheduler) {
this.fScheduler = scheduler;
}

private Sorter getSorter(){
final TestClass clazz = getTestClass();
if(clazz==null)
return null;
SortWith sortMethod=clazz.getJavaClass().getAnnotation(SortWith.class);
if(sortMethod==null)
return null;
return sortMethod.value().getSorter();
}
}
44 changes: 44 additions & 0 deletions src/main/java/org/junit/runners/Sorters.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.junit.runners;
Copy link
Member

Choose a reason for hiding this comment

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

any reason why this class isn't in org.junit.runner.manipulation?


import org.junit.runner.manipulation.Sorter;


/**
* Sorters enumerator used for defining sorting
* method for {@link SortWith}
*
*/
public enum Sorters {

/**
* default sorting method, using hash code
* of a method name o a class name to compare
*/
DEFAULT(Sorter.DEFAULT),

/**
* random sorting method.
*/
RANDOM(Sorter.RANDOM),

/**
* sorting method based on name ascending order
*/
NAME_ASCENDING(Sorter.NAME_ASCENDING),

/**
* JVM sorting method
*/
JVM(null);

private final Sorter sorter;

private Sorters(Sorter sorter){
this.sorter=sorter;
}

public Sorter getSorter(){
return sorter;
}

}
2 changes: 1 addition & 1 deletion src/main/java/org/junit/runners/model/TestClass.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public TestClass(Class<?> klass) {

protected void scanAnnotatedMembers(Map<Class<? extends Annotation>, List<FrameworkMethod>> methodsForAnnotations, Map<Class<? extends Annotation>, List<FrameworkField>> fieldsForAnnotations) {
for (Class<?> eachClass : getSuperClasses(fClass)) {
for (Method eachMethod : MethodSorter.getDeclaredMethods(eachClass)) {
for (Method eachMethod : eachClass.getDeclaredMethods()) {
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Author

Choose a reason for hiding this comment

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

Because it overlaps the method level sorting function supported by @SortWith. And I found that it sorts all the methods no matter it is testable or not (annotated with @test or @ignore) defined in a test class, which is a bit unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

We never want to call Class.getDeclaredMethods() directly. The results are not deterministic.

addToAnnotationLists(new FrameworkMethod(eachMethod), methodsForAnnotations);
}
// ensuring fields are sorted to make sure that entries are inserted
Expand Down
Loading