Fix onlyxxx that behaving inconsistently for unresolvable targets #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By default `ArchConfigurationRule` should use the configured `resolveAdditionalDependenciesFromClassPath` instead of the `boolean` default `false`. Unfortunately this masked that `@Before void setUp() { configuration.... }` does actually not do anything, since `@Before` runs after the rule `before` and the rule `before` sets the configuration.
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
To make assertions about access targets is in general not trivial. From the byte code we can only infer that the method `x` with some signature was called on target class `y`. This does not give us enough information to make assertions about the method itself (thus ArchUnit distinguishes an `AccessTarget` having a name and an owner where it is declared from a full `JavaMember` which can also have e.g. annotations, an information that cannot be derived from the mere byte code access).
Syntax elements of the form `only{Call/Access}...That...`, which want to apply a predicate to some subclass of `JavaMember`, must thus try to resolve the actual call of a target first (by calling `target.resolve()` which will in some cases even report multiple targets, but can also return no target at all, e.g. if the target class has not been imported and thus there is no information about the full method). So far these methods have asserted that at least one element of the resolved targets satisfies the predicate. Unfortunately this will fail and report a violation whenever the target has not been imported, which is a somewhat confusing behavior. Originally I thought it would be good, because if one wants to make assertions about the resolved targets, one should ensure all targets are present and if not failure is okay. But on the one hand the error message is pretty confusing (which by itself would be fixable), on the other hand there are simply classes that cannot be correctly imported, for example array types. Since this behavior is confusing I have decided to let the test pass if the target cannot be resolved. This could lead to tests wrongly passing if not all necessary classes are imported, but in the end it is an user decision what to import and hard to reason about from ArchUnit's point of view (also now that `resolveMissingClassesFromClasspath` is `true` by default, it should also happen way less often, that necessary classes are forgotten for the import).
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
codecholeric
force-pushed
the
fix-onlyxxxThat-behaving-inconsistently-for-unresolvable-targets
branch
from
3b685c0 to
59bb586
Compare
codecholeric
deleted the
fix-onlyxxxThat-behaving-inconsistently-for-unresolvable-targets
branch
codecholeric
added a commit
that referenced
this pull request
Feb 21, 2021
So far the set of `should().only{Access/Call}...That...(..)` syntax methods have behaved a little strange when the target of the call was not imported from a class file. This happens for example for unresolvable types like array types.
The basic problem is, that from the byte code point of view only certain properties about an `AccessTarget` (like `SomeClass.method()`) can be derived. For further details (e.g. is the method called annotated with a certain annotation) it is necessary to try and resolve the respective method that is targeted from the imported target `JavaClass`. This resolution can unfortunately fail, if the target class has not been imported by ArchUnit (or is unresolvable like an array or primitive type).
So far these `only...` methods wanted that at least one of the resolved targets would satisfy the given predicate (e.g. `annotatedWith(..)`). If the target is unresolvable this assertion would always fail, reporting things like `[some.Array;.clone() is not annotated with ...`. But since it would always fail, it could at the same time complain about `[some.Array;.clone() is not annotated with ...` and `[some.Array;.clone() is annotated with ...` which is certainly super confusing (and plain wrong in the second case where `not(annotatedWith(..))` would be used; compare the example).
This PR resolves this issue by counting unresolvable targets as always satisfying these predicates. This might lead to wrongly successful tests if classes are missing from the import, but in the end it is indeterminable for ArchUnit anyway, what classes are wanted / desired for the test and which classes are not wanted. It is also a fair point of view to say that unimported classes should not be considered when asserting `should().only...`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far the set of
should().only{Access/Call}...That...(..)syntax methods have behaved a little strange when the target of the call was not imported from a class file. This happens for example for unresolvable types like array types.The basic problem is, that from the byte code point of view only certain properties about an
AccessTarget(likeSomeClass.method()) can be derived. For further details (e.g. is the method called annotated with a certain annotation) it is necessary to try and resolve the respective method that is targeted from the imported targetJavaClass. This resolution can unfortunately fail, if the target class has not been imported by ArchUnit (or is unresolvable like an array or primitive type).So far these
only...methods wanted that at least one of the resolved targets would satisfy the given predicate (e.g.annotatedWith(..)). If the target is unresolvable this assertion would always fail, reporting things like[some.Array;.clone() is not annotated with .... But since it would always fail, it could at the same time complain about[some.Array;.clone() is not annotated with ...and[some.Array;.clone() is annotated with ...which is certainly super confusing (and plain wrong in the second case wherenot(annotatedWith(..))would be used; compare the example).This PR resolves this issue by counting unresolvable targets as always satisfying these predicates. This might lead to wrongly successful tests if classes are missing from the import, but in the end it is indeterminable for ArchUnit anyway, what classes are wanted / desired for the test and which classes are not wanted. It is also a fair point of view to say that unimported classes should not be considered when asserting
should().only....Resolves: #340