Enabling Surefire to run test classes and test methods in any order specified by a new runOrder

[winglam]

The changes in this pull request enable Surefire to run test classes and test methods in any order specified by a newly added <runOrder> (testorder). Namely, the changes include

• A new <runOrder> called testorder that would make Surefire run test classes and test methods in the order specified by <test>. Regex and include/exclude are supported as is the case for other runOrders
• Tests to TestListResolverTest and RunOrderCalculatorTest for the newly added features

The newly added testorder <runOrder> is supported for all JUnit 3 and JUnit 4 tests. I will plan on supporting JUnit 5 and testng tests after these changes are accepted.

Simple (no regex) example using Dubbo:

mvn test -Dsurefire.runOrder=testorder \
-Dtest=org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest#testSticky2,\
org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest#testSticky1,\
org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest#testSticky3,\
org.apache.dubbo.rpc.protocol.dubbo.DubboProtocolTest#testNonSerializedParameter\
 -pl dubbo-rpc/dubbo-rpc-dubbo

Output from Maven should say that the test class DubboLazyConnectTest ran before DubboProtocolTest

The file DubboLazyConnectTest.xml (dubbo-rpc/dubbo-rpc-dubbo/target/surefire-reports/TEST-org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest.xml) should say

<testcase name="testSticky2"...>
<testcase name="testSticky1"...>
<testcase name="testSticky3...>

Regex example using Dubbo:

mvn test -Dsurefire.runOrder=testorder \
-Dtest=org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest*,\
org.apache.dubbo.rpc.protocol.dubbo.DubboProtocolTest#testDubboProtocol*\
 -pl dubbo-rpc/dubbo-rpc-dubbo

Output from Maven should say

Tests run: 4 … in org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest
Tests run: 3 … in org.apache.dubbo.rpc.protocol.dubbo.DubboProtocolTest

The file DubboProtocolTest.xml (dubbo-rpc/dubbo-rpc-dubbo/target/surefire-reports/TEST-org.apache.dubbo.rpc.protocol.dubbo.DubboProtocolTest.xml) should say

<testcase name=”testDubboProtocol”...>
<testcase name="testDubboProtocolWithMina"...>
<testcase name="testDubboProtocolMultiService”...>

Include/Exclude example using Dubbo:

mvn test -Dsurefire.runOrder=testorder \
-Dtest=org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest#test*,\
!org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest#testSticky2\
 -pl dubbo-rpc/dubbo-rpc-dubbo

Output from Maven should say
Tests run: 3 … in org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest

The file DubboLazyConnectTest.xml (dubbo-rpc/dubbo-rpc-dubbo/target/surefire-reports/TEST-org.apache.dubbo.rpc.protocol.dubbo.DubboLazyConnectTest.xml) should say

<testcase name="testSticky1"...>
<testcase name="testSticky3"...>
<testcase name="testSticky4...>

[Tibor17]

Hi @winglam
What's up?

I checked your commit.
I have one argument to the changes in TestListResolver. This class is a filter and not a Comparator. It would be worth not to change it. Are you aiming for comparing two (class, method) pairs/objects? Would it be possible with a separate class? Do you still need to have TestListResolver? I guess you don't because the tests which have run before, they would be a subject to run again. The only trick is to reorder these pairs, am I right?

[winglam]

Hello @Tibor17

Thanks for following up on these changes. I'm a bit busy this week but I will plan on updating these changes by next week.

Currently, my changes seem to use TestListResolver as both a filter and a Comparator (https://github.com/apache/maven-surefire/pull/348/files#diff-390b26f9fa7b88f45e645b6b1fc6b4018570ea482062b93ea85fa00008c1fe20R533).
Would it be fine if I keep the filter-related changes in TestListResolver and just moved the Comparator changes elsewhere, or do you want me to not make any changes to TestListResolver at all?

[Tibor17]

The TestListResolver does not do the trick. It is only a filter. You should not use it.
The whole trick is done in every Surefire Provider implementation. So in this case, you have to modify the providers.
I think this snap code explains how we do it, see the JUnit4Provider:

    private TestsToRun scanClassPath()
    {
        final TestsToRun scannedClasses = scanResult.applyFilter( jUnit4TestChecker, testClassLoader );
        return runOrderCalculator.orderTestClasses( scannedClasses );
    }

The problem is that we do not work with methods. Only classes.
And we should remember JUnit5 because it knows the scripts, dynamic tests, directories, etc.

So all you have to do is to serialize run order (enum + data), see BooterSerializer and deserialize it via BooterDeserializer.
It means that you have transfered the config for RunOrderCalculator fom plugin JVM to the forked JVM. Similar serialization trick in the in-plugin JVM.
Then you should obtain the instance of RunOrderCalculator in the particular provider. The next step is to modify the section I mentioned above which would include the methods as well.

The system property and TestListResolver parts can be avoided.

[Tibor17]

It would help you and us if you have followed these steps, in separate commits, and each step would have unit tests:

1. serialize run order config + test
2. deserialize config + test
3. create deserialized instance of run order calculator, verify the config, and write a test
4. adjust the provider, isolate it in a test
5. write an integration test

[Tibor17]

Of course the documentation should say that forkCount > 1 does not guarantee the order of classes.

[Tibor17]

Maybe a hint, try to run a basic project in debug mode mvn -X test -Dtests=xxx,yyy and go to the folder target/surefire. You should see two properties files and one jar file. Open one property file and you should see CSV of what you have used in -Dtest=. This is your data. Iterate over this parse each substring, create an instance of TestListResolver and pickup the class/method pairs. Order them and use them as necesary in the run order calculator within the provider(s), see the ProviderParameters#getTestRequest().
Regarding the sanity check, go to the AbstractSurefireMojo class and see the parameters checkers in the execute() method. There are more like this. I think you should write a check that the new enum requires the test list but do not use getTests() directly. We are merging several config parameters together and we will change this part of code in #440 so my advice is to focus on this a bit later or check the #440 .

[winglam]

@Tibor17 Thanks a lot for the detailed information, especially the list of steps.

I still have these changes on my list of todos and hope to get to them soon. Hopefully by the time I have something ready PR 440 would be merged by then. I'll reach out again once I have made some progress on some of the steps.

[Tibor17]

How are you @winglam?
We have discussed your pull request in #169 and it seems that @aslakhellesoy would pay an attention.
@winglam do you have time to participate?

[aslakhellesoy]

I'll try to help move this PR forward. I'll start by trying to bring this branch uptodate with master. What's the easiest way to collaborate? I could send a pr to the TestResearchIllinois repo against the existing umaster-tms branch perhaps?

[Tibor17]

@aslakhellesoy
The code is not suited to rebase because this code has some issues. That's the reason why I wanted to save the spare time of @winglam and I wrote 5 steps on how to implement the feature right way and fast enough, see the comments written since of January 27 2022.

[winglam]

@Tibor17 and @aslakhellesoy I would be happy to participate in the discussion for this feature. Unfortunately, I would not have time to implement the five-step process that Tibor laid out by myself until this May.

@aslakhellesoy If you are able to implement the five steps before then, please feel free to do so. I would be happy to look over a PR and work with you to get this feature out the door. Please let me know if this option interests you and how may I help.

[aslakhellesoy]

Hi @winglam - I have created a JIRA issue related to this: https://issues.apache.org/jira/browse/SUREFIRE-2041, with a related PR: #495

In that JIRA issue I have outlined why I don't think ordering test methods is possible (only classes). I'd be interested in your thoughts about this.

[Tibor17]

@winglam
@aslakhellesoy
See my comment #348 (comment)
and see whatever surefire provider implementation and try to find run order or runOrder. Not the TestListResolver as many people think. The AbstractSurefireMojo has to prepare the string upon -Dtest=... and serialize it for later use in RunOrder. Then it should be possible.

[winglam]

@aslakhellesoy as the very first message in this PR explains already, the capability for Surefire to order test methods is already implemented -- albeit implemented in a way that Tibor would prefer that we change to merge the feature into Surefire.

Mainly we just need to follow the steps Tibor outlined so that Surefire can order not just classes but also methods. Happy to comment on the work or test it out, but I won't have time to implement it myself for the next few weeks.

[aslakhellesoy]

the capability for Surefire to order test methods is already implemented

@winglam your implementation of test method ordering can order tests within the same class, but it cannot order them globally.

For example, if you specify -Dtest=testing.Y#a,testing.X#c,testing.Y#b, then the execution order will not be what you specified. It will be:

• testing.Y#a
• testing.Y#b
• testing.X#c

This is because Surefire runs one test class at a time. Supporting a global ordering of test methods would require such a significant refactoring that I'm not sure it's realistic to attempt it. The JUnit 5 team has said they won't support it: junit-team/junit5#2857

As I have described in SUREFIRE-2041 I think the best we can do is an explicit class ordering.

I have implemented class ordering in #495, inspired from this PR but with a different implementation. I'm waiting for @Tibor17 to approve the running of the GitHub Action in that PR, and also to review the code.

[winglam]

@aslakhellesoy Thanks for the clarification. I was aware from the https://issues.apache.org/jira/browse/SUREFIRE-2041 issue that you wanted a global test method ordering feature that supports the interleaving of test methods across test classes and I agree that it may not be realistic to attempt.

More importantly, I'm not sure how one would implement such a feature. Namely, how should one handle class setup and teardown methods (e.g., @BeforeClass from JUnit) for an order such as testing.Y#a,testing.X#c,testing.Y#b? Should one run testing.Y#beforeClass,testing.Y#a,testing.X#beforeClass,testing.X#c,testing.Y#beforeClass,testing.Y#b? What if running testing.Y#beforeClass twice without running testing.Y#afterClass causes a failure? We could then run testing.Y#afterClass after testing.Y#a and before testing.X#beforeClass but then that can add substantial time cost to running tests and still cause unexpected failures if these class setup and teardown methods are not expected to run twice in one JVM.

Not supporting a global test method ordering feature is a purposeful design feature when I did my implementation of test method ordering. For the same reason we may not want to support the feature of interleaving test classes across different modules (e.g., module1.testing.Y,module2.testing.X,module1.testing.Z), I'm not sure whether a global test method ordering feature should be supported or not based on the way modules and test classes are defined and commonly used. I believe there is still much value to support test method ordering within classes (and also the ordering of test classes) and the changes I have in this PR accomplishes both test class ordering and test method ordering, just not the interleaving of test methods across classes and in the way that Tibor suggests us to do so for this feature.

[aslakhellesoy]

@winglam are you by any chance working on a Test Case Prioritization engine? I am :-) I went on a little twitter rant about it yesterday.

If JUnit implemented global test method ordering, I think @BeforeClass and @AfterClass should still only run once. before the first test and after the last one, even if interleaved with other classes. Maybe a new JUnit TestEngine could implement this.

It's still possible to do TCP with class ordering. The APFD would be lower than with a method ordering, but still better than no ordering!

FWIW, Cucumber-Ruby and Cucumber-JS do support global scenario ordering - they can run scenarios in any order even if they are in different files.

[winglam]

I am not actively working on a test prioritization engine at the moment but have some work on it in the past, e.g., https://cs.gmu.edu/~winglam/publications/2020/LamETAL20ISSTA.pdf

I agree with your Twitter post that we should ideally be able to run them in any order (even orders that interleave) but as you pointed out, such a feature should probably first come from JUnit before it gets to Surefire.

I also agree that it is still possible to do TCP with class ordering, but why stop at class ordering? Why not have your TCP be done at class + non-interleaving method ordering? As we've discussed, global test ordering or interleaving method ordering is going to require substantial efforts to accomplish, but class + non-interleaving method ordering is already supported in this PR and just requires one to improve this PR according to the suggestions Tibor provided.

Thanks for the heads up about Cucumber. I was not aware that the framework supported global test ordering. Do they run @BeforeClass (or their version of it) once before the first test even if methods from different classes are interleaved?

[winglam]

@Tibor17 I will have some time for this change now and plan to complete it in the next few weeks. Just want to confirm: are you still interested in this change and do you still want me to follow the steps you laid out earlier (#348 (comment))?