[GitHub] [kafka] chia7712 commented on a change in pull request #9874: KAFKA-7340: Migrate clients module to JUnit 5

[GitBox <gi...@apache.org>]

chia7712 commented on a change in pull request #9874:
URL: https://github.com/apache/kafka/pull/9874#discussion_r556401268



##########
File path: clients/src/test/java/org/apache/kafka/clients/MetadataTest.java
##########
@@ -347,28 +348,28 @@ public void testUpdateLastEpoch() {
         // Metadata with newer epoch is handled
         metadataResponse = RequestTestUtils.metadataUpdateWith("dummy", 1, Collections.emptyMap(), Collections.singletonMap("topic-1", 1), _tp -> 10);
         metadata.updateWithCurrentRequestVersion(metadataResponse, false, 1L);
-        assertOptional(metadata.lastSeenLeaderEpoch(tp), leaderAndEpoch -> assertEquals(leaderAndEpoch.intValue(), 10));
+        assertOptional(metadata.lastSeenLeaderEpoch(tp), leaderAndEpoch -> Assertions.assertEquals(leaderAndEpoch.intValue(), 10));

Review comment:
       the prefix ```Assertions.``` is unnecessary.

##########
File path: clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorTest.java
##########
@@ -2128,7 +2116,7 @@ private void createAndCheckClientAuthenticationFailure(SecurityProtocol security
             String mechanism, String expectedErrorMessage) throws Exception {
         ChannelState finalState = createAndCheckClientConnectionFailure(securityProtocol, node);
         Exception exception = finalState.exception();
-        assertTrue("Invalid exception class " + exception.getClass(), exception instanceof SaslAuthenticationException);
+        assertTrue(exception instanceof SaslAuthenticationException, "Invalid exception class " + exception.getClass());

Review comment:
       Personally, checking the class type is more readable to me. Also, it can generate meaningful error message (the custom message can be removed). If this comment is valid, we can address it by another PR (as there are a lot of occurances)

##########
File path: clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
##########
@@ -1240,39 +1284,47 @@ private Selector createSelector(Map<String, Object> sslClientConfigs, final Inte
         return selector;
     }
 
-    private NioEchoServer createEchoServer(ListenerName listenerName, SecurityProtocol securityProtocol) throws Exception {
-        return NetworkTestUtils.createEchoServer(listenerName, securityProtocol, new TestSecurityConfig(sslServerConfigs), null, time);
+    private NioEchoServer createEchoServer(Args args, ListenerName listenerName, SecurityProtocol securityProtocol) throws Exception {
+        return NetworkTestUtils.createEchoServer(listenerName, securityProtocol, new TestSecurityConfig(args.sslServerConfigs), null, time);
     }
 
-    private NioEchoServer createEchoServer(SecurityProtocol securityProtocol) throws Exception {
-        return createEchoServer(ListenerName.forSecurityProtocol(securityProtocol), securityProtocol);
+    private NioEchoServer createEchoServer(Args args, SecurityProtocol securityProtocol) throws Exception {
+        return createEchoServer(args, ListenerName.forSecurityProtocol(securityProtocol), securityProtocol);
     }
 
-    private Map<String, Object> getTrustingConfig(CertStores certStores, CertStores peerCertStores) {
-        Map<String, Object> configs = certStores.getTrustingConfig(peerCertStores);
-        configs.putAll(sslConfigOverrides);
-        return configs;
+    private Selector createSelector(Args args) {
+        LogContext logContext = new LogContext();
+        ChannelBuilder channelBuilder = new SslChannelBuilder(Mode.CLIENT, null, false, logContext);
+        channelBuilder.configure(args.sslClientConfigs);
+        selector = new Selector(5000, new Metrics(), time, "MetricGroup", channelBuilder, logContext);
+        return selector;
     }
 
-    private void verifySslConfigs() throws Exception {
-        server = createEchoServer(SecurityProtocol.SSL);
-        createSelector(sslClientConfigs);
+    private void verifySslConfigs(Args args) throws Exception {
+        server = createEchoServer(args, SecurityProtocol.SSL);
+        createSelector(args.sslClientConfigs);
         InetSocketAddress addr = new InetSocketAddress("localhost", server.port());
         String node = "0";
         selector.connect(node, addr, BUFFER_SIZE, BUFFER_SIZE);
         NetworkTestUtils.checkClientConnection(selector, node, 100, 10);
     }
 
-    public void verifySslConfigsWithHandshakeFailure() throws Exception {
-        server = createEchoServer(SecurityProtocol.SSL);
-        createSelector(sslClientConfigs);
+    public void verifySslConfigsWithHandshakeFailure(Args args) throws Exception {

Review comment:
       how about ```private```?

##########
File path: clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java
##########
@@ -366,7 +363,7 @@ public void testFileAsStringSimpleFile() throws IOException {
      * Test to read content of named pipe as string. As reading/writing to a pipe can block,
      * timeout test after a minute (test finishes within 100 ms normally).
      */
-    @Test(timeout = 60 * 1000)
+    @Test @Timeout(60)

Review comment:
       How about moving ```Timeout``` to separate line?

##########
File path: clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinatorTest.java
##########
@@ -110,16 +108,15 @@
 import static org.apache.kafka.common.utils.Utils.mkEntry;
 import static org.apache.kafka.common.utils.Utils.mkMap;
 import static org.apache.kafka.test.TestUtils.toSet;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
-
-@RunWith(value = Parameterized.class)
-public class ConsumerCoordinatorTest {
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public abstract class ConsumerCoordinatorTest {

Review comment:
       Nice idea of dealing with parameterized test :)




----------------------------------------------------------------
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:
users@infra.apache.org