SortWith annotation for TestClass with default implementations

[SpOOnman]

This pull request provides @SortWith annotation for TestClasses. It takes a Class as a value and uses it to sort methods within test class. It allows users to write their own implementations. Three built-in implementations are available: default, name ascending and JVM. @SortWith replaces @FixMethodOrder annotation.

I've decided to write a new pull request like @kcooney pointed out in #998. There is another pull request with @SortWith: #882, but it went too far and it is stuck with no ongoing work. I've read all comments and I hope this implementation would be accepted.

[SpOOnman]

Can you please evaluate this one?

[marcphilipp]

@kcooney Can you please take a look?

[kcooney]

I have been a bit busy lately at work, and the JUnit team has been focused on 4.12. Can this wait for a few weeks?

[SpOOnman]

@kcooney it's up to you. It's a @SortWith change you were considering since April. I don't know if you've planned it to be a part of next release or not.

[kcooney]

@SpOOnman we already have two release candidates for 4.12. At some point we need to stop adding features :-)

[kcooney]

While this is an internal class, it's possible that this could be used by external code. Do we need to rename this to MethodSorterUtil?

[kcooney]

Thanks for the contribution!

This is an interesting approach. It looks like it will nicely handle the case were a user wants a custom order for the tests in a class, and possibly if they want a sorter that will allow you to specify dependencies between tests (the JUnit team frowns on having dependent requests, but it's a common request, so having an extension point that would support it would be good).

I have a few questions/concerns for you. I suggest we address these first before we address the comments I left in the code review. Note that the other contributors might have different concerns, or may disagree with me on any of these concerns

Here are my concerns, in increasing level of importance:

1. If the tests are derived from the methods (like we do for parameterized tests) then the custom sorters will not be able to completely specify the ordering of the tests
2. This only supports ordering of methods. If some custom runner supports a different way to get the tests (an external file, some data structure, etc) they could not reuse any of this infrastructure
3. If a user wants a consistent ordering for a suite of tests, they have to annotate every test. One common request for specifying custom ordering is for randomizing tests, and many users wanting to randomize tests want to have a simple way to shuffle all tests, to catch dependencies between tests (of course, they probably also want a way to override that shuffling on a case by case basis).
4. It breaks @FixMethodOrder

The first two can be handled by sorting Description objects. The last one could be resolved by either a custom adapter or with a small amount of ugliness in the core JUnit code.

The more difficult concern is the third concern. I could imagine users wanting to run all of their tests in a random order, but one or two tests might specify a dependency between tests (say, when testing a Stack we would test push on an empty stack before push-than-pop on a non-empty stack). I don't have a good suggestion for how we do that (it probably involves org.junit.runner.manipulation.Sortable in some way).

It's possible that the solution for "randomize the order of all the tests" is different than the solution for "order the tests in this class". You might want some kind of way for a build tool to specify that the tests should be randomized; if so, we would likely go with a solution that uses ServiceLocator to determine if the end user wants a custom ordering for the entire run (see #923 for one place we are thinking of using ServiceLocator)

[SpOOnman]

I'm glad it looks fine. Thank you for detailed code review. Code can be fixed fast.

Your concerns are harder to resolve. Let's try to sort them out.

First, I want to explain that @FixMethodOrder was my starting point. I've searched for all usages of annotation and methods involved and slowly reworked them. There were no need to modify code in other files than @FixMethodOrder was used and I was very happy that @SortWitn worked without bigger refactoring.

ad. 1. You're probably right. But did that work fine with @FixMethodOrder before or was it overlooked?

ad. 2. That's true, we don't know if custom test runners would support @SortWith. I don't understand how 1) and 2) can be fixed with sorting Description orders.

ad. 3. Did you mean annotate every test suite? @SortWith comes with default implementation, just like @FixMethodOrder did. I want @SortWith to be simple and not allow it for more than one test suite.

ad. 4. Yes, it replaces @FixMethodOrder

I don't know internals very well. I don't know how to address ServiceLocator suggestion. If you want to apply custom sorting for entire run it goes beyond the scope of that change.

[kcooney]

Thanks for your detailed response. To answer your commetns

ad. 1. You're probably right. But did that work fine with @FixMethodOrder before or was it overlooked?

The @FixMethodOrder annotation is solving a different problem. The issue was that the ordering of methods from the JVM become non-deterministic with JDK 7, so we needed a way to make the ordering consistent.

Users wanting sorting of tests have had other needs. There are two common cases I am aware of 1) wanting to specify that a given test should be run after/before another test, and 2) wanting to shuffle test execution order to catch unintended dependencies between tests.

I don't understand how 1) and 2) can be fixed with sorting Description orders.

If we updated ParentRunner to sort the methods by sorting their Description objects, then that would solve both problems, because we would be sorting tests, not class methods.

1. Did you mean annotate every test suite?

I meant annotate every test class. A number of people on threads discussing sorting want to sort all (or most) tests. Requiring that every test class is annotated isn't a great solution for them.

1. Yes, it replaces @FixMethodOrder

We can't remove functionality unless we have a release where the feature is deprecated. After that, we might be able to remove the feature in the next major release (but we are thinking that we should wait at least a year after deprecation to remove a feature)

[SpOOnman]

I think I don't have all the knowledge required to solve above issues. I thought that @SortWith would be a snap replacement for @FixMethodOrder and it's not. I'm also not aware of other sort discussions on mailing lists.

I understand issues we've discussed but I won't be able to fix them any time soon. I would be glad if this PR can be useful and JUnit team will work onwards on this. But I understand that test sorting is much more complex.

Thank you for your comments @kcooney , I've learned some things. This PR can be closed with or without submit.

[marcphilipp]

@kcooney Can this be closed for now?

[kcooney]

Yes.

[kcooney]

I sent out pull #1130 which adds @SortWith.