Add example illustrating how more advanced method-to-method dependencies can be checked #1058
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
…ies can be checked Based on an example provided in TNG#235, this example aims to help people who are working on similar use cases in their projects. Signed-off-by: Per Lundberg <per.lundberg@hibox.tv>
perlun
force-pushed
the
add-method-from-only-particular-methods-example
branch
from
3aa755d
to
87e81ec
Compare
There was a problem hiding this comment.
Thanks a lot for your contribution and sorry for the late follow-up! I looked through it and I think it's a good idea to add another somewhat realistic use case of what can be done with ArchUnit! I added some comments for things I noticed.
One thing I'm not sure about is that it seems a little mingled to me what is about ArchUnit and what is about this specific pattern (singleton with wrong usage). I would try to compress everything that's related to the example itself in one central place, e.g. as Javadoc on the test method. I.e. what is the scenario, what is the anti-pattern, etc. So that inline comments then really only apply to ArchUnit's application itself. Also, I don't think we need comments about how to improve the singleton pattern (like "use Guava"), since that's not the point of the example anyway, right? Cause I'm afraid it might then just clutter the essence how we apply ArchUnit 🤷
| */ | ||
| public class SingletonClass { | ||
|
|
||
| // Poor man's reimplementation of Guava's memoization support. If your project supports Guava, |
There was a problem hiding this comment.
I would drop this comment. The example is about using ArchUnit, not how you should implement a singleton, given library x, right? 🤔 From my perspective you could even drop all code, leave it empty and just write comments // assume implemented, because the internals of this singleton or if it even works is irrelevant for the example / test, no? 🤔
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| /** | ||
| * @author Per Lundberg |
There was a problem hiding this comment.
We generally don't us the @author tag, since you can always see that from the commit history anyway...
| "to override the dependencies for tests, and also means the dependencies are much harder to identify when " + | ||
| "quickly looking at the code. Instead, move all getInstance() calls to the INSTANCE supplier and pass the " + | ||
| "dependency to the constructor that way. If this is impossible for a particular case, add the class name to " + | ||
| "the whitelist in " + getClass().getName() ) |
There was a problem hiding this comment.
| "the whitelist in " + getClass().getName() ) | |
| "the whitelist in the `should` clause above" |
| } | ||
|
|
||
| private ArchCondition<JavaMethod> onlyBeCalledFromWhitelistedOrigins( String... whitelistedOrigins ) { | ||
| return new ArchCondition<JavaMethod>( "only be called by whitelisted methods" ) { |
There was a problem hiding this comment.
| return new ArchCondition<JavaMethod>( "only be called by whitelisted methods" ) { | |
| return new ArchCondition<JavaMethod>( "only be called from whitelisted origins" ) { |
| // TODO: Add your own exceptions as needed here, if you have particular | ||
| // TODO: use cases where getInstance() calls are permissible. | ||
| // Static getInstance() methods are always allowed to call getInstance. This | ||
| // does not break dependency injection and does not come with significant | ||
| // design flaws. |
There was a problem hiding this comment.
I would drop this comment. It's just an example, it's clear that somebody needs to adjust this to their need. If you want to explain theory behind the test case itself I'd rather do it as Javadoc on the test class or test method.
There was a problem hiding this comment.
Otherwise, the comments just make it harder to quickly get through the code 🤷♂️
| .check( classes ); | ||
| } | ||
|
|
||
| private ArchCondition<JavaMethod> onlyBeCalledFromWhitelistedOrigins( String... whitelistedOrigins ) { |
There was a problem hiding this comment.
Technically this is onlyBeCalledFromOtherStaticGetInstanceMethodsOrWhitelistedOrigins? 🤔
| // Static getInstance() methods are always allowed to call getInstance. This | ||
| // does not break dependency injection and does not come with significant | ||
| // design flaws. | ||
| .filter( call -> !( Objects.equals( call.getOrigin().getName(), "getInstance" ) && call.getOrigin().getModifiers().contains( JavaModifier.STATIC ) ) ) |
There was a problem hiding this comment.
Maybe extract this as isNotFromStaticGetInstanceMethod(call)? 🤔
| .filter( call -> { | ||
| Optional<String> result = Arrays.stream( whitelistedOrigins ) | ||
| .filter( o -> call.getOrigin().getFullName().startsWith( o ) ) | ||
| .findFirst(); | ||
|
|
||
| return !result.isPresent(); | ||
| } ) |
There was a problem hiding this comment.
| .filter( call -> { | |
| Optional<String> result = Arrays.stream( whitelistedOrigins ) | |
| .filter( o -> call.getOrigin().getFullName().startsWith( o ) ) | |
| .findFirst(); | |
| return !result.isPresent(); | |
| } ) | |
| .filter(call -> Arrays.stream(whitelistedOrigins).noneMatch(it -> it.equals(call.getOriginOwner().getName()))) |
Based on an example provided in #235, this example aims to help people who are working on similar use cases in their projects.