From ac2970059076100ca43220f9c2c14b4da5d4dd0e Mon Sep 17 00:00:00 2001 From: Peter Gafert Date: Sun, 6 Mar 2022 16:24:03 +0700 Subject: [PATCH] make `@AnalyzeClasses` import package of test class by default In particular coming from a Spring framework background it is a somewhat surprising behavior that a plain `@AnalyzeClasses` without any options will import the whole classpath. The more intuitive behavior would be to import the package of the annotated class. Since importing the whole classpath is likely not what most users want, and the performance drain is large, we will change the behavior to import the package of the annotated class by default if no further locations are specified. However, this might break some existing use cases where scanning the whole classpath in combination with `ImportOptions` was used consciously. I.e. something like ``` @AnalyzeClasses(importOptions = MySpecialFilter.class) ``` To make sure ArchUnit can still satisfy this usecase we introduce a new property `@AnalyzeClasses(wholeClasspath = true)`, which is `false` by default, but can be set to `true` to restore the old behavior. Signed-off-by: Peter Gafert --- archunit-junit/build.gradle | 4 ++ archunit-junit/junit4/build.gradle | 2 +- .../archunit/junit/AnalyzeClasses.java | 6 +++ .../archunit/junit/ArchUnitRunner.java | 5 ++ .../archunit/junit/ArchUnitRunnerTest.java | 4 +- .../archunit/junit/AnalyzeClasses.java | 6 +++ .../junit/ArchUnitTestDescriptor.java | 5 ++ .../junit/ArchUnitTestEngineTest.java | 1 + .../testexamples/FullAnalyzeClassesSpec.java | 1 + .../archunit/junit/ClassAnalysisRequest.java | 2 + .../tngtech/archunit/junit/ClassCache.java | 31 +++++++----- .../archunit/junit/ClassCacheTest.java | 48 ++++++++++++++++++- .../archunit/junit/TestAnalysisRequest.java | 11 +++++ 13 files changed, 109 insertions(+), 17 deletions(-) diff --git a/archunit-junit/build.gradle b/archunit-junit/build.gradle index 11c012e132..663fad3ff1 100644 --- a/archunit-junit/build.gradle +++ b/archunit-junit/build.gradle @@ -50,6 +50,10 @@ dependencies { exclude module: 'guava' } testImplementation project(path: ':archunit', configuration: 'tests') + + // This is a hack for running tests with IntelliJ instead of delegating to Gradle, + // because for some reason this dependency cannot be resolved otherwise :-( + testRuntimeOnly dependency.asm } archUnitTest { diff --git a/archunit-junit/junit4/build.gradle b/archunit-junit/junit4/build.gradle index 80b51f0bc8..894575fac4 100644 --- a/archunit-junit/junit4/build.gradle +++ b/archunit-junit/junit4/build.gradle @@ -27,7 +27,7 @@ dependencies { testImplementation project(path: ':archunit-junit', configuration: 'tests') // This is a hack for running tests with IntelliJ instead of delegating to Gradle, - // because for some reason this dependency cannot be resolved otherwise only for archunit-junit4 :-( + // because for some reason this dependency cannot be resolved otherwise :-( testRuntimeOnly dependency.asm } diff --git a/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java b/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java index fa5b79d5e9..c90d7d4e82 100644 --- a/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java +++ b/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java @@ -62,6 +62,12 @@ */ Class[] locations() default {}; + /** + * @return Whether to import all classes on the classpath. + * Warning: Without any further filtering this can require a lot of resources. + */ + boolean wholeClasspath() default false; + /** * Allows to filter the class import. The supplied types will be instantiated and used to create the * {@link ImportOptions} passed to the {@link ClassFileImporter}. Considering caching, compare the notes on diff --git a/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/ArchUnitRunner.java b/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/ArchUnitRunner.java index f97f64d7a9..730d152c68 100644 --- a/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/ArchUnitRunner.java +++ b/archunit-junit/junit4/src/main/java/com/tngtech/archunit/junit/ArchUnitRunner.java @@ -217,5 +217,10 @@ public Class[] getImportOptions() { public CacheMode getCacheMode() { return analyzeClasses.cacheMode(); } + + @Override + public boolean scanWholeClasspath() { + return analyzeClasses.wholeClasspath(); + } } } diff --git a/archunit-junit/junit4/src/test/java/com/tngtech/archunit/junit/ArchUnitRunnerTest.java b/archunit-junit/junit4/src/test/java/com/tngtech/archunit/junit/ArchUnitRunnerTest.java index a36f41258f..2970de12bd 100644 --- a/archunit-junit/junit4/src/test/java/com/tngtech/archunit/junit/ArchUnitRunnerTest.java +++ b/archunit-junit/junit4/src/test/java/com/tngtech/archunit/junit/ArchUnitRunnerTest.java @@ -66,6 +66,7 @@ public void runner_creates_correct_analysis_request() { assertThat(analysisRequest.getPackageNames()).isEqualTo(analyzeClasses.packages()); assertThat(analysisRequest.getPackageRoots()).isEqualTo(analyzeClasses.packagesOf()); assertThat(analysisRequest.getLocationProviders()).isEqualTo(analyzeClasses.locations()); + assertThat(analysisRequest.scanWholeClasspath()).as("scan whole classpath").isTrue(); assertThat(analysisRequest.getImportOptions()).isEqualTo(analyzeClasses.importOptions()); } @@ -149,6 +150,7 @@ public boolean includes(Location location) { packages = {"com.foo", "com.bar"}, packagesOf = {ArchUnitRunner.class, ArchUnitRunnerTest.class}, locations = {DummyLocation.class, OtherDummyLocation.class}, + wholeClasspath = true, importOptions = {DummyImportOption.class, OtherDummyImportOption.class} ) public static class MaxAnnotatedTest { @@ -156,4 +158,4 @@ public static class MaxAnnotatedTest { public static void someTest(JavaClasses classes) { } } -} \ No newline at end of file +} diff --git a/archunit-junit/junit5/api/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java b/archunit-junit/junit5/api/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java index cbfd0bb9a0..b0e65ef6e6 100644 --- a/archunit-junit/junit5/api/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java +++ b/archunit-junit/junit5/api/src/main/java/com/tngtech/archunit/junit/AnalyzeClasses.java @@ -66,6 +66,12 @@ */ Class[] locations() default {}; + /** + * @return Whether to look for classes on the whole classpath. + * Warning: Without any further {@link #importOptions() filtering} this can require a lot of resources. + */ + boolean wholeClasspath() default false; + /** * Allows to filter the class import. The supplied types will be instantiated and used to create the * {@link ImportOptions} passed to the {@link ClassFileImporter}. Considering caching, compare the notes on diff --git a/archunit-junit/junit5/engine/src/main/java/com/tngtech/archunit/junit/ArchUnitTestDescriptor.java b/archunit-junit/junit5/engine/src/main/java/com/tngtech/archunit/junit/ArchUnitTestDescriptor.java index aaf7fc4b14..d3808f1b8a 100644 --- a/archunit-junit/junit5/engine/src/main/java/com/tngtech/archunit/junit/ArchUnitTestDescriptor.java +++ b/archunit-junit/junit5/engine/src/main/java/com/tngtech/archunit/junit/ArchUnitTestDescriptor.java @@ -289,5 +289,10 @@ public Class[] getImportOptions() { public CacheMode getCacheMode() { return analyzeClasses.cacheMode(); } + + @Override + public boolean scanWholeClasspath() { + return analyzeClasses.wholeClasspath(); + } } } diff --git a/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/ArchUnitTestEngineTest.java b/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/ArchUnitTestEngineTest.java index 4954c08eab..e2f0356106 100644 --- a/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/ArchUnitTestEngineTest.java +++ b/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/ArchUnitTestEngineTest.java @@ -879,6 +879,7 @@ void passes_AnalyzeClasses_to_cache() { assertThat(request.getPackageNames()).isEqualTo(expected.packages()); assertThat(request.getPackageRoots()).isEqualTo(expected.packagesOf()); assertThat(request.getLocationProviders()).isEqualTo(expected.locations()); + assertThat(request.scanWholeClasspath()).as("scan whole classpath").isTrue(); assertThat(request.getImportOptions()).isEqualTo(expected.importOptions()); } diff --git a/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/testexamples/FullAnalyzeClassesSpec.java b/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/testexamples/FullAnalyzeClassesSpec.java index c2775a99af..2d0e81323a 100644 --- a/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/testexamples/FullAnalyzeClassesSpec.java +++ b/archunit-junit/junit5/engine/src/test/java/com/tngtech/archunit/junit/testexamples/FullAnalyzeClassesSpec.java @@ -23,6 +23,7 @@ packages = {"first.pkg", "second.pkg"}, packagesOf = {Object.class, File.class}, locations = {FirstLocationProvider.class, SecondLocationProvider.class}, + wholeClasspath = true, importOptions = {DoNotIncludeTests.class, DoNotIncludeJars.class}) public class FullAnalyzeClassesSpec { @ArchTest diff --git a/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassAnalysisRequest.java b/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassAnalysisRequest.java index fde2e83624..1b9ff0ca6b 100644 --- a/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassAnalysisRequest.java +++ b/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassAnalysisRequest.java @@ -15,4 +15,6 @@ interface ClassAnalysisRequest { Class[] getImportOptions(); CacheMode getCacheMode(); + + boolean scanWholeClasspath(); } diff --git a/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassCache.java b/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassCache.java index 03882dd018..000ec4ce7a 100644 --- a/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassCache.java +++ b/archunit-junit/src/main/java/com/tngtech/archunit/junit/ClassCache.java @@ -16,6 +16,7 @@ package com.tngtech.archunit.junit; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.Map; import java.util.Objects; @@ -154,32 +155,35 @@ private abstract static class RequestedLocations { abstract LocationsKey asKey(); - private static class All extends RequestedLocations { - private Class[] importOptions; + private static class AllInTestPackage extends RequestedLocations { + private final Class[] importOptions; + private final Set locationsOfTestPackage; - private All(Class[] importOptions) { + private AllInTestPackage(Class[] importOptions, Class testClass) { this.importOptions = importOptions; + locationsOfTestPackage = Locations.ofPackage(testClass.getPackage().getName()); } @Override LocationsKey asKey() { - return new LocationsKey(importOptions, Locations.inClassPath()); + return new LocationsKey(importOptions, locationsOfTestPackage); } } private static class Specific extends RequestedLocations { - private final ClassAnalysisRequest classAnalysisRequest; + private final Class[] importOptions; private final Set declaredLocations; private Specific(ClassAnalysisRequest classAnalysisRequest, Class testClass) { - this.classAnalysisRequest = classAnalysisRequest; + importOptions = classAnalysisRequest.getImportOptions(); declaredLocations = ImmutableSet.builder() - .addAll(getLocationsOfPackages()) - .addAll(getLocationsOfProviders(testClass)) + .addAll(getLocationsOfPackages(classAnalysisRequest)) + .addAll(getLocationsOfProviders(classAnalysisRequest, testClass)) + .addAll(classAnalysisRequest.scanWholeClasspath() ? Locations.inClassPath() : Collections.emptySet()) .build(); } - private Set getLocationsOfPackages() { + private Set getLocationsOfPackages(ClassAnalysisRequest classAnalysisRequest) { Set packages = ImmutableSet.builder() .add(classAnalysisRequest.getPackageNames()) .addAll(toPackageStrings(classAnalysisRequest.getPackageRoots())) @@ -187,7 +191,7 @@ private Set getLocationsOfPackages() { return locationsOf(packages); } - private Set getLocationsOfProviders(Class testClass) { + private Set getLocationsOfProviders(ClassAnalysisRequest classAnalysisRequest, Class testClass) { Set result = new HashSet<>(); for (Class providerClass : classAnalysisRequest.getLocationProviders()) { result.addAll(tryCreate(providerClass).get(testClass)); @@ -224,20 +228,21 @@ private Set locationsOf(Set packages) { @Override LocationsKey asKey() { - return new LocationsKey(classAnalysisRequest.getImportOptions(), declaredLocations); + return new LocationsKey(importOptions, declaredLocations); } } public static RequestedLocations by(ClassAnalysisRequest classAnalysisRequest, Class testClass) { return noSpecificLocationRequested(classAnalysisRequest) ? - new All(classAnalysisRequest.getImportOptions()) : + new AllInTestPackage(classAnalysisRequest.getImportOptions(), testClass) : new Specific(classAnalysisRequest, testClass); } private static boolean noSpecificLocationRequested(ClassAnalysisRequest classAnalysisRequest) { return classAnalysisRequest.getPackageNames().length == 0 && classAnalysisRequest.getPackageRoots().length == 0 - && classAnalysisRequest.getLocationProviders().length == 0; + && classAnalysisRequest.getLocationProviders().length == 0 + && !classAnalysisRequest.scanWholeClasspath(); } } } diff --git a/archunit-junit/src/test/java/com/tngtech/archunit/junit/ClassCacheTest.java b/archunit-junit/src/test/java/com/tngtech/archunit/junit/ClassCacheTest.java index 5a9a42a281..e3ae9a353c 100644 --- a/archunit-junit/src/test/java/com/tngtech/archunit/junit/ClassCacheTest.java +++ b/archunit-junit/src/test/java/com/tngtech/archunit/junit/ClassCacheTest.java @@ -6,6 +6,7 @@ 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.ImportOption; import com.tngtech.archunit.core.importer.ImportOptions; import com.tngtech.archunit.core.importer.Location; @@ -15,6 +16,7 @@ import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import org.assertj.core.api.Condition; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -32,6 +34,7 @@ import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -125,7 +128,6 @@ public void get_all_classes_by_LocationProvider() { @Test public void rejects_LocationProviders_without_default_constructor() { - thrown.expect(ArchTestExecutionException.class); thrown.expectMessage("public default constructor"); thrown.expectMessage(LocationProvider.class.getSimpleName()); @@ -134,10 +136,38 @@ public void rejects_LocationProviders_without_default_constructor() { analyzeLocation(WrongLocationProviderWithConstructorParam.class)); } + @Test + public void if_no_import_locations_are_specified_and_whole_classpath_is_set_false_then_the_default_is_the_package_of_the_test_class() { + TestAnalysisRequest defaultOptions = new TestAnalysisRequest().withWholeClasspath(false); + + JavaClasses classes = cache.getClassesToAnalyzeFor(TestClass.class, defaultOptions); + + assertThatTypes(classes).contain(getClass(), TestAnalysisRequest.class); + assertThatTypes(classes).doNotContain(ClassFileImporter.class); + } + + @Test + public void if_whole_classpath_is_set_true_then_the_whole_classpath_is_imported() { + TestAnalysisRequest defaultOptions = new TestAnalysisRequest().withWholeClasspath(true); + Class[] expectedImportResult = new Class[]{getClass()}; + doReturn(new ClassFileImporter().importClasses(expectedImportResult)) + .when(cacheClassFileImporter).importClasses(any(ImportOptions.class), ArgumentMatchers.anyCollection()); + + JavaClasses classes = cache.getClassesToAnalyzeFor(TestClass.class, defaultOptions); + + assertThatTypes(classes).matchExactly(expectedImportResult); + verify(cacheClassFileImporter).importClasses(any(ImportOptions.class), locationCaptor.capture()); + assertThat(locationCaptor.getValue()) + .has(locationContaining("archunit")) + .has(locationContaining("asm")) + .has(locationContaining("google")) + .has(locationContaining("mockito")); + } + @Test public void filters_urls() { JavaClasses classes = cache.getClassesToAnalyzeFor(TestClass.class, - new TestAnalysisRequest().withImportOptions(TestFilterForJUnitJars.class)); + new TestAnalysisRequest().withImportOptions(TestFilterForJUnitJars.class).withWholeClasspath(true)); assertThat(classes).isNotEmpty(); for (JavaClass clazz : classes) { @@ -218,6 +248,20 @@ private void verifyNumberOfImports(int number) { verifyNoMoreInteractions(cacheClassFileImporter); } + private static Condition> locationContaining(final String part) { + return new Condition>() { + @Override + public boolean matches(Iterable locations) { + for (Location location : locations) { + if (location.contains(part)) { + return true; + } + } + return false; + } + }; + } + public static class TestClass { } diff --git a/archunit-junit/src/test/java/com/tngtech/archunit/junit/TestAnalysisRequest.java b/archunit-junit/src/test/java/com/tngtech/archunit/junit/TestAnalysisRequest.java index 383fb012b2..92f4f4cbc3 100644 --- a/archunit-junit/src/test/java/com/tngtech/archunit/junit/TestAnalysisRequest.java +++ b/archunit-junit/src/test/java/com/tngtech/archunit/junit/TestAnalysisRequest.java @@ -7,6 +7,7 @@ class TestAnalysisRequest implements ClassAnalysisRequest { private String[] packages = new String[0]; private Class[] packageRoots = new Class[0]; private Class[] locationProviders = new Class[0]; + private boolean wholeClasspath = false; private Class[] importOptions = new Class[0]; private CacheMode cacheMode = CacheMode.FOREVER; @@ -25,6 +26,11 @@ public Class[] getLocationProviders() { return locationProviders; } + @Override + public boolean scanWholeClasspath() { + return wholeClasspath; + } + @Override public Class[] getImportOptions() { return importOptions; @@ -51,6 +57,11 @@ final TestAnalysisRequest withLocationProviders(Class... importOptions) { this.importOptions = importOptions;