ClassesShould.callMethod and subclasses

[rweisleder]

I want to check that no class prints exception stack traces directly to stderr. My attempt was this:

class ExampleTest {

    @Test
    void noMethodShouldCall_printStackTrace() {
        JavaClasses classes = new ClassFileImporter().importClasses(Example.class);
        noClasses().should().callMethod(Throwable.class, "printStackTrace").check(classes);
    }

    static class Example {

        void printStackTraceOfException() {
            try {
                throw new Exception("Hello World");
            } catch (Exception e) {
                e.printStackTrace();
            }
        }

        void printStackTraceOfRuntimeException() {
            try {
                throw new RuntimeException("Hello World");
            } catch (RuntimeException e) {
                e.printStackTrace();
            }
        }

        void logErrorOnException() {
            try {
                throw new Exception("Hello World");
            } catch (Exception e) {
                // log.error(...)
            }
        }
    }
}

Actual: The check is green.
Expectation: The check fails for printStackTraceOfException and printStackTraceOfRuntimeException.

If I replace to callMethod(Exception.class, "printStackTrace") then the check fails for printStackTraceOfException but not for printStackTraceOfRuntimeException.

[rweisleder]

GeneralCodingRules.NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS.check(classes); is also green but should fail for two methods.

[codecholeric]

The API method callMethod(owner, methodName, params) checks for exactly this call including the exact type, i.e. the behavior is as expected. That is why GeneralCodingRules.NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS internally uses a different approach and checks the method call more widely, namely

target(name("printStackTrace"))
                        .and(target(owner(assignableTo(Throwable.class))))
                        .and(target(parameterTypes(new Class[0]))))

What I don't understand is, why NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS would not fail in your example, I've copied your code, tried that rule, and it fails 2x as expected.
I've tried

@Test
public void noMethodShouldCall_printStackTrace() {
    JavaClasses classes = new ClassFileImporter().importClasses(Example.class);
    NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS.check(classes);
}

with your example class, and it reports

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes should access standard streams' was violated (2 times):
Method <com.tngtech.archunit.exampletest.ExampleTest$Example.printStackTraceOfException()> calls method <java.lang.Exception.printStackTrace()> in (ExampleTest.java:23)
Method <com.tngtech.archunit.exampletest.ExampleTest$Example.printStackTraceOfRuntimeException()> calls method <java.lang.RuntimeException.printStackTrace()> in (ExampleTest.java:31)

I've checked this on master, which is pretty much ArchUnit 0.8.0, can you verify, if the code above really doesn't fail for your example with the current version?

[rweisleder]

I've checked it against 0.8.0 and it's reproducible. With the example class in the first comment

@Test
public void noMethodShouldCall_printStackTrace() {
    JavaClasses classes = new ClassFileImporter().importClasses(Example.class);
    NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS.check(classes);
}

is green but should fail, and

@Test
public void noMethodShouldCall_printStackTrace_importExceptions() {
    JavaClasses classes = new ClassFileImporter().importClasses(Example.class, Exception.class, RuntimeException.class);
    NO_CLASSES_SHOULD_ACCESS_STANDARD_STREAMS.check(classes);
}

fails as expected.

When it comes to evaluate the AssignableToPredicate, JavaClass{name='java.lang.Exception'} and JavaClass{name='java.lang.RuntimeException'} are not assignable to java.lang.Throwable, because the superClass field is empty.

[rweisleder]

Well, this is interesting... In an empty project I see my described behaviour. When I copy+paste the code to archunit/src/test/java to commit an example I see your described behaviour.

The difference is the property resolveMissingDependenciesFromClassPath=true. After adding this to my project I also get the expected failure.

[codecholeric]

Oh yes, this is always confusing (I've come across it before). That's why I normally set that resolveMissing... config to true.
The problem is, that the bytecode imported just doesn't have the necessary information otherwise. In the bytecode there is only a call of Exception.printStacktrace() but ArchUnit knows nothing about Exception, if it is not imported as well. Thus there is no way of knowing, that Exception is extending Throwable, except the 'missing classes' are resolved from the classpath (which naturally works quite well for JDK classes).

[codecholeric]

Since this is pretty much a known limitation (I don't see any way to fix this, since if classes are not imported, ArchUnit just can't know anything about their structure), can I close this issue?
Or do you have any idea how this behavior could be made clearer for the future?

[rweisleder]

I think we should adjust the documentation and javadoc of GeneralCodingRules that it may give the wrong results if used exceptions and their hierarchy are not imported. A hint to resolveMissing... would also be useful.

[codecholeric]

I'll do that, the only problem is, that this is really a basic concept of everything. So even if I add it to GeneralCodingRules, any rule dealing with accesses or any random self-written ArchCondition could produce unexpected behavior for that reason.
There is a section in the user guide referring to this problem: https://www.archunit.org/userguide/html/000_Index.html#_dealing_with_missing_classes
But I guess not many people will read up to there, so the question is, how this information can be distributed in the best way.
Maybe I should add Javadoc to the respective import methods of the ClassFileImporter, but even then, if someone uses the JUnit support, this Javadoc will not be within the focus of the test.
So it's hard for me to decide, where to put this information.
Maybe I'll put it on the import methods of the ClassFileImporter and put a Javadoc reference on the GeneralCodingRules and on @AnalyzeClasses, what do you think?

[rweisleder]

Maybe I'll put it on the import methods of the ClassFileImporter and put a Javadoc reference on the GeneralCodingRules and on @AnalyzeClasses

Sounds good to me.

[codecholeric]

I've tried to add more documentation to those classes, can you check, if this is understandable? Do you think this would have helped you to quicker get to the root of the behavior of your example?

[rweisleder]

Thanks, this is a very good improvement and will help others a lot!

[codecholeric]

Okay, glad it helps 😃
I'll close this issue...

[perlun]

Oh yes, this is always confusing (I've come across it before). That's why I normally set that resolveMissing... config to true.

For reference: resolveMissingDependenciesFromClassPath seems to be true by default nowadays, but can be set to false in e.g. archunit.properties if needed: https://www.archunit.org/userguide/html/000_Index.html#_configuring_the_resolution_behavior

(I started looking into this since I was running into a weird case where a ArchCondition<JavaClass> was trying to do item.getAllAccessesFromSelf(), but this would not include all accesses done by superclasses which are not imported. Adding the superclass' package to the ClassFileImporter.importPackages() call seems to have fixed the problem, but I'm unsure of whether this is by design or not. 🤔 I would presume that this is the exact problem that resolveMissingDependenciesFromClassPath is trying to fix. 🤷)