only import necessary classes when discovering tests from classpath

The JUnit platform API makes it necessary for ArchUnit to provide an implementation to discover test classes from the classpath (e.g. because the discovery request contains a package selector). Since ArchUnit itself is perfectly suited for this and does not demand any third party dependency we simply use the `ClassFileImporter` for this. However, so far we have used the current `ArchConfiguration` of the user, which by default e.g. resolves all further dependencies from the classpath. This is unnecessary overhead and can substantially slow down the discovery process if the test classes e.g. have some framework dependencies like the Spring framework.
We will now switch the `ArchConfiguration` for the discovery process. However, the global nature of `ArchConfiguration` does not make this completely trivial. E.g. if the user has another import in progress in another thread we do not want to affect this import by suddenly switching the user's configuration in the middle. To isolate this configuration change I have added a possibility to override the `ArchConfiguration` thread locally. I.e. the discovery process will only affect the current thread when changing the configuration and only during the scope of a fixed lambda. Afterwards the thread local configuration is cleaned up. This way we should never affect any other import of a user, since the scope of configuration change is tailored exactly to the discovery process and cleaned up afterwards.

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>

archunit-junit/junit5/engine/build.gradle

@@ -18,6 +18,8 @@ dependencies {
     testImplementation dependency.assertj
     testImplementation dependency.mockito
     testImplementation dependency.junit5JupiterApi
+    testImplementation dependency.log4j_core
+    testImplementation dependency.log4j_slf4j
 
     testRuntimeOnly dependency.junit5JupiterEngine
 }

archunit-junit/junit5/engine/src/main/java/com/tngtech/archunit/junit/internal/ArchUnitTestEngine.java

@@ -20,13 +20,17 @@
 import java.net.URL;
 import java.util.List;
 import java.util.Optional;
+import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
 
+import com.tngtech.archunit.ArchConfiguration;
 import com.tngtech.archunit.Internal;
 import com.tngtech.archunit.base.MayResolveTypesViaReflection;
 import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaClasses;
 import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.resolvers.ClassResolver;
 import com.tngtech.archunit.junit.AnalyzeClasses;
 import com.tngtech.archunit.junit.ArchTest;
 import com.tngtech.archunit.junit.engine_api.FieldSelector;
@@ -144,11 +148,21 @@ private void resolveRequestedUniqueIds(EngineDiscoveryRequest discoveryRequest,
     }
 
     private Stream<JavaClass> getContainedClasses(String[] packages) {
-        return new ClassFileImporter().importPackages(packages).stream();
+        if (packages.length == 0) {
+            return Stream.empty();
+        }
+        return discoverClasses(importer -> importer.importPackages(packages));
     }
 
     private Stream<JavaClass> getContainedClasses(ClasspathRootSelector selector) {
-        return new ClassFileImporter().importUrl(toUrl(selector.getClasspathRoot())).stream();
+        return discoverClasses(importer -> importer.importUrl(toUrl(selector.getClasspathRoot())));
+    }
+
+    private Stream<JavaClass> discoverClasses(Function<ClassFileImporter, JavaClasses> importClasses) {
+        return ArchConfiguration.withThreadLocalScope(configuration -> {
+            configuration.setClassResolver(NoOpClassResolver.class);
+            return importClasses.apply(new ClassFileImporter()).stream();
+        });
     }
 
     private Predicate<JavaClass> isAllowedBy(EngineDiscoveryRequest discoveryRequest) {
@@ -203,4 +217,15 @@ ClassCache get() {
             return cache;
         }
     }
+
+    static class NoOpClassResolver implements ClassResolver {
+        @Override
+        public void setClassUriImporter(ClassUriImporter classUriImporter) {
+        }
+
+        @Override
+        public Optional<JavaClass> tryResolve(String typeName) {
+            return Optional.empty();
+        }
+    }
 }

archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/internal/ArchUnitTestEngineTest.java

@@ -14,6 +14,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.tngtech.archunit.ArchConfiguration;
 import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
 import com.tngtech.archunit.junit.AnalyzeClasses;
 import com.tngtech.archunit.junit.ArchTest;
 import com.tngtech.archunit.junit.engine_api.FieldSelector;
@@ -49,7 +50,10 @@
 import com.tngtech.archunit.junit.internal.testexamples.subtwo.SimpleRules;
 import com.tngtech.archunit.junit.internal.testexamples.wrong.WrongRuleMethodNotStatic;
 import com.tngtech.archunit.junit.internal.testexamples.wrong.WrongRuleMethodWrongParameters;
+import com.tngtech.archunit.junit.internal.testutil.LogCaptor;
 import com.tngtech.archunit.junit.internal.testutil.MockitoExtension;
+import com.tngtech.archunit.junit.internal.testutil.TestLogExtension;
+import com.tngtech.archunit.testutil.TestLogRecorder.RecordedLogEvent;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
@@ -89,6 +93,7 @@
 import static java.util.function.Function.identity;
 import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
+import static org.apache.logging.log4j.Level.DEBUG;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.platform.engine.TestDescriptor.Type.CONTAINER;
@@ -729,13 +734,7 @@ void all_without_filters() {
 
         @Test
         void without_importing_classes_already() {
-            EngineDiscoveryTestRequest discoveryRequest = new EngineDiscoveryTestRequest()
-                    .withClass(SimpleRuleField.class)
-                    .withClasspathRoot(uriOfClass(SimpleRuleField.class))
-                    .withField(SimpleRuleField.class, SimpleRuleField.SIMPLE_RULE_FIELD_NAME)
-                    .withPackage(SimpleRuleField.class.getPackage().getName())
-                    .withMethod(SimpleRuleMethod.class, SimpleRuleMethod.SIMPLE_RULE_METHOD_NAME)
-                    .withUniqueId(simpleRulesId(engineId).append(FIELD_SEGMENT_TYPE, SimpleRules.SIMPLE_RULE_FIELD_ONE_NAME));
+            EngineDiscoveryTestRequest discoveryRequest = discoveryRequestUsingAllMethodsOfDiscovery();
 
             doThrow(new AssertionError("The cache should not be queried during discovery, "
                     + "otherwise classes will be imported when tests are possibly skipped afterwards"))
@@ -746,6 +745,39 @@ void without_importing_classes_already() {
             assertThat(rootDescriptor).isNotNull();
         }
 
+        @Test
+        @ExtendWith(TestLogExtension.class)
+        void without_scanning_unnecessary_classes_for_tests(LogCaptor logCaptor) {
+            EngineDiscoveryTestRequest discoveryRequest = discoveryRequestUsingAllMethodsOfDiscovery();
+
+            logCaptor.watch(ClassFileImporter.class, DEBUG);
+            testEngine.discover(discoveryRequest, engineId);
+
+            // since we discover the classes from the classpath with ArchUnit we expect DEBUG log entries for the test classes
+            assertThat(logCaptor.getEvents(DEBUG).stream())
+                    .extracting(RecordedLogEvent::getMessage)
+                    .as("messages of log events with level " + DEBUG)
+                    .anySatisfy(message -> assertThat(message)
+                            .matches(".*Processing class '.*" + SimpleRuleField.class.getSimpleName() + "'"));
+
+            // but we do not want to transitively resolve dependencies of those classes like java.lang.Object
+            assertThat(logCaptor.getEvents(DEBUG).stream())
+                    .extracting(RecordedLogEvent::getMessage)
+                    .as("messages of log events with level " + DEBUG)
+                    .noneSatisfy(message -> assertThat(message)
+                            .matches(".*Processing class '.*" + Object.class.getSimpleName() + "'"));
+        }
+
+        private EngineDiscoveryTestRequest discoveryRequestUsingAllMethodsOfDiscovery() {
+            return new EngineDiscoveryTestRequest()
+                    .withClass(SimpleRuleField.class)
+                    .withClasspathRoot(uriOfClass(SimpleRuleField.class))
+                    .withField(SimpleRuleField.class, SimpleRuleField.SIMPLE_RULE_FIELD_NAME)
+                    .withPackage(SimpleRuleField.class.getPackage().getName())
+                    .withMethod(SimpleRuleMethod.class, SimpleRuleMethod.SIMPLE_RULE_METHOD_NAME)
+                    .withUniqueId(simpleRulesId(engineId).append(FIELD_SEGMENT_TYPE, SimpleRules.SIMPLE_RULE_FIELD_ONE_NAME));
+        }
+
         private Set<TestTag> getTagsForIdEndingIn(String suffix, Map<UniqueId, Set<TestTag>> tagsById) {
             UniqueId matchingId = tagsById.keySet().stream()
                     .filter(id -> getLast(id.getSegments()).getValue().endsWith(suffix))

archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/internal/testutil/LogCaptor.java

@@ -0,0 +1,23 @@
+package com.tngtech.archunit.junit.internal.testutil;
+
+import java.util.List;
+
+import com.tngtech.archunit.testutil.TestLogRecorder;
+import com.tngtech.archunit.testutil.TestLogRecorder.RecordedLogEvent;
+import org.apache.logging.log4j.Level;
+
+public class LogCaptor {
+    private final TestLogRecorder logRecorder = new TestLogRecorder();
+
+    public void watch(Class<?> loggerClass, Level level) {
+        logRecorder.record(loggerClass, level);
+    }
+
+    public void cleanUp() {
+        logRecorder.reset();
+    }
+
+    public List<RecordedLogEvent> getEvents(Level level) {
+        return logRecorder.getEvents(level);
+    }
+}

archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/internal/testutil/TestLogExtension.java

@@ -0,0 +1,26 @@
+package com.tngtech.archunit.junit.internal.testutil;
+
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.extension.ParameterContext;
+import org.junit.jupiter.api.extension.ParameterResolutionException;
+import org.junit.jupiter.api.extension.ParameterResolver;
+
+public class TestLogExtension implements ParameterResolver, AfterEachCallback {
+    private final LogCaptor logCaptor = new LogCaptor();
+
+    @Override
+    public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
+        return LogCaptor.class.equals(parameterContext.getParameter().getType());
+    }
+
+    @Override
+    public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
+        return logCaptor;
+    }
+
+    @Override
+    public void afterEach(ExtensionContext context) {
+        logCaptor.cleanUp();
+    }
+}

archunit/src/main/java/com/tngtech/archunit/ArchConfiguration.java

@@ -24,6 +24,8 @@
 import java.util.Optional;
 import java.util.Properties;
 import java.util.Set;
+import java.util.function.Consumer;
+import java.util.function.Function;
 import java.util.function.Supplier;
 
 import com.google.common.base.Joiner;
@@ -55,31 +57,36 @@ public final class ArchConfiguration {
     private static final Logger LOG = LoggerFactory.getLogger(ArchConfiguration.class);
 
     private static final Supplier<ArchConfiguration> INSTANCE = Suppliers.memoize(ArchConfiguration::new);
+    private static final ThreadLocal<ArchConfiguration> threadLocalConfiguration = new ThreadLocal<>();
 
     @PublicAPI(usage = ACCESS)
     public static ArchConfiguration get() {
-        return INSTANCE.get();
+        return threadLocalConfiguration.get() != null ? threadLocalConfiguration.get() : INSTANCE.get();
     }
 
     private final String propertiesResourceName;
-    private final PropertiesOverwritableBySystemProperties properties = new PropertiesOverwritableBySystemProperties();
+    private PropertiesOverwritableBySystemProperties properties;
 
     private ArchConfiguration() {
         this(ARCHUNIT_PROPERTIES_RESOURCE_NAME);
     }
 
     private ArchConfiguration(String propertiesResourceName) {
+        this(propertiesResourceName, readProperties(propertiesResourceName));
+    }
+
+    private ArchConfiguration(String propertiesResourceName, PropertiesOverwritableBySystemProperties properties) {
         this.propertiesResourceName = propertiesResourceName;
-        readProperties(propertiesResourceName);
+        this.properties = properties;
     }
 
-    private void readProperties(String propertiesResourceName) {
-        properties.clear();
+    private static PropertiesOverwritableBySystemProperties readProperties(String propertiesResourceName) {
+        PropertiesOverwritableBySystemProperties properties = new PropertiesOverwritableBySystemProperties();
 
-        URL archUnitPropertiesUrl = getCurrentClassLoader(getClass()).getResource(propertiesResourceName);
+        URL archUnitPropertiesUrl = getCurrentClassLoader(ArchConfiguration.class).getResource(propertiesResourceName);
         if (archUnitPropertiesUrl == null) {
             LOG.debug("No configuration found in classpath at {} => Using default configuration", propertiesResourceName);
-            return;
+            return properties;
         }
 
         try (InputStream inputStream = archUnitPropertiesUrl.openStream()) {
@@ -88,11 +95,12 @@ private void readProperties(String propertiesResourceName) {
         } catch (IOException e) {
             LOG.warn("Error reading ArchUnit properties from " + archUnitPropertiesUrl, e);
         }
+        return properties;
     }
 
     @PublicAPI(usage = ACCESS)
     public void reset() {
-        readProperties(propertiesResourceName);
+        properties = readProperties(propertiesResourceName);
     }
 
     @PublicAPI(usage = ACCESS)
@@ -262,18 +270,76 @@ public String getPropertyOrDefault(String propertyName, String defaultValue) {
         return properties.getProperty(propertyName, defaultValue);
     }
 
+    /**
+     * Same as {@link #withThreadLocalScope(Function)} but does not return a value.
+     */
+    @PublicAPI(usage = ACCESS)
+    public static void withThreadLocalScope(Consumer<ArchConfiguration> doWithThreadLocalConfiguration) {
+        withThreadLocalScope(configuration -> {
+            doWithThreadLocalConfiguration.accept(configuration);
+            return null;
+        });
+    }
+
+    /**
+     * Sets up a thread local copy of the current {@link ArchConfiguration} to be freely modified.
+     * Within the current thread and the scope of {@code doWithThreadLocalConfiguration}
+     * {@link ArchConfiguration#get() ArchConfiguration.get()} will return the thread local configuration,
+     * i.e. adjustments to the configuration passed to {@code doWithThreadLocalConfiguration} will be
+     * picked up by ArchUnit while executing from this thread within the scope of
+     * {@code doWithThreadLocalConfiguration}.<br><br>
+     * For example:
+     *
+     * <pre><code>
+     * ArchConfiguration.get().setResolveMissingDependenciesFromClassPath(true);
+     *
+     * JavaClasses classesWithoutResolvingFromClasspath =
+     *   ArchConfiguration.withThreadLocalScope((ArchConfiguration configuration) -> {
+     *     configuration.setResolveMissingDependenciesFromClassPath(false);
+     *     return new ClassFileImporter().importPackages(..) // will now not resolve from classpath
+     *   });
+     *
+     * JavaClasses classesWithResolvingFromClasspath =
+     *   new ClassFileImporter().importPackages(..) // will now see the original value and resolve from classpath
+     * </code></pre>
+     *
+     * @param doWithThreadLocalConfiguration A lambda that allows to execute code that will see the thread
+     *                                       local {@link ArchConfiguration} instead of the global one. Once
+     *                                       the lambda has been executed the thread local configuration
+     *                                       is cleaned up and all threads will see the global configuration
+     *                                       again.
+     */
+    @PublicAPI(usage = ACCESS)
+    public static <T> T withThreadLocalScope(Function<ArchConfiguration, T> doWithThreadLocalConfiguration) {
+        ArchConfiguration configuration = INSTANCE.get().copy();
+        ArchConfiguration.threadLocalConfiguration.set(configuration);
+        try {
+            return doWithThreadLocalConfiguration.apply(configuration);
+        } finally {
+            ArchConfiguration.threadLocalConfiguration.set(null);
+        }
+    }
+
+    private ArchConfiguration copy() {
+        return new ArchConfiguration(propertiesResourceName, properties.copy());
+    }
+
     private static class PropertiesOverwritableBySystemProperties {
         private static final Properties PROPERTY_DEFAULTS = createProperties(ImmutableMap.of(
                 RESOLVE_MISSING_DEPENDENCIES_FROM_CLASS_PATH, Boolean.TRUE.toString(),
                 ENABLE_MD5_IN_CLASS_SOURCES, Boolean.FALSE.toString()
         ));
 
-        private final Properties baseProperties = createProperties(PROPERTY_DEFAULTS);
-        private final Properties overwrittenProperties = new Properties();
+        private final Properties baseProperties;
+        private final Properties overwrittenProperties;
 
-        void clear() {
-            replaceProperties(baseProperties, PROPERTY_DEFAULTS);
-            overwrittenProperties.clear();
+        PropertiesOverwritableBySystemProperties() {
+            this(createProperties(PROPERTY_DEFAULTS), new Properties());
+        }
+
+        PropertiesOverwritableBySystemProperties(Properties baseProperties, Properties overwrittenProperties) {
+            this.baseProperties = baseProperties;
+            this.overwrittenProperties = overwrittenProperties;
         }
 
         void load(InputStream inputStream) throws IOException {
@@ -329,6 +395,14 @@ private static Properties createProperties(Map<?, ?> entries) {
             result.putAll(entries);
             return result;
         }
+
+        PropertiesOverwritableBySystemProperties copy() {
+            return new PropertiesOverwritableBySystemProperties(copy(baseProperties), copy(overwrittenProperties));
+        }
+
+        private Properties copy(Properties properties) {
+            return (Properties) properties.clone();
+        }
     }
 
     public final class ExtensionProperties {