Validate actual rule object instead of Field#type for @Rule

[vlsi]

Currently, JUnit validates field's type, and it wants the declaration to implement TestRule.

Is there a reason for that? What if JUnit4 validated the actual value rather than field type?

The validation makes it harder to implement code that supports both JUnit4 and JUnit5.

Sample issue: testcontainers/testcontainers-java#970 (/cc @bsideup, @baev)

TL;DR: Testcontainers has to implement junit4-specific interfaces in Testcontainers APIs, thus Testcontainers forces everybody to have JUnit4 on the classpath which is sad.

https://github.com/testcontainers/testcontainers-java/blob/aa273b5c0136d8bf8d9eb308040b30006ad98144/core/src/main/java/org/testcontainers/containers/Network.java#L20

public interface Network extends AutoCloseable, TestRule { // <-- it would be so much better to be able to remove TestRule here
...

However, if Network extends TestRule is replaced with NetworkImpl implements Network, TestRule, then JUnit4 fails as follows:

@RunWith(Enclosed.class)
public class NetworkTest {

    public static class WithRules {

        @Rule
        public Network network = newNetwork();

Gradle Test Executor 1 > org.testcontainers.containers.NetworkTest > org.testcontainers.containers.NetworkTest$WithRules.initializationError FAILED
    org.junit.runners.model.InvalidTestClassError: Invalid test class 'org.testcontainers.containers.NetworkTest$WithRules':
      1. The @Rule 'network' must implement MethodRule or TestRule.
        at org.junit.runners.ParentRunner.validate(ParentRunner.java:525)
        at org.junit.runners.ParentRunner.<init>(ParentRunner.java:102)
        at org.junit.runners.BlockJUnit4ClassRunner.<init>(BlockJUnit4ClassRunner.java:84)
        at org.junit.runners.JUnit4.<init>(JUnit4.java:23)
        at org.junit.internal.builders.JUnit4Builder.runnerForClass(JUnit4Builder.java:10)
        at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:70)
        at org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:37)
        at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:70)
        at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:125)
        at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:111)
        at org.junit.runners.Suite.<init>(Suite.java:102)
        at org.junit.experimental.runners.Enclosed.<init>(Enclosed.java:31)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
        at org.junit.internal.builders.AnnotatedBuilder.buildRunner(AnnotatedBuilder.java:107)
        at org.junit.internal.builders.AnnotatedBuilder.runnerForClass(AnnotatedBuilder.java:86)
        at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:70)
        at org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:37)
        at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:70)
        at org.junit.internal.requests.ClassRequest.createRunner(ClassRequest.java:28)
        at org.junit.internal.requests.MemoizingRequest.getRunner(MemoizingRequest.java:19)
        at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:78)
        at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
        at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
        at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
        at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)

See also #1020

[panchenko]

It seems like the only reason: TestRule can be created by.a method, so such a check covers both.
Due to the way code is organized - class validation happens before execution, and invoking the method at that moment is not expected.

[vlsi]

What if the failure was delayed till the execution time?

[panchenko]

It's possible to implement it differently, the question is if it's worth modifying old behaviour in a project which is not actively developed at the moment (kind of maintenance mode).

Practically, It would be better designing it differently in testcontainers.
If at the moment there are reasons to keep JUnit4 dependency there, then as a workaround I would suggest building another jar for junit5 which relocates junit4 code into an internal package, keeping only a few referenced classes from it.
The gradle shadow plugin can help with this.

[kcooney]

It doesn't sound like changing JUnit's behavior in this way would resolve all (most?) of the issues that your customers are complaining about, since some of them don't want JUnit 4 x classes on the classpath at all.

Since you need to make a breaking change anyway, have you considered releasing two jars, one that provides extensions compatible with JUnit 5 and the other providing Rule versions of those extensions?

[vlsi]

The typical Java approach is to have an interface and multiple implementations: junit4, junit5. The problem is that junit4 forces interface to extend junit4-related class, so it forbids the idea of having a common api for the clients.

I am not sure if that is the only issue, however, it does look like junit4 issue to me

[kcooney]

From JUnit's perspective, the interface is TestRule. That interface defines the API that the test runner will call. Implementations include TemporaryFolder. So JUnit is also using the typical Java approach. 🤷‍♂️

You can still have a common interface for your users. Your JUnit4 implementation would implement your interface and TestRule (this can be done via a sub interface to hide the implementation class from your users if that is important to you). Yes, JUnit4 users of your library will see a different interface type than JUnit5 users, but that still seems like a reasonable approach .

While it may be possible to resolve this issue the way you want, your users would have to wait for a JUnit 4 x release, and feature development for JUnit4 is presently frozen.

[vlsi]

Does JUnit have a separate API-only jar so TestRule "API" can be used without bringing full JUnit to the classpath and without clients having multiple @test annotations?

[kcooney]

JUnit4 does not have API jars.

If people want to prevent use of JUnit 4 annotations in their tests, I'm guessing they could write an ErrorProne rule to check for that.

[vlsi]

JUnit4 does not have API jars.

That is why it is really sad to suppose that TestRule interface is API that every client must implement.
There are annotations (e.g. @Rule), so there's absolutely no reason to enforce users implement the interface, especially for field type.

If people want to prevent use of JUnit 4 annotations in their tests, I'm guessing they could write an ErrorProne rule to check for that.

Users would have to configure IDE manually to avoid suggesting junit4 test in a given project.

[kcooney]

The others on this thread are simply trying to suggest alternative solutions. Feature development for JUnit4 is presently frozen (as the maintainers that remain are focusing their efforts on JUnit 5) so currently the likelihood to get your proposed change implemented and released is low. That's not to say it wouldn't happen (and a pull request with a fix including tests may change some minds) but it's probably best for you to consider alternative solutions that don't involve a new release of JUnit 4.x.

I suggest exploring the ideas in #1720 (comment) or #1720 (comment) (or at least commenting on why those approaches are fundamentally unworkable).

[vlsi]

@kcooney , the fix at junit4 side is trivial (see #1721), and the "solutions" of shading junit4 or making extra artifacts would be non-trivial, especially when it comes to licensing and build configuration.

In fact, the users should never call TestRule and MethodRule methods directly. Those interfaces should be used by junit4 engine only, so it is really strange to enforce users (the one who create tests) to hardcode field types and method types to return MethodRule and TestRule. The better approach would be to have rules as interfaces with user-friendly methods, and it is junit4 engine that should identify which rule implements TestRule and call the needed SPI.

---

Just in case, I perfectly understand junit4 is dormant, however, the fix seems to be trivial, it breaks no tests (there are tests that expect @Rule int x=0 to fail, and the tests work as expected), so I see no reason to decline the PR.

[panchenko]

it's slightly more involved, as exceptions should be thrown before or after that.