Allow the test classes directory to be configured for the NativeTestMojo

[thegridman]

The NativeTestMojo hard codes the location of the test classes to be the project.build.testOutputDirectory. Some Maven projects have test classes in other locations, for example under src/main/java which compiles them to the project.build.outputDirectory.

This PR allows the test classes location to be configured for the NativeTestMojo in the same way that plugins such as Surefire and Failsafe do by setting the <testClassesDirectory> element in the plugin configuration. The default is the project.build.testOutputDirectory.

[vjovanov]

I am wondering if we can go without an extra parameter. How does the surefire plugin know the test-classes path?

[thegridman]

Surefire and Failsafe both assume that the tests are in the default locations. But both have an optional <testClassesDirectory> configuration element that allows you to set a different location. In our case we have test modules where that only contain tests and the source is all under src/main/java and gets compiled to target/classes so we have to tell Surefire and Failsafe to use these directories.

[vjovanov]

@alvarosanchez @melix do you think we can extract this information from Surefire in our plugin, so we don't have to duplicate this entry?

[thegridman]

@vjovanov Now I have had another look at how your plugin works you could indeed extract the value from the Surefire or Failsafe plugins. The problem here though is how you are doing this now for the system properties and environment variables does not always work. The current code assumes these things are in the <configuration> section of the plugin XML, but that is not always the case. In our projects for example for plugins we use the <executions> section to configure what the plugin does and inside an <execution> is the <configuration> for that execution (which I believe is combined with any global <configuration>). The issue here though is working out which execution you need to use because there could be multiple. Your plugin would need to know the phase that was being executed so that it could work out the right execution.

[thegridman]

Actually it might not be as hard as I thought. For Surefire you just need to find the <execution> bound to the test goal, and for Failsafe the <execution> bound to the integration-test and verfiy goals.

I'm happy to try this out in my branch.

[dnestoro]

This looks okay to me (with few questions that I left). I will wait for @alvarosanchez or @melix approval because I don't know if this is potentially against some Maven policies or conventions (other than that, this looks like a reasonable feature).

[dnestoro]

Am I missing something or this profile is not used in any test? If so, we should delete it.

[dnestoro]

Also, is this profile ever used (same question for the profile below)?

[dnestoro]

Since this is not a default location, should we print some "warning" where the tests come from (if the location is not default)? Maybe with logger.debug?

[thegridman]

You could log it but the Surefire plugin does not specifically log config settings (Surefire is basically the functionality you are copying). If you run a Maven build with debug (i.e. -X) then it logs all the plugin configs before they execute anyway.

[thegridman]

I have been doing some more work on this based on @vjovanov comment about getting the test classes directory from the Surefire plugin. The way this plugin works currently, it merges in the system properties and environment variables from both the surefire and failsafe plugins if they are configured in the project. So it sounds like it should be simple to also pull in the test classes directory from those plugins but it starts to become a can of worms.

If my pom.xml only contained is a simple surefire config like the one below then it is easy.

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-surefire-plugin</artifactId>
    <version>3.5.3</version>
    <configuration>
        <testClassesDirectory>${project.build.outputDirectory}</testClassesDirectory>
    </configuration>
</plugin>

But we use executions in plugin configs like below, which your plugin does not support

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-surefire-plugin</artifactId>
    <version>3.5.3</version>
    <executions>
        <execution>
            <goals>
                <goal>test</goal>
            </goals>
            <phase>test</phase>
            <configuration>
                <testClassesDirectory>${project.build.outputDirectory}</testClassesDirectory>
            </configuration>
        </execution>
    </executions>
</plugin>

I do not think this is too big an issue, you just need to search through any executions defined for the Surefire plugin to find the one with the test goal. I cannot see a reason why you'd have more than one execution for the Surefire plugin so that should work.

The complications start when we have use both Surefire and Failsafe. As I said above, the plugin is currently merging system properties and environment variables from both Surefire and Failsafe and this might not be correct. Typically Surefire is for unit tests that execute all in a single JVM, which is how the native test plugin runs tests. Failsafe is typically used for integration tests where each test class runs in a forked JVM so tests are more isolated.

I could have a pom.xml file like this:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-surefire-plugin</artifactId>
    <configuration>
        <systemPropertyVariables>
            <my.property>one</my.property>
        </systemPropertyVariables>
    </configuration>
</plugin>

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-failsafe-plugin</artifactId>
    <configuration>
        <systemPropertyVariables>
            <my.property>two</my.property>
        </systemPropertyVariables>
    </configuration>
    <executions>
        <execution>
            <goals>
                <goal>integration-test</goal>
                <goal>verify</goal>
            </goals>
        </execution>
    </executions>
</plugin>

Both Surefire and Failsafe set a value for the my.property system property, but they set different values. Which will be used? Currently the Failsafe value just because that is the order the native plugin processes things.

It would be reasonably simple if a mojo knew which phase was executing when execute() was called, but Maven does not supply this information to the mojo.

I believe the way to fix this is to have two native test mojos, the current NativeTestMojo that has the test goal and by default binds to the test phase, then a second mojo NativeIntegrationTestMojo that extends NativeTestMojo and has an integration-test goal and by default binds to the integration-test phase.

The bulk of the code would remain in the NativeTestMojo (or could all be extracted to an abstract test mojo). When either of the mojos is executed we then know which goal is running, either test or integration-test and then the correct values can be pulled from Surefire and Failsafe.

I had a go at writing the code for this, but I did not want to do too much in case there are big objections to it.

[thegridman]

OK, so maybe I shouldn't believe everything said on the web before looking at all the Maven code. You can determine things like the goal and phase in a mojo. One NativeTestMojo would be fine as it can work out whether it is in running the test or integration-test goal

[thegridman]

I have pushed new commits to this PR to fix the following issues.

• The native plugin merged configuration from both Surefire and Failsafe into the native plugin test configuration regardless of whether the goal being executed is test or integration-test. This is fixed by having two goals for the native plugin, test and integration-test and then the plugin knows how to correctly configure itself.
• The native plugin only obtains configuration from the Surefire or Failsafe <configuration> section and ignored any configuration in the <executions>. This is fixed by making the native plugin properly look for executions in the Surefire and Failsafe plugins.
• The native plugin added a JUnit listener to the Surefire and Failsafe configuration to create a file of test identifiers which is how the native plugin identifies the tests to run. The problem here is that if a project uses both Surefire and Failsafe to run unit and integration tests. First Surefire runs and creates a test-id file, then native unit tests are run with that file, then Failsafe runs and creates another test-id file for the integration tests, then the native intergration test runs and sees both test id files so runs unit and integration tests. This is fixed by putting the test id files in different directories.

With the current code in this PR it looks like the native plugin can now be used to run both unit and integration tests using both Surefire and Failsafe. Which is how I would think most project use these plugins.

Even with these fixes though, we could not run integration tests in our project because we rely on Failsafe fork mode where every test class runs in a fork. I can see no way the native plugin can do this as it just uses Unit to run the tests and Unit does not support forks. You would end up duplicating a lot of what Falsafe/Surefire do to support forks, which is a massive job, or somehow being able to extend the functionality of Surefire/Failsafe so that when they execute a fork, they actually run a native image. Either way a big amount of work.

[vjovanov]

Are these two needed for the testing now that all is working? These are the flags to use for easier debugging.

[vjovanov]

@melix are we handling this case in Gradle properly, i.e. the testClassesDirs?

[vjovanov]

Should we use 3.5.3 consistently?

[vjovanov]

@dnestoro do we need to document this? Also, could we execute only native integration tests (once the JUnit issues are fixed)? What would be the command for that?

[vjovanov]

Looks great from my side. Thank you very much for the contribution!

Most of the remaining questions are now on the NBT reviewers to discuss and answer.

[thegridman]

Another commit to clean up some unused code paths. After a bit of testing I discovered that the Plugin model always has a default PluginExecution for its default goal and that Maven merges the plugins global configuration into each execution configuration. This then simplifies the code a little as it just needs to find the execution for Surefire or Failsafe that is bound to the currently executing goal.

[thegridman]

One other thing to be aware of. When using the native plugin to execute tests it must run after the Surefire or Failsafe plugin otherwise the test id files do not exist and the build fails. I'm not sure whether this is in the doc or not and maybe it goes away if you sort out the double test execution thing anyway. The native plugin is typically bound to the same phase as the Surefire or Failsafe plugin, i.e. test or integration-test and the only way to ensure execution order is to ensure the Surefire or Failsafe plugins come before the Native Plugin in the pom.xml. This is one of the flakey things about Maven.

[vjovanov]

One other thing to be aware of. When using the native plugin to execute tests it must run after the Surefire or Failsafe plugin otherwise the test id files do not exist and the build fails. I'm not sure whether this is in the doc or not and maybe it goes away if you sort out the double test execution thing anyway. The native plugin is typically bound to the same phase as the Surefire or Failsafe plugin, i.e. test or integration-test and the only way to ensure execution order is to ensure the Surefire or Failsafe plugins come before the Native Plugin in the pom.xml. This is one of the flakey things about Maven.

Ouch. Maybe we can handle this with a proper error message and documentation. @dnestoro @alvarosanchez can we do this to make it easier for the users?