Repository: netty/netty
Issue: Migrating to JUnit 5
Labels: help wanted

I propose that we start migrating the test suite for Netty 5 (the `master` branch) to JUnit 5.

JUnit 5 is more expressive, extensible, and composable in many ways, and it's better able to run tests in parallel.
It can also directly run JUnit 4 tests, using the vintage engine, which means we can do this in small steps; there's no need to migrate all tests all at once.

Once the JUnit 5 infrastructure is there, tests or modules can be migrated one by one, or be left as is for the time being. Regardless, new tests that are only added to the `master` branch can immediately use the new JUnit 5 test APIs.

Some benefits I anticipate are:

1. Some tests will become easier to write, because JUnit 5 is more capable at expressing nesting of tests, and parameterisations.
2. Builds should complete faster because JUnit 5 is better able to run tests in parallel. This is something that needs to be enabled explicitly, however, since it can cause latent test isolation failures to pop up.
3. JUnit 5 is extensible enough that something like `TimedOutTestsListener` won't need to live in the `netty-build` anymore.

A drawback I can think of is that forward porting changes to a JUnit 4 test in Netty `4.1`, to a migrated JUnit 5 test in `master`, will be more challenging and add to the workload of doing changes in `4.1`.

What do people think?

Issue comments:
Is there any reason that JUnit 5 cannot be added to Netty 4.1?

Considering test artifacts are not exported as part of the build process, it doesn't seem like it should pose a compatibility issue.
The testsuite is exposed so we can consume it . That said I would not consider this API

> Am 30.10.2020 um 21:16 schrieb Bennett Lynch <notifications@github.com>:
> 
> ﻿
> Is there any reason that JUnit 5 cannot be added to Netty 4.1?
> 
> Considering test artifacts are not exported as part of the build process, it doesn't seem like it should pose a compatibility issue.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or unsubscribe.

JUnit 5 is now enabled in both the `4.1` branch and the `master` branch.
- [x] `all` - no tests
- [x] `bom` - no tests
- [x] `buffer` - #11305
- [x] `codec` - #11306
- [x] `codec-dns` - #11307
- [x] `codec-haproxy` - #11308
- [x] `codec-http` - #11316
- [x] `codec-http2` - #11422
- [x] `codec-memcache` - #11310
- [ ] `codec-mqtt`
- [x] `codec-redis` - #11318
- [x] `codec-smtp` - #11309
- [x] `codec-socks` - #11314
- [x] `codec-stomp` - #11312
- [x] `codec-xml` - #11311
- [x] `common` - #11319
- [x] `dev-tools` - no tests
- [x] `example` - no tests
- [ ] `handler`
- [x] `handler-proxy` - #11313
- [x] `microbench` - #11443
- [ ] `resolver`
- [x] `resolver-dns` - #11326
- [x] `resolver-dns-native-macos`
- [x] `tarball` - no tests
- [x] `testsuite` - #11320 
- [x] `testsuite-autobahn` - no tests
- [x] `testsuite-http2` - no tests
- [x] `testsuite-native`
- [x] `testsuite-native-image` - no tests
- [x] `testsuite-native-image-client` - no tests
- [x] `testsuite-native-image-client-runtime-init` - no tests
- [ ] `testsuite-osgi` - https://github.com/ops4j/org.ops4j.pax.exam2/issues/951
- [x] `testsuite-shading` - #11323
- [x] `transport` - #11315
- [x] `transport-blockhound-tests` - #11322
- [x] `transport-native-epoll` - #11320 #11442
- [x] `transport-native-kqueue` - #11320 
- [x] `transport-native-unix-common` - #11321
- [x] `transport-native-unix-common-tests` - #11320 
- [ ] `transport-sctp` - #11325
I hope that's a good start for now! I have pushed all my progress so far, and I will probably pick up on this again later today.
The `handler` module is proving problematic to migrate - it heavily uses class-scoped parameterized tests.

Pull request review comments:

Patch that resolved the issue:
diff --git a/microbench/pom.xml b/microbench/pom.xml
index 18aca81bcaa..32a26bfb089 100644
--- a/microbench/pom.xml
+++ b/microbench/pom.xml
@@ -185,16 +185,6 @@
       <artifactId>junit-jupiter-engine</artifactId>
       <scope>compile</scope>
     </dependency>
-    <dependency>
-      <groupId>org.junit.vintage</groupId>
-      <artifactId>junit-vintage-engine</artifactId>
-      <scope>compile</scope>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <scope>compile</scope>
-    </dependency>
     <dependency>
       <groupId>org.openjdk.jmh</groupId>
       <artifactId>jmh-core</artifactId>
diff --git a/pom.xml b/pom.xml
index 512965f98a0..d2d22be0960 100644
--- a/pom.xml
+++ b/pom.xml
@@ -886,18 +886,6 @@
         <version>${junit.version}</version>
         <scope>test</scope>
       </dependency>
-      <dependency>
-        <groupId>org.junit.vintage</groupId>
-        <artifactId>junit-vintage-engine</artifactId>
-        <version>${junit.version}</version>
-        <scope>test</scope>
-      </dependency>
-      <dependency>
-        <groupId>junit</groupId>
-        <artifactId>junit</artifactId>
-        <version>4.13.1</version>
-        <scope>test</scope>
-      </dependency>
       <dependency>
         <groupId>${project.groupId}</groupId>
         <artifactId>netty-build-common</artifactId>
@@ -1030,16 +1018,6 @@
       <artifactId>junit-jupiter-params</artifactId>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>org.junit.vintage</groupId>
-      <artifactId>junit-vintage-engine</artifactId>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>${project.groupId}</groupId>
       <artifactId>netty-build-common</artifactId>
@@ -1148,6 +1126,12 @@
                   <classQualifiedName>io.netty.util.internal.InternalThreadLocalMap</classQualifiedName>
                   <justification>Ignore cache padding.</justification>
                 </item>
+                <item>
+                  <ignore>true</ignore>
+                  <code>java.method.removed</code>
+                  <old>method java.lang.String io.netty.testsuite.util.TestUtils::testMethodName(org.junit.rules.TestName)</old>
+                  <justification>This should be test-only, and we're removing support for JUnit 4.</justification>
+                </item>
               </differences>
             </revapi.differences>
           </analysisConfiguration>
diff --git a/transport-native-unix-common-tests/pom.xml b/transport-native-unix-common-tests/pom.xml
index 0f56941dbcc..5743bdc0f9f 100644
--- a/transport-native-unix-common-tests/pom.xml
+++ b/transport-native-unix-common-tests/pom.xml
@@ -54,15 +54,5 @@
       <artifactId>junit-jupiter-engine</artifactId>
       <scope>compile</scope>
     </dependency>
-    <dependency>
-      <groupId>org.junit.vintage</groupId>
-      <artifactId>junit-vintage-engine</artifactId>
-      <scope>compile</scope>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <scope>compile</scope>
-    </dependency>
   </dependencies>
 </project>
diff --git a/transport-native-unix-common-tests/src/main/java/io/netty/channel/unix/tests/SocketTest.java b/transport-native-unix-common-tests/src/main/java/io/netty/channel/unix/tests/SocketTest.java
index 115ab353958..c33afc2759b 100644
--- a/transport-native-unix-common-tests/src/main/java/io/netty/channel/unix/tests/SocketTest.java
+++ b/transport-native-unix-common-tests/src/main/java/io/netty/channel/unix/tests/SocketTest.java
@@ -17,10 +17,10 @@
 
 import io.netty.channel.unix.Buffer;
 import io.netty.channel.unix.Socket;
-import org.junit.AssumptionViolatedException;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.opentest4j.TestAbortedException;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
@@ -127,10 +127,10 @@ public void testRawOpt() throws IOException {
     }
 
     protected int level() {
-        throw new AssumptionViolatedException("Not supported");
+        throw new TestAbortedException("Not supported");
     }
 
     protected int optname() {
-        throw new AssumptionViolatedException("Not supported");
+        throw new TestAbortedException("Not supported");
     }
 }


Test patch:
diff --git a/codec/src/test/resources/io/netty/handler/codec/xml/sample-04.xml b/codec/src/test/resources/io/netty/handler/codec/xml/sample-04.xml
index 8a75afccd6b..22b9d702b31 100644
--- a/codec/src/test/resources/io/netty/handler/codec/xml/sample-04.xml
+++ b/codec/src/test/resources/io/netty/handler/codec/xml/sample-04.xml
@@ -210,12 +210,6 @@
 
   <dependencies>
     <!-- Testing frameworks and related dependencies -->
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <version>4.12</version>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>org.easymock</groupId>
       <artifactId>easymock</artifactId>
diff --git a/testsuite-native/pom.xml b/testsuite-native/pom.xml
index 8f4c534ad58..afba034fb3d 100644
--- a/testsuite-native/pom.xml
+++ b/testsuite-native/pom.xml
@@ -45,14 +45,6 @@
       <groupId>org.junit.jupiter</groupId>
       <artifactId>junit-jupiter-engine</artifactId>
     </dependency>
-    <dependency>
-      <groupId>org.junit.vintage</groupId>
-      <artifactId>junit-vintage-engine</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-    </dependency>
   </dependencies>
   <profiles>
     <profile>
diff --git a/testsuite-shading/pom.xml b/testsuite-shading/pom.xml
index 535552985ab..49d0f00f1c3 100644
--- a/testsuite-shading/pom.xml
+++ b/testsuite-shading/pom.xml
@@ -200,14 +200,6 @@
       <groupId>org.junit.jupiter</groupId>
       <artifactId>junit-jupiter-engine</artifactId>
     </dependency>
-    <dependency>
-      <groupId>org.junit.vintage</groupId>
-      <artifactId>junit-vintage-engine</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-    </dependency>
   </dependencies>
   <profiles>
     <profile>
diff --git a/testsuite/pom.xml b/testsuite/pom.xml
index f6dc457b86c..6e7813c9103 100644
--- a/testsuite/pom.xml
+++ b/testsuite/pom.xml
@@ -94,16 +94,6 @@
       <artifactId>junit-jupiter-params</artifactId>
       <scope>compile</scope>
     </dependency>
-    <dependency>
-      <groupId>org.junit.vintage</groupId>
-      <artifactId>junit-vintage-engine</artifactId>
-      <scope>compile</scope>
-    </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <scope>compile</scope>
-    </dependency>
     <dependency>
       <groupId>org.hamcrest</groupId>
       <artifactId>hamcrest-library</artifactId>
diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/socket/DatagramUnicastIPv6Test.java b/testsuite/src/main/java/io/netty/testsuite/transport/socket/DatagramUnicastIPv6Test.java
index 2235747e91a..308669c6f5a 100644
--- a/testsuite/src/main/java/io/netty/testsuite/transport/socket/DatagramUnicastIPv6Test.java
+++ b/testsuite/src/main/java/io/netty/testsuite/transport/socket/DatagramUnicastIPv6Test.java
@@ -18,8 +18,8 @@
 import io.netty.channel.socket.InternetProtocolFamily;
 import io.netty.util.internal.PlatformDependent;
 import io.netty.util.internal.SuppressJava6Requirement;
-import org.junit.AssumptionViolatedException;
 import org.junit.jupiter.api.BeforeAll;
+import org.opentest4j.TestAbortedException;
 
 import java.io.IOException;
 import java.net.StandardProtocolFamily;
@@ -38,7 +38,7 @@ public static void assumeIpv6Supported() {
             Channel channel = SelectorProvider.provider().openDatagramChannel(StandardProtocolFamily.INET6);
             channel.close();
         } catch (UnsupportedOperationException e) {
-           throw new AssumptionViolatedException("IPv6 not supported", e);
+           throw new TestAbortedException("IPv6 not supported", e);
         } catch (IOException ignore) {
             // Ignore
         }
diff --git a/testsuite/src/main/java/io/netty/testsuite/util/TestUtils.java b/testsuite/src/main/java/io/netty/testsuite/util/TestUtils.java
index eeb37f2f9f5..f1192def7c5 100644
--- a/testsuite/src/main/java/io/netty/testsuite/util/TestUtils.java
+++ b/testsuite/src/main/java/io/netty/testsuite/util/TestUtils.java
@@ -21,7 +21,6 @@
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
 import org.junit.jupiter.api.TestInfo;
-import org.junit.rules.TestName;
 import org.tukaani.xz.LZMA2Options;
 import org.tukaani.xz.XZOutputStream;
 
@@ -120,17 +119,6 @@ public String apply(Method method) {
         return testMethodName;
     }
 
-    /**
-     * Returns the method name of the current test.
-     */
-    public static String testMethodName(TestName testName) {
-        String testMethodName = testName.getMethodName();
-        if (testMethodName.contains("[")) {
-            testMethodName = testMethodName.substring(0, testMethodName.indexOf('['));
-        }
-        return testMethodName;
-    }
-
     public static void dump(String filenamePrefix) throws IOException {
 
         ObjectUtil.checkNotNull(filenamePrefix, "filenamePrefix");