Private lifecycle methods should cause an exception

[juriad]

The JavaDoc contains the following sentences:

@BeforeEach methods must have a void return type, must not be private, and must not be static. @BeforeEach methods are inherited from superclasses as long as they are not overridden. Furthermore, @BeforeEach methods from superclasses will be executed before @BeforeEach methods in subclasses.

Private methods are executed, even though they should not be (?). The JavaDoc prohibits private methods but does not explain what will happen.

Java defines overriding methods as: https://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.4.8.1, the JavaDoc uses the term "overridden" with a different meaning than Java. JLS even says:

Note that methods are overridden or hidden on a signature-by-signature basis. If, for example, a class declares two public methods with the same name (§8.4.9), and a subclass overrides one of them, the subclass still inherits the other method.

In the previous issue #2390, it was suggested to also include mention of shadowing - which I think is also wrong. JLS already defines shadowing - https://docs.oracle.com/javase/specs/jls/se8/html/jls-6.html#jls-6.4.1.

Both instances should be either fixed to be compliant with the documentation, or the documentation should be updated.

Other annotations (@BeforeAll, @AfterAll, and @AfterEach) are expected to be affected too.

Steps to reproduce

Let's imagine two packages and a class in each:

package sup;

public class Sup {
    @BeforeEach
    MODSUP void before() {
        System.out.println("Before in Sup");
    }
}

package sub;
import sup.Sup;

public class Sub extends Sup {
    @BeforeEach
    void before() {
        System.out.println("Before in Sub");
    }

    @Test
    MODSUB void test() {
        System.out.println("Test");
    }
}

We can then write a table of expected behavior based on the modifiers:

Sub \ Sup	public	protected	none	private
public	Before in SubTest	Before in SubTest	Before in SupBefore in SubTest (*)	Before in SubTest
protected	compilation error	Before in SubTest	Before in SupBefore in SubTest (*)	Before in SubTest
none	compilation error	compilation error	Before in SupBefore in SubTest (*)	Before in SubTest
private	compilation error	compilation error	Before in SupTest (*)	Test (*)

The combinations with (*) instead produce:

Before in Sub
Test

Context

• Used versions (Jupiter/Vintage/Platform): 5.8.2
• Build Tool/IDE: Gradle, Java 11

[sbrannen]

Quoting my previous feedback in #2390 (comment):

With regard to private methods, those should not be executed. So that sounds like a bug.

With regard to the use of the term "overridden", the documentation should likely be updated to say "overridden or shadowed" [although the term "shadowed" may be misleading with regard to the definition of the term in the JLS].

[marcphilipp]

With regard to private methods, those should not be executed. So that sounds like a bug.

Team decision: Throw an exception when encountering them similar to what is done when they're static.

With regard to the use of the term "overridden", the documentation should likely be updated to say "overridden or shadowed" [although the term "shadowed" may be misleading with regard to the definition of the term in the JLS].

Team decision: Clarify documentation.

[rschmitt]

FYI, we did a quick search of Amazon's codebase and found 5,787 results for using @BeforeEach with a private method. This change may have had more impact than you expected.

[marcphilipp]

@rschmitt Thanks for letting us know! However, the new behavior aligns with what was already documented and the required changes are minimal. There are tools to help with large-scale refactorings such as https://github.com/openrewrite/rewrite.

[rschmitt]

However, the new behavior aligns with what was already documented

Nevertheless, it is a breaking change; compatibility is determined by actual behavior, not by documented behavior. Is this change strategically important for you, e.g. will it dramatically simplify JUnit's internals or make it easier to ship improvements in the future? Or was it done solely to match the documentation? I'll note that you could have changed the documentation as easily as changing the code, and documentation changes never break anyone.

Personally, I always declare test and lifecycle methods public, because that's the only option that makes sense in terms of the visibility rules of the language: the methods have to be called by an unrelated class, so either they have to be public or they have to be called by extralinguistic means such as core reflection or runtime bytecode generation, and at that point I don't see the value in splitting hairs about private vs protected vs default visibility; they all seem equally fragile to me.

In addition, the JDK maintainers are increasingly shutting off access to any capabilities that "affect platform and application integrity" unless the application opts in to them by passing a JVM flag. Will setAccessible(true) be restricted in the future? I don't know, and I don't want to find out, so I always put in the "redundant" public modifier. I see this change as the worst of both worlds: private lifecycle methods were broken without (to my knowledge) a deprecation period, but the similarly fragile non-public alternatives have been left in place.

the required changes are minimal

Not for me. In our build system, we have modeled JUnit 5 as a single major version, 5.x, and we push out the latest version to the whole company all at once -- I'm talking hundreds of thousands of Java packages that use JUnit 5. JUnit 5 has historically been so good about backwards compatibility that this is the first time in over five years that we have been unable to internally release the latest version of JUnit 5. Furthermore, I can't just refactor Amazon's entire code base to facilitate this change. That simply isn't how it works here, and even if it were I would have more important things to do with that capability. So far, we have dealt with this by patching JUnit 5 internally to revert this change.

If you'd like, we can work together in the future to catch these types of breaking changes before they are released, or to empirically assess the impact of slightly breaking changes that it may be possible to get away with. We have the ability to dryrun build pre-release versions of JUnit, which will immediately build and test over 15,000 Java packages against whichever JUnit version you like, and we can probably have the results for you in a day or two. Would this be valuable to you?

[marcphilipp]

@rschmitt Thanks for your detailed response. We'll discuss it internally and will get back to you.

[bjmi]

Fortunately we had to change only ~70 test classes in our code base that allows us to update to JUnit 5.9.
Nevertheless we like the ability to have private setup or teardown methods as we almost have no inheritance in test classes esp. no inheritance in setup and teardown methods. Thus the most obvious thing was to make them private and encapsulate them from other tests.
A similar counterexample are private @TempDir fields that were allowed eventually (#2687).

[marcphilipp]

Team decision: Revert the validation but document private lifecycle methods as being discouraged in #3242.