chia7712 commented on a change in pull request #9874:
URL: https://github.com/apache/kafka/pull/9874#discussion_r556671677
##########
File path:
clients/src/test/java/org/apache/kafka/common/security/ssl/SslFactoryTest.java
##########
@@ -47,45 +46,38 @@
import org.apache.kafka.common.utils.Utils;
import org.apache.kafka.test.TestSslUtils;
import org.apache.kafka.common.network.Mode;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
import java.security.Security;
import java.util.Properties;
+import java.util.stream.Stream;
-@RunWith(value = Parameterized.class)
public class SslFactoryTest {
- private final String tlsProtocol;
-
- @Parameterized.Parameters(name = "tlsProtocol={0}")
- public static Collection<Object[]> data() {
- List<Object[]> values = new ArrayList<>();
- values.add(new Object[] {"TLSv1.2"});
- if (Java.IS_JAVA11_COMPATIBLE) {
- values.add(new Object[] {"TLSv1.3"});
- }
- return values;
- }
-
- public SslFactoryTest(String tlsProtocol) {
- this.tlsProtocol = tlsProtocol;
+ private static Stream<Arguments> parameters() {
Review comment:
It seems to me the parameterized test which is on following condition
should be separated by subclassing.
1. too many ```@ParameterizedTest``` (i.e it has a bunch of test cases)
2. few parameters (for example, only true/false or few enums)
The benefit is that it can make smaller changes if we want to format the
output for all @ParameterizedTest (I have filed a PR to junit 5 to avoid using
magic string https://github.com/junit-team/junit5/pull/2526).
> Subclassing is a bit less flexible once you want more than one parameter
You are right. Current code style is ok to me :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
