@ArchTest instance fields or static methods in abstract base classes

[hankem]

#97 has removed the necessity to declare @ArchTests as static fields or methods.

archunit-junit4:0.9.1 however fails with

com.tngtech.archunit.junit.ArchTestInitializationException: java.lang.InstantiationException

on [@ArchTest (instance fields or non-static methods) in (a base class that cannot be instantiated itself, e.g. when the base class is abstract or does not have a default constructor)].

Example:

    @AnalyzeClasses(packages = "some.pkg")
    public class ArchTestWithAbstractBaseClass extends AbstractBaseClass {
    }

    abstract class AbstractBaseClass {
        @ArchTest
        ArchRule abstractBaseClassInstanceField = /*...*/

        @ArchTest
        void abstractBaseClassNonStaticMethod(JavaClasses classes) {
            /*...*/
        }
    }

The same holds true for ArchRules.in a class with such a base class.

Even though such constellations can probably be avoided (we all favor composition over inheritance, don't we...?), ArchUnit should try to instantiate the correct class (ArchTestWithAbstractBaseClass instead of AbstractBaseClass in the example above).

(Since I'm not experienced with Junit 5, I cannot judge yet whether archunit-junit5 has similar issues.)

[codecholeric]

Honestly I was aware of this, while I implemented it, and yes, I was thinking "it will fail if a rule is declared in an abstract class, but then who would do that, if ArchUnit offers such nice ways of composition" 😉
Especially since this feature was mainly created for use with Kotlin anyway, if you keep your rules static, it should work either way.
But since you provide a PR that fixes it, I won't argue about the necessity 😉
I'm not really a fan of abstract test base classes, and ArchUnit really offers a nice way to just compose suites via ArchRules.in(..), but I guess if there is the use case out there in the wild, we better support it.
But yes, since this was a conscious decision, JUnit 5 has exactly the same lack, I thought the complexity of pushing the class around was more cost than worth it. Should have known that it wouldn't take long until it bites me 😉
I'll fix this for JUnit 5 then as well

[codecholeric]

BTW, is this a real problem for you (i.e. should I release a 0.9.2), or are you okay with the current state for now? You can avoid it by keeping the rules in the abstract base class static I think, can't you?

[codecholeric]

Keeping this open for JUnit 5

[hankem]

Thanks for merging the PR! ;)
But don't hesitate to revert it again as soon as the added complexity turns out to hurt in any way; I absolutely agree with you that this is not an actual issue at all (Sorry for not pointing this out more clearly!), but was rather kind of an academic problem teasing me... ;)
So no, no need to release that, and please also don't waste your time on the Junit5-support of it.

[codecholeric]

Naah, you were right that I took a shortcut, it's definitely more proper now 😉
But I'm a fan of consistency, so I want JUnit 4 and JUnit 5 support to behave the same and support the same. Won't be a big drama to add this to the JUnit 5 support, I really just gambled that this would never pop up as an issue cause nobody would ever feel the need to have abstract base classes 😉

[hankem]

You can also leave it as another excercise for me... 😉

[codecholeric]

If you want, go for it, I'd be happy 😉 But don't feel obliged, if it's too stressful I'll implement it at some point before 1.0.0...

[bobtiernay-okta]

Are there plans to support placing @AnalyzeClasses on base classes as well? It would be nice to be able to allow encapsulation of it's configuration.

[codecholeric]

Hmm, usually I don't think inheritance is such a great pattern to share configuration or logic. What I usually do, is to create a library via composition to run the tests, like

@AnalyzeClasses(..)
class MyArchLibrary {
    @ArchTest
    static final ArchRules uiRules = ArchRules.in(MyUiRules.class);

    @ArchTest
    static final ArchRules serviceRules = ArchRules.in(MyServiceRules.class);

    // ...
} 

I would rather use your suggestion from #182 to encapsulate the details 😉
But if someone would want to supply a PR, I guess I'd merge it anyway 😉 Or if I have some spare time I might implement this. Also it would be necessary to decide what should happen if a base class and a sub class declare @AnalyzeClasses. Should the behavior be the same as for #182 or should the subclass overwrite the superclass?

[ejemba]

@codecholeric Hi if I can add an argument for the support of the @AnalyzeClasses on base classes :
We have an internal framework where we define our rules and the projects that use it. Inside the framework we define abstract base classes with @AnalyzeClasses so in the projects developers just extends base classes without any knowledge of @AnalyzeClasses options.
With the current composition pattern developers have to redefine @AnalyzeClasses options each time even if they are the same.

[codecholeric]

As I said, I'm not totally against it, feels like it would be consistent behavior. It will just take a little time until I find the time to implement this, but if anyone wants to contribute this, I'd be more than happy to merge it 👍

[bobtiernay-okta]

@codecholeric The main issue with not supporting abstract base classes is that your example above using rule libraries makes it next to impossible to run a single test from the command line or in an IDE like IntellliJ. At least I have been unsuccessful there. Am I missing something?

[codecholeric]

One big problem is, that IDEs like IntelliJ don't support to run a field as test, even if the JUnit TestEngine (or the Runner) supports it. So I don't see how abstract base classes would help with this, because an @ArchTest field in an abstract base class still can't be run individually. It might help for the one case that you have an instance test method, because that you could selectively override in the subclass to execute, but that is also not terribly convenient.

Another option might be to add a quick marker annotation like

@OnlyRunThis
@ArchTest
ArchRule rule = ... 

Which then would only run this one rule, even if multiple @ArchTest ArchRules have been imported 🤔 That would at least be platform independent and only depend on ArchUnitRunner / ArchUnitTestEngine...