-
Notifications
You must be signed in to change notification settings - Fork 332
Adjust AnalyzeClasses annotation to support individual classes as parameter #1569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,16 @@ | |
| * a rule checking for no accesses to classes assignable to C will not fail, since ArchUnit does not know about the details | ||
| * of class B, but only simple information like the fully qualified name. For information how to configure the import and | ||
| * resolution behavior of missing classes, compare {@link ClassFileImporter}. | ||
| * <br><br> | ||
| * Classes to be analyzed can be specified in different ways: | ||
| * <ul> | ||
| * <li>{@link #packages()} - specify package names as strings</li> | ||
| * <li>{@link #packagesOf()} - specify packages relative to classes</li> | ||
| * <li>{@link #classes()} - specify individual classes to analyze</li> | ||
| * <li>{@link #locations()} - specify custom locations via {@link LocationProvider}</li> | ||
| * <li>{@link #wholeClasspath()} - import all classes on the classpath</li> | ||
| * </ul> | ||
| * These options can be combined. If no option is specified, the package of the annotated test class will be imported. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here. can we make clear that each option adds more classes and never reduce the selection? docs/userguide/009_JUnit_Support.adoc and archunit-junit/src/main/java/com/tngtech/archunit/junit/internal/ClassCache.java confirm this assumption |
||
| * | ||
| * @see ClassFileImporter | ||
| */ | ||
|
|
@@ -87,4 +97,9 @@ | |
| * @return The {@link CacheMode} to use for this test class. | ||
| */ | ||
| CacheMode cacheMode() default CacheMode.FOREVER; | ||
|
|
||
| /** | ||
| * @return Classes to be used for testing instead of packages | ||
| */ | ||
| Class<?>[] classes() default {}; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import com.tngtech.archunit.junit.engine_api.FieldSelector; | ||
| import com.tngtech.archunit.junit.engine_api.FieldSource; | ||
| import com.tngtech.archunit.junit.internal.ArchUnitTestEngine.SharedCache; | ||
| import com.tngtech.archunit.junit.internal.testexamples.AnalyzeClassesWithClassesProperty; | ||
| import com.tngtech.archunit.junit.internal.testexamples.ClassWithPrivateTests; | ||
| import com.tngtech.archunit.junit.internal.testexamples.ComplexMetaTags; | ||
| import com.tngtech.archunit.junit.internal.testexamples.ComplexRuleLibrary; | ||
|
|
@@ -1079,6 +1080,7 @@ void passes_AnalyzeClasses_to_cache() { | |
| assertThat(request.getLocationProviders()).isEqualTo(expected.locations()); | ||
| assertThat(request.scanWholeClasspath()).as("scan whole classpath").isTrue(); | ||
| assertThat(request.getImportOptions()).isEqualTo(expected.importOptions()); | ||
| assertThat(request.getClassesToAnalyze()).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -1104,6 +1106,21 @@ void a_class_with_analyze_classes_as_meta_annotation() { | |
| assertThat(request.scanWholeClasspath()).as("scan whole classpath").isTrue(); | ||
| assertThat(request.getImportOptions()).isEqualTo(expected.importOptions()); | ||
| } | ||
|
|
||
| @Test | ||
| void passes_AnalyzeClasses_with_new_classes_property_to_cache() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like names like "new" because it will not be new at some time. Please remove that from the test name |
||
| execute(createEngineId(), AnalyzeClassesWithClassesProperty.class); | ||
|
|
||
| verify(classCache).getClassesToAnalyzeFor(eq(AnalyzeClassesWithClassesProperty.class), classAnalysisRequestCaptor.capture()); | ||
| ClassAnalysisRequest request = classAnalysisRequestCaptor.getValue(); | ||
| AnalyzeClasses expected = AnalyzeClassesWithClassesProperty.class.getAnnotation(AnalyzeClasses.class); | ||
| assertThat(request.getClassesToAnalyze()).isEqualTo(expected.classes()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you use the classes from the annotation here but an explicit empty for other properties like |
||
| assertThat(request.getImportOptions()).isEqualTo(expected.importOptions()); | ||
| assertThat(request.getPackageNames()).isEmpty(); | ||
| assertThat(request.getPackageRoots()).isEmpty(); | ||
| assertThat(request.getLocationProviders()).isEmpty(); | ||
| assertThat(request.scanWholeClasspath()).as("scan whole classpath").isFalse(); | ||
| } | ||
| } | ||
|
|
||
| @Nested | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package com.tngtech.archunit.junit.internal.testexamples; | ||
|
|
||
| import com.tngtech.archunit.core.domain.JavaClass; | ||
| import com.tngtech.archunit.core.importer.ImportOption; | ||
| import com.tngtech.archunit.junit.AnalyzeClasses; | ||
| import com.tngtech.archunit.junit.ArchTest; | ||
| import com.tngtech.archunit.lang.ArchCondition; | ||
| import com.tngtech.archunit.lang.ArchRule; | ||
| import com.tngtech.archunit.lang.ConditionEvents; | ||
| import com.tngtech.archunit.library.testclasses.coveringallclasses.first.First; | ||
| import com.tngtech.archunit.library.testclasses.coveringallclasses.second.Second; | ||
|
|
||
| import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; | ||
|
|
||
| @AnalyzeClasses( | ||
| classes = {First.class, Second.class}, | ||
| importOptions = {ImportOption.DoNotIncludeTests.class, ImportOption.DoNotIncludeJars.class} | ||
| ) | ||
| public class AnalyzeClassesWithClassesProperty { | ||
| @ArchTest | ||
| public static final ArchRule irrelevant = classes().should(new ArchCondition<JavaClass>("exist") { | ||
| @Override | ||
| public void check(JavaClass item, ConditionEvents events) { | ||
| } | ||
| }); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,15 @@ public void gets_all_classes_relative_to_class() { | |
| assertThat(classes.contain(getClass())).as("root class is contained itself").isTrue(); | ||
| } | ||
|
|
||
| @Test | ||
| public void gets_all_classes_specified() { | ||
| JavaClasses classes = cache.getClassesToAnalyzeFor(TestClass.class, new TestAnalysisRequest() | ||
| .withClassesToAnalyze(getClass())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe |
||
|
|
||
| assertThat(classes).hasSize(1); | ||
| assertThat(classes.contain(getClass())).as("root class is contained itself").isTrue(); | ||
| } | ||
|
|
||
| @Test | ||
| public void get_all_classes_by_LocationProvider() { | ||
| JavaClasses classes = cache.getClassesToAnalyzeFor(TestClass.class, new TestAnalysisRequest() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find it obvious whether the different options are used as union or intersect.
AFAIK it is a union, so each option can only add more classes.
Should we make that clear?
docs/userguide/009_JUnit_Support.adoc and archunit-junit/src/main/java/com/tngtech/archunit/junit/internal/ClassCache.java confirm this assumption