-
Notifications
You must be signed in to change notification settings - Fork 336
Allow package info classes to be resolved as a dependency #1565
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 |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.function.Function; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import com.google.common.collect.HashMultimap; | ||
| import com.google.common.collect.ImmutableSet; | ||
|
|
@@ -121,7 +122,12 @@ public static <T> MetricsComponents<T> from(Collection<T> elements, Function<? s | |
| public static MetricsComponents<JavaClass> fromPackages(Collection<JavaPackage> packages) { | ||
| ImmutableSet.Builder<MetricsComponent<JavaClass>> components = ImmutableSet.builder(); | ||
| for (JavaPackage javaPackage : packages) { | ||
| components.add(MetricsComponent.of(javaPackage.getName(), javaPackage.getClassesInPackageTree())); | ||
| components.add(MetricsComponent.of( | ||
| javaPackage.getName(), | ||
| javaPackage.getClassesInPackageTree().stream() | ||
| .filter(jc -> !jc.getSimpleName().equals("package-info")) | ||
|
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. Sorry for opening a new frontier, but I needed a fresh review from the start after a month... this is a behavior change in case of having a package-info file. please look at MetricsComponentsTest.creates_components_from_Java_packages and make explicit that the package-info is not imported (you need to create one in the test scenario, probably best in case 1 for top-level and subpackage. adding the package-info might also require changes to the the other test methods. it would be interesting to see whether creates_components_from_Java_classes stick with 3 components. I hope this doesn't break too much for our users becasue the old behavior is unexpected for me and I haven't seen MetricsCompment before |
||
| .collect(Collectors.toList()) | ||
| )); | ||
| } | ||
| return MetricsComponents.of(components.build()); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| import java.util.List; | ||
| import java.util.concurrent.BlockingQueue; | ||
| import java.util.function.Predicate; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import com.tngtech.archunit.base.DescribedPredicate; | ||
| import com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation; | ||
|
|
@@ -423,6 +424,15 @@ public void test_getPackageInfo() { | |
| JavaPackage nonAnnotatedPackage = importPackage("packageexamples"); | ||
|
|
||
| assertThat(annotatedPackage.getPackageInfo()).isNotNull(); | ||
| assertThat(annotatedPackage.getClasses()) | ||
| .hasSize(3); | ||
| assertThat(annotatedPackage.getClasses().stream().map(JavaClass::getFullName).collect(Collectors.toList())) | ||
| .as("all classes and package-info are loaded as JavaClass") | ||
| .containsExactlyInAnyOrder( | ||
| "com.tngtech.archunit.core.domain.packageexamples.annotated.package-info", | ||
|
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. should we really get the package-info in the classes? I just wanted to write that you can't get it via reflection as Class.forName("com.tngtech.archunit.core.domain.packageexamples.annotated.package-info") but you actually get it as an interface... there is always sth to learn about java... What are your thoughts on including the package-info class in the classes of the package? What about a missing package-info file? I just checked that it is NOT included in that case. please extend the related/caused by to f0fca6f#r3136501904
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 also just checked how the libraries Reflections and ClassGraph handle this: which is quite hacky. |
||
| "com.tngtech.archunit.core.domain.packageexamples.annotated.PackageLevelAnnotation", | ||
| "com.tngtech.archunit.core.domain.packageexamples.annotated.WithinAnnotatedPackage" | ||
| ); | ||
|
|
||
| assertThatThrownBy(nonAnnotatedPackage::getPackageInfo) | ||
|
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. created by f0fca6f#r3332389020 I'd prefer to exchange the tested package by one with some classes in it (e.g. |
||
| .isInstanceOf(IllegalArgumentException.class) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| package com.tngtech.archunit.core.domain.packageexamples.annotated; | ||
|
|
||
| public class WithinAnnotatedPackage { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
|
|
||
|
|
@@ -22,17 +23,20 @@ | |
| import com.tngtech.archunit.core.domain.JavaConstructorCall; | ||
| import com.tngtech.archunit.core.domain.JavaConstructorReference; | ||
| import com.tngtech.archunit.core.domain.JavaEnumConstant; | ||
| import com.tngtech.archunit.core.domain.JavaField; | ||
| import com.tngtech.archunit.core.domain.JavaFieldAccess; | ||
| import com.tngtech.archunit.core.domain.JavaMethod; | ||
| import com.tngtech.archunit.core.domain.JavaMethodCall; | ||
| import com.tngtech.archunit.core.domain.JavaMethodReference; | ||
| import com.tngtech.archunit.core.domain.JavaPackage; | ||
| import com.tngtech.archunit.core.domain.JavaParameterizedType; | ||
| import com.tngtech.archunit.core.domain.JavaType; | ||
| import com.tngtech.archunit.core.domain.JavaWildcardType; | ||
| import com.tngtech.archunit.core.domain.ReferencedClassObject; | ||
| import com.tngtech.archunit.core.domain.ThrowsDeclaration; | ||
| import com.tngtech.archunit.core.domain.properties.HasAnnotations; | ||
| import com.tngtech.archunit.core.importer.DependencyResolutionProcessTestUtils.ImporterWithAdjustedResolutionRuns; | ||
| import com.tngtech.archunit.core.importer.testexamples.OtherClass; | ||
| import com.tngtech.archunit.core.importer.testexamples.SomeAnnotation; | ||
| import com.tngtech.archunit.core.importer.testexamples.annotatedclassimport.ClassWithUnimportedAnnotation; | ||
| import com.tngtech.archunit.core.importer.testexamples.annotatedparameters.ClassWithMethodWithAnnotatedParameters; | ||
|
|
@@ -44,9 +48,11 @@ | |
| import com.tngtech.archunit.core.importer.testexamples.annotationresolution.SomeAnnotationWithAnnotationParameter; | ||
| import com.tngtech.archunit.core.importer.testexamples.annotationresolution.SomeAnnotationWithClassParameter; | ||
| import com.tngtech.archunit.core.importer.testexamples.classhierarchyresolution.Child; | ||
| import com.tngtech.archunit.core.importer.testexamples.packageinforesolution.ClassThatReferencesOtherClass; | ||
| 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.Assertions; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
|
|
||
|
|
@@ -60,6 +66,7 @@ | |
| import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_GENERIC_SIGNATURE_TYPES_PROPERTY_NAME; | ||
| import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_MEMBER_TYPES_PROPERTY_NAME; | ||
| import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_SUPERTYPES_PROPERTY_NAME; | ||
| import static com.tngtech.archunit.core.importer.DependencyResolutionProcess.MAX_ITERATIONS_FOR_PACKAGE_INFO_PROPERTY_NAME; | ||
| import static com.tngtech.archunit.core.importer.testexamples.SomeEnum.OTHER_VALUE; | ||
| import static com.tngtech.archunit.core.importer.testexamples.SomeEnum.SOME_VALUE; | ||
| import static com.tngtech.archunit.core.importer.testexamples.annotatedparameters.ClassWithMethodWithAnnotatedParameters.methodWithOneAnnotatedParameterWithTwoAnnotations; | ||
|
|
@@ -621,6 +628,33 @@ class Innermost { | |
| assertThatType(outermost).matches(Outermost.class); | ||
| } | ||
|
|
||
| @Test | ||
| public void automatically_resolves_packages() { | ||
| JavaClass otherClass = new ClassFileImporter() | ||
| .importClass(OtherClass.class); | ||
|
|
||
| JavaPackage checkedClassPackageInfo = otherClass.getPackage(); | ||
|
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. The
Contributor
Author
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. Looks like these exist in JavaPackageTest:
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. yes, but since you re-create the JavaPackage, it might be worth checking that the re-created package behaves as expected in these methods
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. still open, please add an assertion like |
||
| assertThat(checkedClassPackageInfo.getAnnotations()).hasSize(1); | ||
| assertThat(checkedClassPackageInfo.isAnnotatedWith(SomeAnnotation.class)).isTrue(); | ||
| } | ||
|
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. When running
Contributor
Author
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. This is already the normal behavior in scanning a package. Ill add this assertion to JavaPackageTest.test_getPackageInfo() to show this:
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. how about packages without a package-info file? I would prefer to never have package-info in this result
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. related to f0fca6f#r3332389020 |
||
|
|
||
| @Test | ||
| public void automatically_resolves_dependency_packages() { | ||
| JavaClass checkedClass = ImporterWithAdjustedResolutionRuns | ||
| .disableAllIterationsExcept(MAX_ITERATIONS_FOR_PACKAGE_INFO_PROPERTY_NAME, MAX_ITERATIONS_FOR_MEMBER_TYPES_PROPERTY_NAME) | ||
| .importClass(ClassThatReferencesOtherClass.class); | ||
|
|
||
| Optional<JavaField> firstField = checkedClass.getAllFields().stream().findFirst(); | ||
| assertThat(firstField).isPresent(); | ||
| JavaClass otherClass = firstField.get().getRawType(); | ||
| assertThat(otherClass).isFullyImported(true); | ||
|
|
||
| JavaPackage otherClassPackageInfo = otherClass.getPackage(); | ||
|
|
||
| Assertions.assertThat(otherClassPackageInfo.getAnnotations()).hasSize(1); | ||
| Assertions.assertThat(otherClassPackageInfo.isAnnotatedWith(SomeAnnotation.class)).isTrue(); | ||
| } | ||
|
|
||
| @DataProvider | ||
| public static Object[][] data_automatically_resolves_generic_type_parameter_bounds() { | ||
| @SuppressWarnings("unused") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package com.tngtech.archunit.core.importer.testexamples.packageinforesolution; | ||
|
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 understand this new package name.
Contributor
Author
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. It is important that this class is in a different package than
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 already suspected that, but "package in for resolution" doesn't really tell me anything. |
||
|
|
||
| import com.tngtech.archunit.core.importer.testexamples.OtherClass; | ||
|
|
||
| public class ClassThatReferencesOtherClass { | ||
| OtherClass otherClass; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.