Remove @Inherited from annotations and upgrade utils to find them

[eeverman]

This is ultimately related to #696

Remove @Inherited from all annotations used to register extensions and upgrade the PioneerAnnotationUtils to find annotations on superclasses, even if they are not @Inherited.

From JUnit's perspective, there is no requirement that annotations on a test superclass be marked as @Inherited. And the meaning is not correct: When an extension-registering annotation is placed on a test superclass, it really is on the superclass and not on the subclass, as seen in the distinct registration order of the extensions (superclass extensions are registered first). The only reason to include @Inherited on extension-registering annotation is because the PioneerAnnotationUtils.findClosestEnclosingAnnotation doesn't currently find annotations on superclasses without it.

The tie-in to #696
If users are allowed to compose Pioneer annotations into their own custom annotations (#696), then it is very easy for users to forget to include the @Inherited marker. This issue really needs to be resolved for 696 to be workable w/out a lot of users running into issues.

[Michael1993]

PioneerAnnotationUtils.findClosestEnclosingAnnotation specifically does not find not @Inherited annotations on superclasses. That is intentional.

I am closing this as the premise is incorrect.

[eeverman]

I'm not sure where the method you have linked sits in the typical flow of an extension finding its annotation. My assumption is that PioneerAnnotationUtils.findClosestEnclosingAnnotation is the method most extensions use to find their annotation.

Confirming the intentional behavior, this test shows that PioneerAnnotationUtils.findClosestEnclosingAnnotation cannot find an annotation on a superclass unless it is marked as @Inherited.

If findClosestEnclosingAnnotation cannot find them, but JUnit still registers them, doesn't that create a problem for users who compose them into other annotations and are not aware of the 'must be marked as Inherited' requirement?

FWIW, I created a separate test that simulates a user composed annotation on a superclass. To make annotations easy to use, I think it would be ideal for this test to work.

[nipafx]

I'm not sure where the method you have linked sits in the typical flow of an extension finding its annotation. My assumption is that PioneerAnnotationUtils.findClosestEnclosingAnnotation is the method most extensions use to find their annotation.

It doesn't matter much, which exact public method from PioneerAnnotationUtils are used the most, they all eventually call into findOnType (that's the method @Michael1993 pointed out), so that's where the issue lies.

I typed out a full reply why we should keep the code as is and then almost changed my mind. I'll put both arguments here.

Keep as is

And the meaning is not correct: When an extension-registering annotation is placed on a test superclass, it really is on the superclass and not on the subclass, as seen in the distinct registration order of the extensions (superclass extensions are registered first).

I agree, but I don't see how that is an argument to change the behavior of extension discovery. As it is, a class-level extension is either present on just the superclass (no @Inherited) or also on all subclasses (with @Inherited). That maps pretty well on the behavior you describe.

The only reason to include @Inherited on extension-registering annotation is because the PioneerAnnotationUtils.findClosestEnclosingAnnotation doesn't currently find annotations on superclasses without it.

No, the only reason to include @Inherited on extension-registering annotations is to express that that extension is inherited by subclasses. 😉 Maybe not all extensions want that.

And the JLS and Javadoc are pretty clear:

The annotation type java.lang.annotation.Inherited is used to indicate that annotations on a class C corresponding to a given annotation type are inherited by subclasses of C.

We are aligned with the language semantics.

then it is very easy for users to forget to include the @Inherited marker

I think that's the real issue here: Users might forget to correctly apply the language semantics. I don't think it makes sense to accommodate that by (a) violating the semantics ourselves and (b) limiting user's options.

Change

What started to change my mind was this:

If findClosestEnclosingAnnotation cannot find them, but JUnit still registers them, doesn't that create a problem for users who compose them into other annotations and are not aware of the 'must be marked as Inherited' requirement?

Why does Jupiter register these extensions? And what's the effect of it. In some cases, a pretty hilarious one:

public class Tests implements TestA, TestB { }

interface TestA {
	@Test
	default void testA() { }
}

@Disabled
interface TestB {
	@Test
	default void testB() { }
}

How many tests get executed?

• 2 because @Disabled on TestB is ignored?
• 1 because @Disabled on TestB disables those tests?
• 0 because @Disabled on TestB is inherited by Tests and everything is disabled?

It's indeed the last option, which is... maybe unexpected? Didn't the user just want to disable the tests in TestB? But figuring out which tests those are exactly, is a hassle because it requires to figure out what interface a test method comes from. And what happens then if two interfaces declare the same method with conflicting annotations? So instead of "sharding" a test class by where an extension comes from, Jupiter just treats them all as present on the very test method that is being executed - from the user guide section on extension inheritance:

Registered extensions are inherited within test class hierarchies with top-down semantics. Similarly, extensions registered at the class-level are inherited at the method-level.

But what about the language semantics? This described @Inherited behavior without @Inherited annotation, which means there's no way to get non-@Inherited behavior. I uphold that this is against the spirit of the JLS!

Which brought me to the question what it would even mean for a test annotation to be applied to only the superclass. Because here's the thing: In the end, everything Jupiter does, revolves around method execution. There's no code generation, no information gathering, etc.. Jupiter executes test (and setup/teardown) methods and nothing else.

So what would be the value of requiring @Inherited? It would allow the distinction between test methods that are executed as members of interface Tests from the same methods executed as members of class MoreTests implements Tests. But that can never happen! All methods run in the context of an instance of a concrete class and tests on an interface or an abstract class are never executed unless a concrete class extends/implements it and inherits them.

That leaves as only meaningful place of distinction concrete test classes that also extended. There, requiring @Inherited would allow extensions to run differently in class MoreTests than in class EvenMoreTests. I don't think I've ever seen this in a test suite.

Synthesis

My current understanding is that JLS and Jupiter are at odds here. The former requires @Inherited to apply an annotation to subclasses but the latter ignores that and treats all extensions as inheritable. And I think there's a strong argument to be made that the loss of expressiveness that Jupiter's approach entails doesn't matter much.

So which path do we choose? Unless there are compelling arguments for why the loss of expressiveness is an issue, I would prefer to conform with Jupiter. Particularly because what it says about extensions, should also apply to ours:

Registered extensions are inherited within test class hierarchies with top-down semantics. Similarly, extensions registered at the class-level are inherited at the method-level.

I will add this to the 2.0 milestone because this feels like an incompatible change. I don't know when I'll have time to make the change and see how it ripples through the code base, so if anybody wants to give it a shot, feel free to do so.

[eeverman]

Wow, thank you for all the analysis on this!

IMO, tests happening as a result of default interface methods is a degenerate/unplanned case. I think that possibility opened up when default methods were added to the JLS and the JUnit people never really thought through the implications. Perhaps that should be a JUnit feature request: Throw an error if tests are discovered in an interface's default methods.

[jbduncan]

IMO, tests happening as a result of default interface methods is a degenerate/unplanned case. I think that possibility opened up when default methods were added to the JLS and the JUnit people never really thought through the implications.

@eeverman I think they have thought about it, actually, but I'm not sure to what extent. I say this because somewhere in this section in the user guide, they describe how to test interface contracts, which is useful for testing common behaviour across a number of units. For example, it can be used to check that two versions of a unit have the same behaviour, if one is doing a big refactoring or following "branch by abstraction". Another example is checking that a specification is followed, like the javadocs for Collection with guava-testlib's collection testers (although they use a different approach from JUnit 4 called TestSuites).

[beatngu13]

First of all: thanks @eeverman for bringing this to our attention! 🙏

I think there are two distinct things to discuss:

1. "Remove @Inherited from all annotations used to register extensions […]"
2. "[U]pgrade the PioneerAnnotationUtils to find annotations on superclasses, even if they are not @Inherited."

When it comes to 2, I believe this is the corresponding change in Jupiter (2017):

• Issue: Treat non-inherited composed annotations for inherited annotations as inherited junit-team/junit5#1133
• Commit: junit-team/junit5@c706853

From Jupiter's 5.1.0 release notes:

• Due to a change in the JUnit Platform's AnnotationUtils class, non-inherited
  composed annotations which are meta-annotated with a given @Inherited annotation
  are now considered to be implicitly inherited when searching for the given
  meta-annotation within a class hierarchy.
  • For example, an @Inherited annotation such as @TestInstance will now be discovered
    on a custom composed annotation declared on a superclass even if the composed
    annotation is not declared as @Inherited.

It feels like we should adopt this behavior in PioneerAnnotationUtils to be aligned with Jupiter.

When it comes to 1, I'm a bit undecided – mainly because I haven't had enough time to think about it yet. 😅

But anyway, here are our related issues:

• Annotations like @SetSystemProperty are not inherited #448
• Add @Inherited to annotations targeting ElementType.TYPE #456

[eeverman]

Thank you everyone for the great discussion!

I need to correct one key error in my original ticket:

If users are allowed to compose Pioneer annotations into their own custom annotations, then it is very easy for users to forget to include the @Inherited marker.

...and in fact, if the Pioneer annotations are all marked as @Inherited, as they are, the current findClosestEnclosingAnnotation method does find that annotation, even if a user composes it into a non-@Inherited annotation. I had so many permutations of characterization tests, I missed the importance of this one passing.

I think that takes the urgency of this issue down a few notches. I still feel like marking the extension registration annotations @Inherited shouldn't be required (and is kind of cheating?), but it does work and doesn't actually push any additional requirements onto users.

If JUnit doesn't require @Inherited but Pioneer does on its own annotations, and there are no adverse affects on users for requiring it, maybe it's OK?

[nipafx]

So I'm making this change... I remove all @Inherited annotations and all mentions thereof and update tests that should now discover non-inherited annotations. But one of them doesn't want to. 🤔 It turns out that PioneerAnnotationUtils::.findClosestEnclosingRepeatableAnnotations doesn't discover non-inherited repeatable annotations. Why? It calls Jupiter's AnnotationSupport::findRepeatableAnnotations, which says:

In addition, if the element is a class and the repeatable annotation is @Inherited, this method will search on superclasses first in order to support top-down semantics. The result is that this algorithm finds repeatable annotations that would be shadowed and therefore not visible according to Java's standard semantics for inherited, repeatable annotations, but most developers will naturally assume that all repeatable annotations in JUnit are discovered regardless of whether they are declared stand-alone, in a container, or as a meta-annotation (e.g., multiple declarations of @ExtendWith within a test class hierarchy).

Häh? 😕 So suddenly it does make a difference whether an annotations @Inherited or not?

(Btw, this is not just theory. The practical consequence is that environment variable and system property extensions no longer find annotations on super classes.)

[Michael1993]

Yes, that's the behaviour PioneerAnnotationUtils is (supposed to be) mimicking. There is a weird mismatch between JUnit's AnnotationSupport and how the @ExtendWith annotation gets discovered.

[nipafx]

I kept investigating this and successfully bent my brain into a pretzel. 🥨 I also now believe that the Jupiter user guide is a lying little hobbit. Remember this?

Registered extensions are inherited within test class hierarchies with top-down semantics. Similarly, extensions registered at the class-level are inherited at the method-level.

Then why does the test in Tests get executed?! 🤔

@Nested
class Tests extends TestSuper { }

@Disabled
@DisplayName("Super")
class TestSuper {

	@Test
	void testC() { }

}

Also, shouldn't the test nodes for both classes be named "Super" (given that the "extension" is "inherited")? 😠

This made me curious and I went looking for type-level annotations that are not inherited:

• in org.junit.jupiter.api: @DisplayName, @Order, @Nested
• in org.junit.jupiter.api.condition: all, some of which are repeatable

As for @Nested:

@Nested
class InOrder {

	@Nested @Order(1) class TestA { @Test void testA() { } }
	@Nested @Order(2) class TestB { @Test void testB() { } }
	@Nested class TestC extends Order0 { @Test void testC() { } }

	@Order(0) class Order0 { }
}

TestC does not appear before TestA (if it does on your machine, change the number on Order0 to 3 and observe that it stays in the same place).

[nipafx]

PS: My conclusion is that, in line with the JLS, not @Inherited annotations are not treated as being present.

[nipafx]

I am hence inclined to close this ticket. Will take votes on that until Monday afternoon. If we vote to kick it out, we can then release 2.0.0-RC1.

[nipafx]

For completeness' sake: lab/not-inherited

[jbduncan]

I am hence inclined to close this ticket. Will take votes on that until Monday afternoon. If we vote to kick it out, we can then release 2.0.0-RC1.

Sounds reasonable. But regardless of the outcome, let's raise an issue with JUnit 5 so they can prioritise a fix.

[beatngu13]

@nipafx thank you for investigating this further! 🙏 As I said earlier, I was undecided about removing @Inherited everywhere - in particular because of JLS and the loss of expressiveness.

Now, if we also don't want to tweak the algorithm of PioneerAnnotationUtils, I'm with @jbduncan on opening an issue in Jupiter. I'm a bit confused by Jupiter's behavior/documentation. 🤔 (Or maybe, we can once again summon @marcphilipp?)