Discover tests in a fork when a toolchain JDK is used (#3444)

* Discover tests in a fork when a toolchain JDK is used

When forking has to list the tests first (forkCount > 1 or
reuseForks=false), Surefire did it in Maven's JVM. If the tests were
built for a newer JDK, that failed with UnsupportedClassVersionError.

Now the listing runs in a short-lived fork using the toolchain JDK, so
no test class is loaded in Maven's JVM. The extra fork only starts when
the test JVM differs from Maven's.

Fixes #2151


---------

Signed-off-by: Olivier Lamy <olamy@apache.org>

Author: olamy

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/BooterSerializer.java

@@ -37,12 +37,14 @@
 import org.apache.maven.surefire.booter.ProcessCheckerType;
 import org.apache.maven.surefire.booter.ProviderConfiguration;
 import org.apache.maven.surefire.booter.StartupConfiguration;
+import org.apache.maven.surefire.booter.TypeEncodedValue;
 
 import static org.apache.maven.plugin.surefire.SurefireHelper.replaceForkThreadsInPath;
 import static org.apache.maven.surefire.booter.AbstractPathConfiguration.CHILD_DELEGATION;
 import static org.apache.maven.surefire.booter.AbstractPathConfiguration.CLASSPATH;
 import static org.apache.maven.surefire.booter.AbstractPathConfiguration.ENABLE_ASSERTIONS;
 import static org.apache.maven.surefire.booter.AbstractPathConfiguration.SUREFIRE_CLASSPATH;
+import static org.apache.maven.surefire.booter.BooterConstants.DISCOVER_TESTS_OUTPUT_FILE;
 import static org.apache.maven.surefire.booter.BooterConstants.EXCLUDES_PROPERTY_PREFIX;
 import static org.apache.maven.surefire.booter.BooterConstants.FAIL_FAST_COUNT;
 import static org.apache.maven.surefire.booter.BooterConstants.FORKTESTSET;
@@ -105,10 +107,12 @@ File serialize(
             boolean readTestsFromInStream,
             Long pid,
             int forkNumber,
-            String forkNodeConnectionString)
+            String forkNodeConnectionString,
+            String discoverTestsOutputFile)
             throws IOException {
         SurefireProperties properties = new SurefireProperties(sourceProperties);
         properties.setNullableProperty(FORK_NODE_CONNECTION_STRING, forkNodeConnectionString);
+        properties.setNullableProperty(DISCOVER_TESTS_OUTPUT_FILE, discoverTestsOutputFile);
         properties.setProperty(PLUGIN_PID, pid);
 
         AbstractPathConfiguration cp = startupConfiguration.getClasspathConfiguration();
@@ -179,6 +183,10 @@ private static String getTypeEncoded(Object value) {
         if (value == null) {
             return null;
         }
+        if (value instanceof TypeEncodedValue) {
+            TypeEncodedValue encoded = (TypeEncodedValue) value;
+            return encoded.getType() + "|" + encoded.getValue();
+        }
         String valueToUse = value instanceof Class ? ((Class<?>) value).getName() : value.toString();
         return value.getClass().getName() + "|" + valueToUse;
     }

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/booterclient/ForkStarter.java

@@ -23,9 +23,12 @@
 import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Queue;
 import java.util.Set;
@@ -73,6 +76,7 @@
 import org.apache.maven.surefire.booter.StartupConfiguration;
 import org.apache.maven.surefire.booter.SurefireBooterForkException;
 import org.apache.maven.surefire.booter.SurefireExecutionException;
+import org.apache.maven.surefire.booter.TypeEncodedValue;
 import org.apache.maven.surefire.extensions.EventHandler;
 import org.apache.maven.surefire.extensions.ForkChannel;
 import org.apache.maven.surefire.extensions.ForkNodeFactory;
@@ -86,6 +90,7 @@
 import static java.lang.StrictMath.min;
 import static java.lang.System.currentTimeMillis;
 import static java.lang.Thread.currentThread;
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Collections.addAll;
 import static java.util.UUID.randomUUID;
 import static java.util.concurrent.Executors.newScheduledThreadPool;
@@ -109,6 +114,7 @@
 import static org.apache.maven.surefire.api.util.internal.StringUtils.NL;
 import static org.apache.maven.surefire.booter.SystemPropertyManager.writePropertiesFile;
 import static org.apache.maven.surefire.booter.SystemUtils.pidOf;
+import static org.apache.maven.surefire.booter.SystemUtils.toJdkHomeFromJre;
 import static org.apache.maven.surefire.shared.utils.cli.ShutdownHookUtils.addShutDownHook;
 import static org.apache.maven.surefire.shared.utils.cli.ShutdownHookUtils.removeShutdownHook;
 
@@ -342,8 +348,8 @@ private RunResult runSuitesForkOnceMultiple(SurefireProperties effectiveSystemPr
 
         Queue<String> tests = new ConcurrentLinkedQueue<>();
 
-        for (Class<?> clazz : getSuitesIterator()) {
-            tests.add(clazz.getName());
+        for (String testClassName : getTestClassNames(effectiveSystemProps)) {
+            tests.add(testClassName);
         }
 
         Queue<TestProvidingInputStream> forks = new ConcurrentLinkedQueue<>();
@@ -408,7 +414,8 @@ private RunResult runSuitesForkPerTestSet(SurefireProperties effectiveSystemProp
             addShutDownHook(shutdown);
             int failFastCount = providerConfiguration.getSkipAfterFailureCount();
             AtomicInteger notifyForksToSkipTestsNow = new AtomicInteger(failFastCount);
-            Collection<Future<RunResult>> results = stream(((Iterable<?>) getSuitesIterator()).spliterator(), false)
+            Collection<Future<RunResult>> results = stream(
+                            getForkPerTestSetTestSets(effectiveSystemProps).spliterator(), false)
                     .filter(fork -> !forkConfiguration.getPluginPlatform().isShutdown())
                     .map(testSet -> (Callable<RunResult>) () -> {
                         int forkNumber = drawNumber();
@@ -513,6 +520,29 @@ private RunResult fork(
             ForkNodeFactory forkNodeFactory,
             boolean readTestsFromInStream)
             throws SurefireBooterForkException {
+        return fork(
+                testSet,
+                providerProperties,
+                forkClient,
+                effectiveSystemProperties,
+                forkNumber,
+                commandReader,
+                forkNodeFactory,
+                readTestsFromInStream,
+                null);
+    }
+
+    private RunResult fork(
+            Object testSet,
+            PropertiesWrapper providerProperties,
+            ForkClient forkClient,
+            SurefireProperties effectiveSystemProperties,
+            int forkNumber,
+            AbstractCommandReader commandReader,
+            ForkNodeFactory forkNodeFactory,
+            boolean readTestsFromInStream,
+            String discoverTestsOutputFile)
+            throws SurefireBooterForkException {
         CloseableCloser closer = new CloseableCloser(forkNumber, commandReader);
         final String tempDir;
         final File surefireProperties;
@@ -542,7 +572,8 @@ private RunResult fork(
                     readTestsFromInStream,
                     pluginPid,
                     forkNumber,
-                    connectionString);
+                    connectionString,
+                    discoverTestsOutputFile);
 
             if (effectiveSystemProperties != null) {
                 SurefireProperties filteredProperties =
@@ -720,11 +751,130 @@ private Iterable<Class<?>> getSuitesIterator() throws SurefireBooterForkExceptio
                     startupConfiguration, providerConfiguration, unifiedClassLoader, reporterFactory);
             SurefireProvider surefireProvider = providerFactory.createProvider(false);
             return surefireProvider.getSuites();
+        } catch (UnsupportedClassVersionError e) {
+            throw new SurefireBooterForkException(
+                    "Could not load the test classes to figure out which tests to run. This usually means the tests "
+                            + "were compiled with a newer JDK than the one running Maven, and forking (forkCount greater "
+                            + "than 1 or reuseForks=false) needs to list the tests using Maven's own JVM. Point the "
+                            + "'jvm' parameter or a 'jdk' toolchain at a JDK that is at least as new as the one used to "
+                            + "compile the tests.",
+                    e);
         } catch (SurefireExecutionException e) {
             throw new SurefireBooterForkException("Unable to create classloader to find test suites", e);
         }
     }
 
+    /**
+     * Gives back the names of the test classes to run. If the tests run in a different JVM than Maven itself
+     * (because a {@code jdk} toolchain or the {@code jvm} parameter is set), we ask a short-lived fork to list
+     * them, so Maven never has to load test classes that may be compiled for a newer JDK. Otherwise we just
+     * list them here, like before.
+     *
+     * @see <a href="https://github.com/apache/maven-surefire/issues/2151">Issue 2151</a>
+     */
+    private List<String> getTestClassNames(SurefireProperties effectiveSystemProperties)
+            throws SurefireBooterForkException {
+        if (isForkJvmDifferentFromBuildJvm()) {
+            return discoverTestClassNames(effectiveSystemProperties);
+        }
+        List<String> testClassNames = new ArrayList<>();
+        for (Class<?> clazz : getSuitesIterator()) {
+            testClassNames.add(clazz.getName());
+        }
+        return testClassNames;
+    }
+
+    /**
+     * Gives back the test sets for {@code fork-per-test-set} runs. When the tests run in a different JVM than Maven,
+     * we discover the class names in a fork (see {@link #getTestClassNames}) and wrap each one as a
+     * {@link TypeEncodedValue} of type {@link Class}, so every fork loads the class with its own JVM. Otherwise we
+     * reuse the already-loaded {@link Class} instances, like before.
+     */
+    private Iterable<?> getForkPerTestSetTestSets(SurefireProperties effectiveSystemProperties)
+            throws SurefireBooterForkException {
+        if (isForkJvmDifferentFromBuildJvm()) {
+            return discoverTestClassNames(effectiveSystemProperties).stream()
+                    .map(testClassName -> new TypeEncodedValue(Class.class.getName(), testClassName))
+                    .collect(toList());
+        }
+        return getSuitesIterator();
+    }
+
+    /**
+     * @return {@code true} when the tests run in a different JDK than Maven (set through a {@code jdk} toolchain or the
+     *     {@code jvm} parameter), in which case we need a fork to list the test classes.
+     */
+    private boolean isForkJvmDifferentFromBuildJvm() {
+        File testsJdkHome = forkConfiguration.getJdkForTests().getJdkHome();
+        return testsJdkHome != null && !testsJdkHome.equals(toJdkHomeFromJre());
+    }
+
+    /**
+     * Starts one short-lived fork with the configured test JVM that lists the test classes to run and writes their
+     * names to a temporary file, then reads them back. The candidate class names already travel to the fork through
+     * the provider properties (from the directory scan Maven did earlier), so no test class is loaded in Maven's JVM.
+     */
+    private List<String> discoverTestClassNames(SurefireProperties effectiveSystemProperties)
+            throws SurefireBooterForkException {
+        File discoveryFile;
+        try {
+            discoveryFile =
+                    File.createTempFile("surefire-discovered-tests", ".txt", forkConfiguration.getTempDirectory());
+        } catch (IOException e) {
+            throw new SurefireBooterForkException("Cannot create temporary file for test discovery", e);
+        }
+
+        TestLessInputStreamBuilder builder = new TestLessInputStreamBuilder();
+        TestLessInputStream stream = builder.build();
+        Thread shutdown = createImmediateShutdownHookThread(builder, providerConfiguration.getShutdown());
+        ScheduledFuture<?> ping = triggerPingTimerForShutdown(builder);
+        int forkNumber = drawNumber();
+        PropertiesWrapper props = new PropertiesWrapper(providerConfiguration.getProviderProperties());
+        try {
+            addShutDownHook(shutdown);
+            DefaultReporterFactory forkedReporterFactory =
+                    new DefaultReporterFactory(startupReportConfiguration, log, forkNumber);
+            defaultReporterFactories.add(forkedReporterFactory);
+            ForkClient forkClient = new ForkClient(forkedReporterFactory, stream, forkNumber);
+            ForkNodeFactory node = forkConfiguration.getForkNodeFactory();
+            if (!forkConfiguration.getPluginPlatform().isShutdown()) {
+                fork(
+                        null,
+                        props,
+                        forkClient,
+                        effectiveSystemProperties,
+                        forkNumber,
+                        stream,
+                        node,
+                        false,
+                        discoveryFile.getAbsolutePath());
+            }
+            return readTestClassNames(discoveryFile);
+        } finally {
+            returnNumber(forkNumber);
+            removeShutdownHook(shutdown);
+            ping.cancel(true);
+            builder.removeStream(stream);
+            if (!forkConfiguration.isDebug() && !discoveryFile.delete()) {
+                discoveryFile.deleteOnExit();
+            }
+        }
+    }
+
+    private static List<String> readTestClassNames(File discoveryFile) throws SurefireBooterForkException {
+        if (!discoveryFile.isFile()) {
+            return Collections.emptyList();
+        }
+        try {
+            return Files.readAllLines(discoveryFile.toPath(), UTF_8).stream()
+                    .map(String::trim)
+                    .filter(String::isEmpty)
+                    .collect(toList());
+        } catch (IOException e) {
+            throw new SurefireBooterForkException("Cannot read discovered tests from " + discoveryFile, e);
+        }
+    }
+
     private static Thread createImmediateShutdownHookThread(
             final TestLessInputStreamBuilder builder, final Shutdown shutdownType) {
         return SHUTDOWN_HOOK_THREAD_FACTORY.newThread(new Runnable() {

maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/BooterDeserializerProviderConfigurationTest.java

@@ -234,7 +234,15 @@ private ProviderConfiguration saveAndReload(
             test = "aTest";
         }
         final File propsTest = booterSerializer.serialize(
-                props, booterConfiguration, testProviderConfiguration, test, readTestsFromInStream, 51L, 1, "pipe://1");
+                props,
+                booterConfiguration,
+                testProviderConfiguration,
+                test,
+                readTestsFromInStream,
+                51L,
+                1,
+                "pipe://1",
+                null);
         BooterDeserializer booterDeserializer = new BooterDeserializer(new FileInputStream(propsTest));
         assertEquals("51", (Object) booterDeserializer.getPluginPid());
         assertEquals("pipe://1", booterDeserializer.getConnectionString());

maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/booterclient/BooterDeserializerStartupConfigurationTest.java

@@ -165,7 +165,8 @@ private StartupConfiguration saveAndReload(StartupConfiguration startupConfigura
                 false,
                 null,
                 1,
-                "tcp://localhost:63003");
+                "tcp://localhost:63003",
+                null);
         try (InputStream inputStream = Files.newInputStream(propsTest.toPath())) {
             BooterDeserializer booterDeserializer = new BooterDeserializer(inputStream);
             assertNull(booterDeserializer.getPluginPid());