ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559234137



##########
File path: core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala
##########
@@ -154,11 +154,8 @@ abstract class BaseProducerSendTest extends 
KafkaServerTestHarness {
       assertEquals(3L, producer.send(record3, callback).get.offset, "Should 
have offset 3")
 
       // send a record with null topic should fail
-      assertThrows(classOf[IllegalArgumentException], () => {
-        val record4 = new ProducerRecord[Array[Byte], Array[Byte]](null, 
partition, "key".getBytes(StandardCharsets.UTF_8),
-          "value".getBytes(StandardCharsets.UTF_8))
-        producer.send(record4, callback)
-      })
+      assertThrows(classOf[IllegalArgumentException], () => new 
ProducerRecord[Array[Byte], Array[Byte]](null, partition, 
"key".getBytes(StandardCharsets.UTF_8),
+        "value".getBytes(StandardCharsets.UTF_8)))

Review comment:
       We should probably delete this and test the constructor via 
KafkaProducerTest instead.

##########
File path: core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala
##########
@@ -92,8 +92,7 @@ class DynamicConfigChangeTest extends KafkaServerTestHarness {
   }
 
   private def testQuotaConfigChange(user: String, clientId: String, 
rootEntityType: String, configEntityName: String): Unit = {
-    
assertTrue(this.servers.head.dynamicConfigHandlers.contains(rootEntityType) ,
-               rootEntityType + "Should contain a ConfigHandler for ")
+    
assertTrue(this.servers.head.dynamicConfigHandlers.contains(rootEntityType), 
rootEntityType + "Should contain a ConfigHandler for ")

Review comment:
       For this one, I meant that the `rootEntityType` should probably be after 
the "Should contain...", right?

##########
File path: 
core/src/test/scala/integration/kafka/api/ConsumerTopicCreationTest.scala
##########
@@ -17,49 +17,58 @@
 
 package kafka.api
 
-import java.lang.{Boolean => JBoolean}
-import java.time.Duration
-import java.util
-import java.util.Collections
-
 import kafka.server.KafkaConfig
 import kafka.utils.TestUtils
 import org.apache.kafka.clients.admin.NewTopic
 import org.apache.kafka.clients.consumer.ConsumerConfig
 import org.apache.kafka.clients.producer.{ProducerConfig, ProducerRecord}
-import org.junit.Assert._
-import org.junit.Test
-import org.junit.runner.RunWith
-import org.junit.runners.Parameterized
-import org.junit.runners.Parameterized.Parameters
+import org.junit.jupiter.api.Assertions._
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.{Arguments, MethodSource}
+
+import java.lang.{Boolean => JBoolean}
+import java.time.Duration
+import java.util
+import java.util.Collections
 
 /**
  * Tests behavior of specifying auto topic creation configuration for the 
consumer and broker
  */
-@RunWith(value = classOf[Parameterized])
-class ConsumerTopicCreationTest(brokerAutoTopicCreationEnable: JBoolean, 
consumerAllowAutoCreateTopics: JBoolean) extends IntegrationTestHarness {
+class ConsumerTopicCreationTest extends IntegrationTestHarness {
   override protected def brokerCount: Int = 1
 
   val topic_1 = "topic-1"
   val topic_2 = "topic-2"
   val producerClientId = "ConsumerTestProducer"
   val consumerClientId = "ConsumerTestConsumer"
 
-  // configure server properties
-  this.serverConfig.setProperty(KafkaConfig.ControlledShutdownEnableProp, 
"false") // speed up shutdown
-  this.serverConfig.setProperty(KafkaConfig.AutoCreateTopicsEnableProp, 
brokerAutoTopicCreationEnable.toString)
+  @BeforeEach
+  override def setUp(): Unit = {
+    // junit 5 can't make parametered BeforeEach so we override setup to do 
nothing as we do setup cluster in test case
+  }

Review comment:
       This is not ideal. Since we have a single test, maybe we can do the 
parameterization manually within the test.

##########
File path: 
core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala
##########
@@ -19,39 +19,34 @@ package kafka.log
 
 import java.io.File
 import java.util.Properties
-
 import kafka.api.KAFKA_0_11_0_IV0
 import kafka.api.{KAFKA_0_10_0_IV1, KAFKA_0_9_0}
 import kafka.server.KafkaConfig
 import kafka.server.checkpoints.OffsetCheckpointFile
 import kafka.utils._
 import org.apache.kafka.common.TopicPartition
 import org.apache.kafka.common.record._
-import org.junit.Assert._
-import org.junit._
-import org.junit.runner.RunWith
-import org.junit.runners.Parameterized
-import org.junit.runners.Parameterized.Parameters
+import org.junit.jupiter.api.Assertions._
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.{Arguments, MethodSource}
 
 import scala.collection._
 import scala.jdk.CollectionConverters._
 
 /**
  * This is an integration test that tests the fully integrated log cleaner
  */
-@RunWith(value = classOf[Parameterized])
-class LogCleanerParameterizedIntegrationTest(compressionCodec: String) extends 
AbstractLogCleanerIntegrationTest {
+class LogCleanerParameterizedIntegrationTest extends 
AbstractLogCleanerIntegrationTest {
 
-  val codec: CompressionType = CompressionType.forName(compressionCodec)
   val time = new MockTime()
 
   val topicPartitions = Array(new TopicPartition("log", 0), new 
TopicPartition("log", 1), new TopicPartition("log", 2))
 
-
-  @Test
-  def cleanerTest(): Unit = {
+  @ParameterizedTest
+  @MethodSource(Array("all"))

Review comment:
       Since there are many methods, would it be better to use a provider to 
make this strongly typed?

##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala
##########
@@ -353,11 +353,11 @@ class ClientQuotaManagerTest extends 
BaseClientQuotaManagerTest {
       metrics.removeSensor("ProduceThrottleTime-:client1")
       // should not throw an exception even if the throttle time sensor does 
not exist.
       val throttleTime = maybeRecord(clientQuotaManager, "ANONYMOUS", 
"client1", 10000)
-      assertTrue("Should be throttled", throttleTime > 0)
+      assertTrue(throttleTime > 0, "Should be throttled")
       // the sensor should get recreated
       val throttleTimeSensor = 
metrics.getSensor("ProduceThrottleTime-:client1")
-      assertTrue("Throttle time sensor should exist", throttleTimeSensor != 
null)
-      assertTrue("Throttle time sensor should exist", throttleTimeSensor != 
null)
+      assertTrue(throttleTimeSensor != null, "Throttle time sensor should 
exist")
+      assertTrue(throttleTimeSensor != null, "Throttle time sensor should 
exist")

Review comment:
       Seems like this was marked as resolved, but not updated? There are many 
similar examples in this file.

##########
File path: core/src/test/scala/unit/kafka/log/LogTest.scala
##########
@@ -2862,13 +2850,11 @@ class LogTest {
                                     magicValue = magic, codec = compression,
                                     baseOffset = firstOffset)
 
-      withClue(s"Magic=$magic, compressionType=$compression") {
         val exception = assertThrows(classOf[UnexpectedAppendOffsetException], 
() => log.appendAsFollower(records = batch))
-        assertEquals(s"Magic=$magic, compressionType=$compression, 
UnexpectedAppendOffsetException#firstOffset",
-                     firstOffset, exception.firstOffset)
-        assertEquals(s"Magic=$magic, compressionType=$compression, 
UnexpectedAppendOffsetException#lastOffset",
-                     firstOffset + 2, exception.lastOffset)
-      }
+        assertEquals(firstOffset, exception.firstOffset,

Review comment:
       Indentation (`withClue` was removed).

##########
File path: 
core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala
##########
@@ -185,12 +178,9 @@ class 
LogCleanerParameterizedIntegrationTest(compressionCodec: String) extends A
     checkLogAfterAppendingDups(log, startSize, appends2)
   }
 
-  @Test
-  def testCleaningNestedMessagesWithMultipleVersions(): Unit = {
-    // zstd compression is not supported with older message formats
-    if (codec == CompressionType.ZSTD)
-      return
-
+  @ParameterizedTest
+  @MethodSource(Array("excludeZstd")) // zstd compression is not supported 
with older message formats
+  def testCleaningNestedMessagesWithMultipleVersions(codec: CompressionType): 
Unit = {

Review comment:
       I think this should be `testCleaningNestedMessagesWithV0AndV1`.

##########
File path: 
core/src/test/scala/unit/kafka/admin/TopicCommandWithAdminClientTest.scala
##########
@@ -82,17 +78,18 @@ class TopicCommandWithAdminClientTest extends 
KafkaServerTestHarness with Loggin
     TestUtils.waitUntilMetadataIsPropagated(servers, topicName, partition = 0, 
timeout)
   }
 
-  @Before
-  def setup(): Unit = {
+  @BeforeEach
+  def setup(info: TestInfo): Unit = {
     // create adminClient
     val props = new Properties()
     props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG, brokerList)
     adminClient = Admin.create(props)
     topicService = AdminClientTopicService(adminClient)
-    testTopicName = 
s"${testName.getMethodName}-${Random.alphanumeric.take(10).mkString}"
+    // the method name in junit 5 ends with "()"
+    testTopicName = s"${info.getDisplayName.replaceAll("\\(\\)", 
"")}-${Random.alphanumeric.take(10).mkString}"

Review comment:
       This was marked as resolved, but not updated.

##########
File path: 
core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerTest.scala
##########
@@ -664,21 +664,21 @@ class AclAuthorizerTest extends ZooKeeperTestHarness with 
BaseAuthorizerTest {
     val aclOnSpecificPrincipal = new AccessControlEntry(principal.toString, 
WildcardHost, WRITE, ALLOW)
     addAcls(aclAuthorizer, Set(aclOnSpecificPrincipal), resource)
 
-    assertEquals("acl on specific should not be returned for wildcard request",
-      0, getAcls(aclAuthorizer, wildcardPrincipal).size)
-    assertEquals("acl on specific should be returned for specific request",
-      1, getAcls(aclAuthorizer, principal).size)
-    assertEquals("acl on specific should be returned for different principal 
instance",
-      1, getAcls(aclAuthorizer, new KafkaPrincipal(principal.getPrincipalType, 
principal.getName)).size)
+    assertEquals(0,
+      getAcls(aclAuthorizer, wildcardPrincipal).size, "acl on specific should 
not be returned for wildcard request")
+    assertEquals(1,
+      getAcls(aclAuthorizer, principal).size, "acl on specific should be 
returned for specific request")
+    assertEquals(1,
+      getAcls(aclAuthorizer, new KafkaPrincipal(principal.getPrincipalType, 
principal.getName)).size, "acl on specific should be returned for different 
principal instance")
 
     removeAcls(aclAuthorizer, Set.empty, resource)
     val aclOnWildcardPrincipal = new 
AccessControlEntry(WildcardPrincipalString, WildcardHost, WRITE, ALLOW)
     addAcls(aclAuthorizer, Set(aclOnWildcardPrincipal), resource)
 
-    assertEquals("acl on wildcard should be returned for wildcard request",
-      1, getAcls(aclAuthorizer, wildcardPrincipal).size)
-    assertEquals("acl on wildcard should not be returned for specific request",
-      0, getAcls(aclAuthorizer, principal).size)
+    assertEquals(1,
+      getAcls(aclAuthorizer, wildcardPrincipal).size, "acl on wildcard should 
be returned for wildcard request")
+    assertEquals(0,
+      getAcls(aclAuthorizer, principal).size, "acl on wildcard should not be 
returned for specific request")

Review comment:
       It may be worth tweaking the formatting of the lines above.




----------------------------------------------------------------
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


Reply via email to