Merge pull request #51298 from holly-cummins/tolerate-surefire-354

Dispose of the FacadeClassLoader at the end of every test plan so surefire reruns are tolerated

Author: gsmet

integration-tests/maven/src/test/resources-filtered/projects/test-flaky-test-multiple-profiles/pom.xml

@@ -89,6 +89,12 @@
           </execution>
         </executions>
       </plugin>
+      <!-- Always use the same version of surefire as the main Quarkus build -->
+      <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-surefire-plugin</artifactId>
+          <version>${version.surefire.plugin}</version>
+      </plugin>
     </plugins>
   </build>
   <profiles>

integration-tests/maven/src/test/resources-filtered/projects/test-flaky-test-single-profile/pom.xml

@@ -89,6 +89,12 @@
           </execution>
         </executions>
       </plugin>
+      <!-- Always use the same version of surefire as the main Quarkus build -->
+      <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-surefire-plugin</artifactId>
+          <version>${version.surefire.plugin}</version>
+      </plugin>
     </plugins>
   </build>
   <profiles>

test-framework/junit5/src/main/java/io/quarkus/test/junit/classloading/FacadeClassLoader.java

@@ -267,7 +267,9 @@ public FacadeClassLoader(ClassLoader parent, boolean isAuxiliaryApplication, Cur
 
     @Override
     public Class<?> loadClass(String name) throws ClassNotFoundException {
-        log.debugf("Facade classloader loading %s", name);
+        // Debuggers, beware: If the same class is loaded with the same FacadeClassLoader instance using Class.forName(), a cached copy will be returned,
+        // and nothing will be logged here; it will look like classloading didn't happen, but the problem is actually a stale instance
+        log.debugf("Facade classloader loading %s for the first time\n", name);
 
         if (peekingClassLoader == null) {
             throw new RuntimeException("Attempted to load classes with a closed classloader: " + this);

test-framework/junit5/src/main/java/io/quarkus/test/junit/launcher/CustomLauncherInterceptor.java

@@ -4,10 +4,13 @@
 import org.junit.platform.launcher.LauncherDiscoveryRequest;
 import org.junit.platform.launcher.LauncherSession;
 import org.junit.platform.launcher.LauncherSessionListener;
+import org.junit.platform.launcher.TestExecutionListener;
+import org.junit.platform.launcher.TestPlan;
 
 import io.quarkus.test.junit.classloading.FacadeClassLoader;
 
-public class CustomLauncherInterceptor implements LauncherDiscoveryListener, LauncherSessionListener {
+public class CustomLauncherInterceptor
+        implements LauncherDiscoveryListener, LauncherSessionListener, TestExecutionListener {
 
     private static FacadeClassLoader facadeLoader = null;
     // Also use a static variable to store a 'first' starting state that we can reset to
@@ -93,7 +96,6 @@ public void launcherDiscoveryStarted(LauncherDiscoveryRequest request) {
             System.setProperty("java.util.concurrent.ForkJoinPool.common.threadFactory",
                     "io.quarkus.bootstrap.forkjoin.QuarkusForkJoinWorkerThreadFactory");
         }
-
     }
 
     private void adjustContextClassLoader() {
@@ -108,7 +110,6 @@ private void adjustContextClassLoader() {
 
     @Override
     public void launcherDiscoveryFinished(LauncherDiscoveryRequest request) {
-
         if (!isProductionModeTests()) {
             // We need to support two somewhat incompatible scenarios.
             // If there are user extensions present which implement `ExecutionCondition`, and they call config in `evaluateExecutionCondition`,
@@ -129,7 +130,10 @@ public void launcherDiscoveryFinished(LauncherDiscoveryRequest request) {
 
     @Override
     public void launcherSessionClosed(LauncherSession session) {
+        clearContextClassloader();
+    }
 
+    private static void clearContextClassloader() {
         try {
             // Tidy up classloaders we created, but not ones created upstream
             // Also make sure to reset the TCCL so we don't leave a closed classloader on the thread
@@ -150,4 +154,37 @@ public void launcherSessionClosed(LauncherSession session) {
             throw new RuntimeException("Failed to close custom classloader", e);
         }
     }
+
+    /**
+     * Called when the execution of the {@link TestPlan} has started,
+     * <em>before</em> any test has been executed.
+     *
+     * <p>
+     * Called from the same thread as {@link #testPlanExecutionFinished(TestPlan)}.
+     *
+     * In continuous testing, the test plan listener seems not to be called, perhaps because of a different execution model.
+     *
+     * @param testPlan describes the tree of tests about to be executed
+     */
+    public void testPlanExecutionStarted(TestPlan testPlan) {
+        // Do nothing, but have the method here for symmetry :)
+    }
+
+    /**
+     * Called when the execution of the {@link TestPlan} has finished,
+     * <em>after</em> all tests have been executed.
+     *
+     * <p>
+     * Called from the same thread as {@link #testPlanExecutionStarted(TestPlan)}.
+     *
+     * If tests failed and are rerun by surefire, the same session will be used for all runs, so we need to get rid of the
+     * FacadeClassLoader associated with the previous run, since its app will be closed and its classloaders will all be stale.
+     *
+     * In continuous testing, the test plan listener seems not to be called, perhaps because of a different execution model.
+     *
+     * @param testPlan describes the tree of tests that have been executed
+     */
+    public void testPlanExecutionFinished(TestPlan testPlan) {
+        clearContextClassloader();
+    }
 }

test-framework/junit5/src/main/resources/META-INF/services/org.junit.platform.launcher.TestExecutionListener

@@ -1 +1,2 @@
 io.quarkus.test.junit.launcher.ExecutionListener
+io.quarkus.test.junit.launcher.CustomLauncherInterceptor