Adjust AnalyzeClasses annotation to support individual classes as parameter#1569
Adjust AnalyzeClasses annotation to support individual classes as parameter#1569TheManWhoStaresAtCode wants to merge 4 commits into
Conversation
11e740b to
f6a08b9
Compare
StefanGraeber
left a comment
StefanGraeber
left a comment
There was a problem hiding this comment.
looks good, just some comment and naming updates I'd prefer
| * <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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for pointing this out. I (hopefully) improved the documentation to address this point as well. Furthermore, I added a test to ensure that the behavior is indeed a union as pointed out by you.
For the later, I had the choice between using mocks and having a fast test run (< 1s) or use the actual class loader resulting in longer test runs (5 seconds or more). I decided to prefer test speed however, I'm interested in getting your opinion on that topic.
There was a problem hiding this comment.
The new documentation looks good to me.
The new test is IMO sufficient regarding mocking or real class loader, but has another problem with poorly chosen test data. There is a separate comment for that issue
StefanGraeber
left a comment
StefanGraeber
left a comment
There was a problem hiding this comment.
please rebase and cleanup the commits in the end. it's currently really hard to follow with merging old branches
the only blocking issue for me is now the overlapping testdata effectively skipping one assertion
|
|
||
| verify(cacheClassFileImporter).importClasses(anySet(), locationCaptor.capture()); | ||
| assertThat(locationCaptor.getValue()) | ||
| .has(locationContaining("archunit/junit/internal")) |
There was a problem hiding this comment.
both .withPackages("com.tngtech.archunit.junit.internal") and .withClassesToAnalyze(getClass()) satisfy this condition as ClassCacheTest resides in that package.
i.e. you can remove withPackages("com.tngtech.archunit.junit.internal") and the test is still green
please choose another package.
I tried java.lang.util but it doesn't work, likely because the setup restrict the scan to the current package.
adding an empty class in a subpackage and referencing that package works.
an empty package doesn't work, it must contain a class
There was a problem hiding this comment.
Good catch - I managed to get the test working with java.utils.
| * <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. |
There was a problem hiding this comment.
The new documentation looks good to me.
The new test is IMO sufficient regarding mocking or real class loader, but has another problem with poorly chosen test data. There is a separate comment for that issue
…ameter Relates to #1195 Signed-off-by: Andreas Zöller <andreas.zoeller@tngtech.com>
…e test * extend @AnalyzeClasses Javadoc to clarify that combining several properties results in the union of all derived classes * add ClassCacheTest.combines_locations_from_all_sources_into_union to verify the union behavior * streamline test name and assertions in ArchUnitTestEngineTest to compare against the annotation values instead of hardcoding emptiness Signed-off-by: Andreas Zöller <andreas.zoeller@tngtech.com>
…ed test instance unambiguous Signed-off-by: Andreas Zöller <andreas.zoeller@tngtech.com>
* use a more specific package to avoid overlapping assertions Signed-off-by: Andreas Zöller <andreas.zoeller@tngtech.com>
9a2e8b6 to
beb48c7
Compare
2 participants
As suggested in the ticket adjust the annotation to have a new classes property to select individual classes as an additional option.
Resolves #1195