Move FacadeClassLoader to the JUnit5 module and repatriate some of th…

…e classes it dragged across
holly-cummins · Feb 26, 2025 · 57767e1 · 57767e1
1 parent 6bf447f
commit 57767e1
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 47 deletions.
diff --git a/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java b/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java
@@ -4,6 +4,8 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -74,7 +76,7 @@
 import io.quarkus.deployment.util.IoUtil;
 import io.quarkus.dev.console.QuarkusConsole;
 import io.quarkus.dev.testing.TracingHandler;
-import io.quarkus.test.junit.classloading.FacadeClassLoader;
+import io.quarkus.logging.Log;
 
 /**
  * This class is responsible for running a single run of JUnit tests.
@@ -602,8 +604,6 @@ private DiscoveryResult discoverTestClasses() {
         //we will need to fix this sooner rather than later though
 
         //we also only run tests from the current module, which we can also revisit later
-
-        // TODO consolidate logic here with facadeclassloader, which is trying to solve similar problems; maybe even share the canary loader class?
         Indexer indexer = new Indexer();
         moduleInfo.getTest()
                 .ifPresent(test -> {
@@ -755,26 +755,40 @@ private DiscoveryResult discoverTestClasses() {
         List<Class<?>> utClasses = new ArrayList<>();
 
         // TODO guard to only do this once? is this guard sufficient? see "wrongprofile" in QuarkusTestExtension
+        ClassLoader testLoadingClassLoader;
+        try {
+            Class fclClazz = Class.forName("io.quarkus.test.junit.classloading.FacadeClassLoader");
+            Method clearSingleton = fclClazz.getMethod("clearSingleton");
+            Method instance = fclClazz.getMethod("instance", ClassLoader.class, boolean.class, Map.class, Set.class,
+                    String[].class);
 
-        FacadeClassLoader.clearSingleton();
-        // Passing in the test classes is annoyingly necessary because in dev mode getAnnotations() on the class returns an empty array
+            clearSingleton.invoke(null);
 
-        FacadeClassLoader facadeClassLoader = FacadeClassLoader.instance(this.getClass().getClassLoader(), true, profiles,
-                quarkusTestClassesForFacadeClassLoader, moduleInfo.getMain()
-                        .getClassesPath(),
-                moduleInfo.getTest()
-                        .get()
-                        .getClassesPath()); // TODO ideally it would be in a different module, but that is hard CollaboratingClassLoader.construct(parent);
+            // Passing in the test classes is annoyingly necessary because in dev mode getAnnotations() on the class returns an empty array
+            testLoadingClassLoader = (ClassLoader) instance.invoke(this.getClass().getClassLoader(), true, profiles,
+                    quarkusTestClassesForFacadeClassLoader, moduleInfo.getMain()
+                            .getClassesPath(),
+                    moduleInfo.getTest()
+                            .get()
+                            .getClassesPath());
 
-        Thread.currentThread()
-                .setContextClassLoader(facadeClassLoader);
+            Thread.currentThread()
+                    .setContextClassLoader(testLoadingClassLoader);
+        } catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+            e.printStackTrace(); // TODO remove this
+            // This is fine, and usually just means that test-framework/junit5 isn't one of the project dependencies
+            // In that case, fallback to loading classes as we normally would, using our own classloader
+            Log.debug("Could not create FacadeClassLoader: " + e);
+
+            testLoadingClassLoader = Thread.currentThread().getContextClassLoader();
+        }
 
         for (String i : quarkusTestClasses) {
             try {
                 // We could load these classes directly, since we know the profile and we have a handy interception point;
                 // but we need to signal to the downstream interceptor that it shouldn't interfere with the classloading
                 // While we're doing that, we may as well share the classloading logic
-                itClasses.add(facadeClassLoader.loadClass(i));
+                itClasses.add(testLoadingClassLoader.loadClass(i));
             } catch (Exception e) {
                 // TODO how handle this?
                 e.printStackTrace();

diff --git a/...loyment/dev/testing/TestClassIndexer.java → ...quarkus/test/common/TestClassIndexer.java b/...loyment/dev/testing/TestClassIndexer.java → ...quarkus/test/common/TestClassIndexer.java
@@ -1,4 +1,4 @@
-package io.quarkus.deployment.dev.testing;
+package io.quarkus.test.common;
 
 import static io.quarkus.test.common.PathTestHelper.getTestClassesLocation;
 
@@ -22,7 +22,6 @@
 import org.jboss.jandex.UnsupportedVersion;
 
 import io.quarkus.fs.util.ZipUtils;
-import io.quarkus.test.common.PathTestHelper;
 
 public final class TestClassIndexer {
 

diff --git a/test-framework/common/src/main/java/io/quarkus/test/common/TestResourceManager.java b/test-framework/common/src/main/java/io/quarkus/test/common/TestResourceManager.java
@@ -39,7 +39,6 @@
 import org.jboss.jandex.DotName;
 import org.jboss.jandex.IndexView;
 
-import io.quarkus.deployment.dev.testing.TestClassIndexer;
 import io.quarkus.deployment.dev.testing.TestStatus;
 
 /**

diff --git a/...framework/junit5/src/main/java/io/quarkus/test/junit/AbstractJvmQuarkusTestExtension.java b/...framework/junit5/src/main/java/io/quarkus/test/junit/AbstractJvmQuarkusTestExtension.java
@@ -42,10 +42,9 @@
 public class AbstractJvmQuarkusTestExtension extends AbstractQuarkusTestWithContextExtension
         implements ExecutionCondition {
 
-    // TODO it would be nicer to store these here, but cannot while some consumers are in the core module
-    protected static final String TEST_LOCATION = TestBuildChainFunction.TEST_LOCATION;
-    protected static final String TEST_CLASS = TestBuildChainFunction.TEST_CLASS;
-    protected static final String TEST_PROFILE = TestBuildChainFunction.TEST_PROFILE;
+    protected static final String TEST_LOCATION = "test-location";
+    protected static final String TEST_CLASS = "test-class";
+    protected static final String TEST_PROFILE = "test-profile";
 
     protected ClassLoader originalCl;
 

diff --git a/...eployment/dev/testing/AppMakerHelper.java → ...io/quarkus/test/junit/AppMakerHelper.java b/...eployment/dev/testing/AppMakerHelper.java → ...io/quarkus/test/junit/AppMakerHelper.java
@@ -1,4 +1,4 @@
-package io.quarkus.deployment.dev.testing;
+package io.quarkus.test.junit;
 
 import static io.quarkus.test.common.PathTestHelper.getAppClassLocationForTestLocation;
 import static io.quarkus.test.common.PathTestHelper.getTestClassesLocation;
@@ -34,12 +34,12 @@
 import io.quarkus.bootstrap.workspace.SourceDir;
 import io.quarkus.bootstrap.workspace.WorkspaceModule;
 import io.quarkus.commons.classloading.ClassLoaderHelper;
+import io.quarkus.deployment.dev.testing.ClassCoercingTestProfile;
 import io.quarkus.paths.PathList;
 import io.quarkus.runtime.LaunchMode;
 import io.quarkus.test.common.PathTestHelper;
 import io.quarkus.test.common.RestorableSystemProperties;
-import io.quarkus.test.junit.QuarkusTestProfile;
-import io.quarkus.test.junit.TestBuildChainFunction;
+import io.quarkus.test.common.TestClassIndexer;
 
 public class AppMakerHelper {
 

diff --git a/test-framework/junit5/src/main/java/io/quarkus/test/junit/IntegrationTestUtil.java b/test-framework/junit5/src/main/java/io/quarkus/test/junit/IntegrationTestUtil.java
@@ -44,13 +44,13 @@
 import io.quarkus.bootstrap.workspace.ArtifactSources;
 import io.quarkus.bootstrap.workspace.SourceDir;
 import io.quarkus.deployment.builditem.DevServicesLauncherConfigResultBuildItem;
-import io.quarkus.deployment.dev.testing.TestClassIndexer;
 import io.quarkus.deployment.util.ContainerRuntimeUtil;
 import io.quarkus.paths.PathList;
 import io.quarkus.runtime.LaunchMode;
 import io.quarkus.runtime.logging.LoggingSetupRecorder;
 import io.quarkus.test.common.ArtifactLauncher;
 import io.quarkus.test.common.PathTestHelper;
+import io.quarkus.test.common.TestClassIndexer;
 import io.quarkus.test.common.TestResourceManager;
 import io.quarkus.test.common.http.TestHTTPResourceManager;
 import io.smallrye.config.SmallRyeConfig;

diff --git a/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java b/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
@@ -78,7 +78,6 @@
 import io.quarkus.deployment.builditem.TestClassPredicateBuildItem;
 import io.quarkus.deployment.builditem.TestProfileBuildItem;
 import io.quarkus.deployment.dev.testing.DotNames;
-import io.quarkus.deployment.dev.testing.TestClassIndexer;
 import io.quarkus.dev.testing.ExceptionReporting;
 import io.quarkus.dev.testing.TracingHandler;
 import io.quarkus.runtime.ApplicationLifecycleManager;
@@ -92,6 +91,7 @@
 import io.quarkus.test.common.PropertyTestUtil;
 import io.quarkus.test.common.RestAssuredURLManager;
 import io.quarkus.test.common.RestorableSystemProperties;
+import io.quarkus.test.common.TestClassIndexer;
 import io.quarkus.test.common.TestResourceManager;
 import io.quarkus.test.common.TestScopeManager;
 import io.quarkus.test.common.http.TestHTTPEndpoint;

diff --git a/...us/test/junit/TestBuildChainFunction.java → ...us/test/junit/TestBuildChainFunction.java b/...us/test/junit/TestBuildChainFunction.java → ...us/test/junit/TestBuildChainFunction.java
@@ -26,23 +26,18 @@
 import io.quarkus.deployment.builditem.TestClassPredicateBuildItem;
 import io.quarkus.deployment.builditem.TestProfileBuildItem;
 import io.quarkus.deployment.dev.testing.DotNames;
-import io.quarkus.deployment.dev.testing.TestClassIndexer;
 import io.quarkus.test.common.PathTestHelper;
+import io.quarkus.test.common.TestClassIndexer;
 import io.quarkus.test.junit.buildchain.TestBuildChainCustomizerProducer;
 
-// TODO ideally this would live in the test-framework modules, but that needs the FacadeClassLoader to be over there, which needs the JUnitRunner to be over there.
 public class TestBuildChainFunction implements Function<Map<String, Object>, List<Consumer<BuildChainBuilder>>> {
 
-    protected static final String TEST_LOCATION = "test-location";
-    protected static final String TEST_CLASS = "test-class";
-    protected static final String TEST_PROFILE = "test-profile";
-
     @Override
     public List<Consumer<BuildChainBuilder>> apply(Map<String, Object> stringObjectMap) {
-        Path testLocation = (Path) stringObjectMap.get(TEST_LOCATION);
+        Path testLocation = (Path) stringObjectMap.get(AbstractJvmQuarkusTestExtension.TEST_LOCATION);
         // the index was written by the extension
         Index testClassesIndex = TestClassIndexer.readIndex(testLocation,
-                (Class<?>) stringObjectMap.get(TEST_CLASS));
+                (Class<?>) stringObjectMap.get(AbstractJvmQuarkusTestExtension.TEST_CLASS));
 
         List<Consumer<BuildChainBuilder>> allCustomizers = new ArrayList<>(1);
         Consumer<BuildChainBuilder> defaultCustomizer = new Consumer<BuildChainBuilder>() {
@@ -82,9 +77,7 @@ public boolean test(String className) {
                 buildChainBuilder.addBuildStep(new BuildStep() {
                     @Override
                     public void execute(BuildContext context) {
-                        // TODO ideally we would use the .class object, but we can't if we're in core
-                        // TODO should this be a dot name?
-                        context.produce(new TestAnnotationBuildItem("io.quarkus.test.junit.QuarkusTest")); // QuarkusTest.class.getName()));
+                        context.produce(new TestAnnotationBuildItem(QuarkusTest.class.getName()));
                     }
                 })
                         .produces(TestAnnotationBuildItem.class)
@@ -146,7 +139,7 @@ public void execute(BuildContext context) {
                 buildChainBuilder.addBuildStep(new BuildStep() {
                     @Override
                     public void execute(BuildContext context) {
-                        Object testProfile = stringObjectMap.get(TEST_PROFILE);
+                        Object testProfile = stringObjectMap.get(AbstractJvmQuarkusTestExtension.TEST_PROFILE);
                         if (testProfile != null) {
                             context.produce(new TestProfileBuildItem(testProfile.toString()));
                         }

diff --git a/...junit/classloading/FacadeClassLoader.java → ...junit/classloading/FacadeClassLoader.java b/...junit/classloading/FacadeClassLoader.java → ...junit/classloading/FacadeClassLoader.java
@@ -30,7 +30,10 @@
 import io.quarkus.bootstrap.app.CuratedApplication;
 import io.quarkus.bootstrap.app.StartupAction;
 import io.quarkus.bootstrap.classloading.QuarkusClassLoader;
-import io.quarkus.deployment.dev.testing.AppMakerHelper;
+import io.quarkus.test.junit.AppMakerHelper;
+import io.quarkus.test.junit.QuarkusIntegrationTest;
+import io.quarkus.test.junit.TestProfile;
+import io.quarkus.test.junit.TestResourceUtil;
 
 /**
  * JUnit has many interceptors and listeners, but it does not allow us to intercept test discovery in a fine-grained way that
@@ -48,6 +51,8 @@ public class FacadeClassLoader extends ClassLoader implements Closeable {
     private static final String NAME = "FacadeLoader";
     private static final String IO_QUARKUS_TEST_JUNIT_QUARKUS_TEST_EXTENSION = "io.quarkus.test.junit.QuarkusTestExtension";
     public static final String VALUE = "value";
+    public static final String KEY_PREFIX = "QuarkusTest-";
+    public static final String DISPLAY_NAME_PREFIX = "JUnit";
     // TODO it would be nice, and maybe theoretically possible, to re-use the curated application?
     // TODO and if we don't, how do we get a re-usable deployment classloader?
 
@@ -169,11 +174,10 @@ public FacadeClassLoader(ClassLoader parent, boolean isAuxiliaryApplication, Map
                     .loadClass(RegisterExtension.class.getName());
             quarkusTestAnnotation = (Class<? extends Annotation>) annotationLoader
                     .loadClass("io.quarkus.test.junit.QuarkusTest");
-            // TODO if this was in the right module, could use class getname
             quarkusIntegrationTestAnnotation = (Class<? extends Annotation>) annotationLoader
-                    .loadClass("io.quarkus.test.junit.QuarkusIntegrationTest");
+                    .loadClass(QuarkusIntegrationTest.class.getName());
             profileAnnotation = (Class<? extends Annotation>) annotationLoader
-                    .loadClass("io.quarkus.test.junit.TestProfile");
+                    .loadClass(TestProfile.class.getName());
         } catch (ClassNotFoundException e) {
             // If QuarkusTest is not on the classpath, that's fine; it just means we definitely won't have QuarkusTests. That means we can bypass a whole bunch of logic.
             log.debug("Could not load annotations for FacadeClassLoader: " + e);
@@ -349,7 +353,7 @@ private boolean registersQuarkusTestExtensionOnField(Class<?> inspectionClass) {
 
     private QuarkusClassLoader getQuarkusClassLoader(Class requiredTestClass, Class<?> profile) {
         final String profileName = profile != null ? profile.getName() : NO_PROFILE;
-        String profileKey = "QuarkusTest" + "-" + profileName;
+        String profileKey = KEY_PREFIX + profileName;
 
         try {
             StartupAction startupAction;
@@ -378,8 +382,6 @@ private QuarkusClassLoader getQuarkusClassLoader(Class requiredTestClass, Class<
                 key = profileKey + resourceKey;
                 startupAction = runtimeClassLoaders.get(key);
                 if (startupAction == null) {
-                    // TODO can we make this less confusing?
-
                     // Making a classloader uses the profile key to look up a curated application
                     startupAction = makeClassLoader(profileKey, requiredTestClass, profile);
                 }
@@ -406,8 +408,10 @@ private String getResourceKey(Class<?> requiredTestClass, Class profile)
         String resourceKey;
 
         ClassLoader classLoader = keyMakerClassLoader;
+        // We have to access TestResourceUtil reflectively, because if we used this class's classloader, it might be an augmentation classloader without access to application classes
+        // TODO check this is true, try skipping reflection and also using the peeking loader
         Method method = Class
-                .forName("io.quarkus.test.junit.TestResourceUtil", true, classLoader) // TODO use class, not string, but that would need us to be in a different module
+                .forName(TestResourceUtil.class.getName(), true, classLoader)
                 .getMethod("getReloadGroupIdentifier", Class.class, Class.class);
 
         ClassLoader original = Thread.currentThread()
@@ -436,7 +440,7 @@ private StartupAction makeClassLoader(String key, Class requiredTestClass, Class
         if (curatedApplication == null) {
             Collection<Runnable> shutdownTasks = new HashSet();
 
-            String displayName = "JUnit" + key; // TODO come up with a good display name
+            String displayName = DISPLAY_NAME_PREFIX + key;
             curatedApplication = appMakerHelper.makeCuratedApplication(requiredTestClass, displayName,
                     isAuxiliaryApplication,
                     shutdownTasks);

diff --git a/...lassloading/ParentLastURLClassLoader.java → ...lassloading/ParentLastURLClassLoader.java b/...lassloading/ParentLastURLClassLoader.java → ...lassloading/ParentLastURLClassLoader.java