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