sortWith() and orderWith() should respect @FixMethodOrder

[kcooney]

When you order a suite using Request.sortWith() all of the runners that implement Sortable (like BlockJUnit4ClassRunner) will reorder their methods using the passed-in Comparator, even if the ordering was specified with @FixMethodOrder. The same holds true for Request.orderWith(). JUnit should instead not reorder methods for classes annotated with @FixMethodOrder

Background:

Someone at my company was trying to implement code to shuffle method orders to catch tests that produced different pass/fail results based on ordering (these shuffling tests are useful, for instance, if you want to ensure that you could always add @Ignore to a broken test without causing otherwise-passing tests to start failing). We have a very large code base, and some tests are annotated with @FixMethodOrder because we know that they only pass if run in a specific order. We have enough of these that it will take a significant amount of time to fix them, especially for parts of the code where the original authors have left the company.

We had to write custom shuffling code, because the code in JUnit for ordering did not respect @FixMethodOrder. In some cases, our customized shuffling code has to bail out with a "Shuffling not supported for this suite" error message.

Can we please have ParentRunner.sort(Sorter) and ParentRunner.order(Orderer) do nothing if the Description indicates that the class was annotated with @FixMethodOrder?

I do realize that this would be a change in behavior from previous versions of JUnit. I don't think the change I'm proposing would cause tests to fail. The only way that could happen is if there is some code outside of JUnit that specifically orders the suite in an order, but if projects run into this problem when trying to migrate to 4.13, the easy work-around would be to remove the @FixMethodOrder annotation.

[marcphilipp]

Makes sense to me.

@stefanbirkner WDYT?

[stefanbirkner]

Thanks @kcooney for the thoroughly introduction to the problem. IMHO we should implement this change. @kcooney do you have time to create a PR or should I implement the change. I have some free time during the next two days.

I think we should not postpone 4.13 because of this change but instead target a 4.14 release within the next three month. We currently have a release candidate that seems to be ready for the final release. With a new change we need to create another release candidate that will delay the release again although IMO a lot of people are waiting for it. @kcooney @marcphilipp WDYT?

[kcooney]

@stefanbirkner I thought we weren't planning on having any releases after 4.13 (which is why we decided to deprecate ExpectedException.none() in the same release where Assert.assertThrows() was added). If we think we will have a 4.14, then perhaps we should remove the deprecation.

I have limited free time right now, but this isn't a big change, so I might be able to find some time at the end of the week.

[stefanbirkner]

You're right. There was a plan for 4.13 being the last release. @marcphilipp is this still valid?

[marcphilipp]

Yes, I think so unless there's a really good reason.

Since orderWith() is new, I think we should make the change now and postpone 4.13.

[kcooney]

@marcphilipp while Request,orderWith() and @OrderWith are new, Request.sortWith() is not. I think it might be odd to have the orderWith() respect @FixMethodOrder but sortWith() not respect it. That being said, if we change the sorting code so sortWith() respects @FixMethodOrder we would likely need to see who might broken by the change, which would require a delay of the 4.13 release.

I'll start looking into where we could apply the fixes.