junit4
junit4 copied to clipboard
junit-team
Validate actual rule object instead of Field#type for @Rule
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: https://github.com/testcontainers/testcontainers-java/issues/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 https://github.com/junit-team/junit4/pull/1020
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.
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.
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?
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
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.
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?
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.
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.
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 https://github.com/junit-team/junit4/issues/1720#issuecomment-939214966 or https://github.com/junit-team/junit4/issues/1720#issuecomment-938598851 (or at least commenting on why those approaches are fundamentally unworkable).
@kcooney , the fix at junit4 side is trivial (see https://github.com/junit-team/junit4/pull/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.
it's slightly more involved, as exceptions should be thrown before or after that.
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.
I'm not convinced that is the case. Couldn't Testcontainers use its own interface (e.g. TestcontainersResource) and have separate testing framework specific artifacts that take care of the lifecycle of these resources? For example, there could be a TestRule for JUnit 4 and an Extension for JUnit Jupiter.
@marcphilipp that's exactly the purpose of the issue - currently JUnit 4 requires that the rule fields / methods should be declared with a type which extends TestRule.
One of the sad consequences of TestRule in the field type is that TestRule methods appear in autocomplete for the end-users even though the end-users should never call TestRule methods directly.
@panchenko What I meant was sth. along these lines:
JUnit 4
public class SomeTests {
@Rule TestRule rule = new TestcontainersRule();
Network network = new Network();
GenericContainer container = new GenericContainer(...);
@Test public void test() { ... }
}
JUnit Jupiter
@ExtendWith(TestcontainersExtension.class)
public class SomeTests {
Network network = new Network();
GenericContainer container = new GenericContainer(...);
@Test public void test() { ... }
}
Would be easier to implement if passing containers to the rule/extension constructor:
Network network = new Network();
GenericContainer container = new GenericContainer(...);
@RegisterExtension
TestcontainersExtension extension = new TestcontainersExtension(network, container);
Slightly more code to write, also a breaking change, but looks good.
There are certainly some details to be worked out which is why I tried to "summon" @bsideup and @rnorth above.
@Rule TestRule rule = new TestcontainersRule();
^^ This is sad because uses might try calling methods on rule object :)
This is sad because uses might try calling methods on rule object :)
I raised that concern back when TestRule was introduced, but, in practice, that hasn't really been an issue. It's hard to accidentally call apply().
Having rule classes directly implement TestRule also allowed them to be ordered via RuleChain and allows custom rules to explicitly compose rules (in which case users actually do want to call apply()).
@marcphilipp There are indeed plans to remove the dependency to JUnit4 from the core of Testcontainers and bring JUnit4 support into a module, similar to how we do it for JUnit5 or Spock.
That being said, we currently don't have an ETA for this, since it is a slightly more involved and potentially breaking change on our side, while ultimately not bringing any new functionality.
For the time being, I'd suggest reconsidering if having JUnit4 on the classpath is a fundamental problem after all.
