Implement forking support in the JUnitPlatformProvider for Maven Surefire

[smoyer64]

This is a TODO in the JUnitPlatformProvider.

Related Issues

• Reach out to Maven Surefire maintainers regarding JUnit Platform provider #31
• Opt-in support for parallel execution #60
• Introduce a @NotThreadSafe annotation #141
• Implement suite support in the JUnitPlatformProvider for Maven Surefire #362

[lutovich]

@sbrannen may I help with this one? :)

[sbrannen]

@lutovich, definitely... go for it! 👍

[lutovich]

Hello @sbrannen,

This is my first time working with surefire code so I'd like to double check with you couple things.
AbstractProvider requires two methods to be implemented:

1. Iterable<Class<?>> getSuites()
2. RunResult invoke(Object)
   First method returns set of things for forks to run. Second method is invoked in a fork for each thing returned by getSuites().
   Launcher API can discover and execute tests. So (1) is basically a discovery phase and (2) is an execution phase.

Is my understanding correct?

I've done some preliminary coding here: lutovich@59de0a2.
Tested this change on https://github.com/junit-team/junit5-samples/tree/master/junit5-maven-consumer and it seems, tests are actually executed in different VMs :)

[marcphilipp]

Looks good to me. However, I am not sure of the exact semantics of those two methods. Our plan was to get M1 out of the door and then get in touch with the Surefire maintainers again. Having said that, I think it certainly can't hurt to have a more complete "prototype" implementation. So, I'm in favor of merging a pull request with your changes.

I know that there are currently no tests for the Surefire provider. Do you have time to add some?

[lutovich]

@marcphilipp thanks! I'll add tests and create a PR.

There seem to be more features that current implementation does not support, like:
http://maven.apache.org/surefire/maven-surefire-plugin/examples/rerun-failing-tests.html
http://maven.apache.org/surefire/maven-surefire-plugin/examples/skip-after-failure.html.
I do not know if they should be addressed now.

Small question about Launcher API. Currently both #discover() and #execute() accept a discovery request. For the current surefire provider this means that classpath scanning and reflection magic will happen twice - in #getSuits() and in #invoke(). Is this on purpose to keep the API clean? Or maybe there are considerations to make #execute() take a TestPlan?

[marcphilipp]

Classpath scanning in invoke() will only happen if it is called with null, right? So it won't happen twice or am I missing something?

[lutovich]

@marcphilipp my understanding of surefire provider lifecycle is like this:

1. with no forks classpath scanning happens in invoke() when it is given a null argument
2. with forks classpath scanning happens in getSuits() and then invoke() of each fork gets an element from the Iterator returned by getSuits()

My question was mostly about Launcher. Both Launcher#discover() and Launcher#execute() execute discovery request, this probably means that each class will be walked by reflection twice. Once by #discover() and once by #execute().

[marcphilipp]

There are a bunch of reasons why discovery is done twice. First and foremost, TestPlan and the contained TestIdentifiers do not contain all the information the engines need to execute the tests. They need TestDescriptors. In addition, the result of discover might already be obsolete when execute is called. The result discover is intended to be used as a preview of which tests will get executed by a request.

If performance turns out to become a problem, we can think about a caching or session concept in Launcher. It could store the mapping from TestIdentifiers to TestDescriptors and we could add an execute(TestPlan) method. But I would rather not do that.

[lutovich]

Sounds good, thanks for the explanation!

[marcphilipp]

Fixed by #382.