You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/01/10 21:37:20 UTC
[GitHub] [kafka] chia7712 opened a new pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
chia7712 opened a new pull request #9855:
URL: https://github.com/apache/kafka/pull/9855
issue: https://issues.apache.org/jira/browse/KAFKA-7341
This PR includes following changes.
1. replace ```org.junit.Assert``` by ```org.junit.jupiter.api.Assertions```
1. replace ```org.junit``` by ```org.junit.jupiter.api```
1. remove ```ScalaTest``` from core dependencies
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559302819
##########
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:
This is another comment that was seemingly resolved without a fix btw.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r557397651
##########
File path: core/src/test/scala/unit/kafka/server/epoch/LeaderEpochFileCacheTest.scala
##########
@@ -267,7 +267,7 @@ class LeaderEpochFileCacheTest {
cache.assign(epoch = 2, startOffset = 6); logEndOffset = 7
//When we update an epoch in the past with a different offset, the log has already reached
- //an inconsistent state. Our options are either to raise an error, ignore the new append,
+ //an inconsistent state. Our options are either to raise an error, Disabled the new append,
Review comment:
Seems like this was not intended.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559305059
##########
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 the last commit covers the cases I noticed.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 merged pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #9855:
URL: https://github.com/apache/kafka/pull/9855
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558976772
##########
File path: core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
##########
@@ -237,14 +237,9 @@ class ProducerFailureHandlingTest extends KafkaServerTestHarness {
createTopic(topicName, replicationFactor = numServers, topicConfig = topicProps)
val record = new ProducerRecord(topicName, null, "key".getBytes, "value".getBytes)
- try {
- producer3.send(record).get
- fail("Expected exception when producing to topic with fewer brokers than min.insync.replicas")
- } catch {
- case e: ExecutionException =>
- if (!e.getCause.isInstanceOf[NotEnoughReplicasException]) {
- fail("Expected NotEnoughReplicasException when producing to topic with fewer brokers than min.insync.replicas")
- }
+ val e = assertThrows(classOf[ExecutionException], () => producer3.send(record).get)
+ if (!e.getCause.isInstanceOf[NotEnoughReplicasException]) {
+ fail("Expected NotEnoughReplicasException when producing to topic with fewer brokers than min.insync.replicas")
}
Review comment:
Replace with `assertEquals`? Similar for the code below, it seems like one of the asserts can be used instead of `fail`.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#issuecomment-760205274
Quick update: I reviewed over 200 files, so I think I'll be able to complete this later today.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r557408197
##########
File path: build.gradle
##########
@@ -787,9 +787,7 @@ project(':core') {
testCompile libs.apachedsLdifPartition
testCompile libs.apachedsMavibotPartition
testCompile libs.apachedsJdbmPartition
- testCompile libs.junitJupiterApi
- testCompile libs.junitVintageEngine
- testCompile libs.scalatest
Review comment:
We should update `dependencies.gradle` to remove `scalatest` too, right?
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559301849
##########
File path: core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala
##########
@@ -325,11 +315,9 @@ class LogCleanerParameterizedIntegrationTest(compressionCodec: String) extends A
}
object LogCleanerParameterizedIntegrationTest {
- @Parameters
- def parameters: java.util.Collection[Array[String]] = {
- val list = new java.util.ArrayList[Array[String]]()
- for (codec <- CompressionType.values)
- list.add(Array(codec.name))
- list
- }
+ def all: java.util.stream.Stream[Arguments] =
+ java.util.Arrays.stream(CompressionType.values.map(codec => Arguments.of(codec)))
+
+ def excludeZstd: java.util.stream.Stream[Arguments] =
Review comment:
I will add some comments for this parameters.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559309116
##########
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:
The last commit rewrite the test by nested test and it seems to me that is more readable than dynamic test. WDYT?
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558465841
##########
File path: core/src/test/scala/unit/kafka/server/AlterUserScramCredentialsRequestTest.scala
##########
@@ -43,20 +41,27 @@ import scala.jdk.CollectionConverters._
* Also tests the Alter and Describe APIs for the case where credentials are successfully altered/described.
*/
class AlterUserScramCredentialsRequestTest extends BaseRequestTest {
+
+ private[this] var className = classOf[AlterCredentialsTest.TestPrincipalBuilderReturningAuthorized].getName
+
+ override def setUp(): Unit = {
+ // do nothing as we will setup cluster by 'before'
+ }
+
+ @BeforeEach
+ def before(info: TestInfo): Unit = {
+ if (info.getDisplayName.contains("NotAuthorized"))
Review comment:
It required to override the properties of starting server according to method name. You are right, current code is weird so I will separate the tests which need different props to another file.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559255133
##########
File path: core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala
##########
@@ -325,11 +315,9 @@ class LogCleanerParameterizedIntegrationTest(compressionCodec: String) extends A
}
object LogCleanerParameterizedIntegrationTest {
- @Parameters
- def parameters: java.util.Collection[Array[String]] = {
- val list = new java.util.ArrayList[Array[String]]()
- for (codec <- CompressionType.values)
- list.add(Array(codec.name))
- list
- }
+ def all: java.util.stream.Stream[Arguments] =
+ java.util.Arrays.stream(CompressionType.values.map(codec => Arguments.of(codec)))
+
+ def excludeZstd: java.util.stream.Stream[Arguments] =
Review comment:
Nit: even though this is technically true, it doesn't seem to capture the reason why we exclude zstd. It seems like we basically want to generate the compression types supported by V0 and V1 in this method. A name that reflects that would work better if we add more compression types in the future.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559182688
##########
File path: core/src/test/scala/unit/kafka/server/AlterUserScramCredentialsRequestTest.scala
##########
@@ -356,17 +320,17 @@ class AlterUserScramCredentialsRequestTest extends BaseRequestTest {
}
private def checkAllErrorsAlteringCredentials(resultsToCheck: util.List[AlterUserScramCredentialsResult], expectedError: Errors, contextMsg: String) = {
Review comment:
yep
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559676643
##########
File path: core/src/test/scala/unit/kafka/cluster/AssignmentStateTest.scala
##########
@@ -29,53 +29,53 @@ object AssignmentStateTest extends AbstractPartitionTest {
Arguments.of(
List[Integer](brokerId, brokerId + 1, brokerId + 2),
List[Integer](brokerId, brokerId + 1, brokerId + 2),
- List.empty[Integer], List.empty[Integer], Seq.empty[Int], false),
+ List.empty[Integer], List.empty[Integer], Seq.empty[Int], Boolean.box(false)),
Review comment:
I’ll keep it in mind.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559304098
##########
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:
please see the latest commit (https://github.com/apache/kafka/pull/9855/commits/1df45e40680e99c8f86efa85eaeb77c2df7f0bc3)
I will check all comments again.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#issuecomment-762136464
Will you go ahead and merge then?
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559302704
##########
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:
That requires to modify server configs "before" the ```@BeforeEach```. It seem to me the only way to address it is to separate the test class. WDYT?
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558473729
##########
File path: core/src/test/scala/unit/kafka/server/FetchRequestMaxBytesTest.scala
##########
@@ -58,11 +58,13 @@ class FetchRequestMaxBytesTest extends BaseRequestTest {
array
}
+ @BeforeEach
Review comment:
yep, that is a behavior change. see https://junit.org/junit5/docs/5.0.1/api/org/junit/jupiter/api/BeforeEach.html
`@BeforeEach methods are inherited from superclasses as long as they are not overridden. Furthermore, @BeforeEach methods from superclasses will be executed before @BeforeEach methods in subclasses.`
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558976772
##########
File path: core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
##########
@@ -237,14 +237,9 @@ class ProducerFailureHandlingTest extends KafkaServerTestHarness {
createTopic(topicName, replicationFactor = numServers, topicConfig = topicProps)
val record = new ProducerRecord(topicName, null, "key".getBytes, "value".getBytes)
- try {
- producer3.send(record).get
- fail("Expected exception when producing to topic with fewer brokers than min.insync.replicas")
- } catch {
- case e: ExecutionException =>
- if (!e.getCause.isInstanceOf[NotEnoughReplicasException]) {
- fail("Expected NotEnoughReplicasException when producing to topic with fewer brokers than min.insync.replicas")
- }
+ val e = assertThrows(classOf[ExecutionException], () => producer3.send(record).get)
+ if (!e.getCause.isInstanceOf[NotEnoughReplicasException]) {
+ fail("Expected NotEnoughReplicasException when producing to topic with fewer brokers than min.insync.replicas")
}
Review comment:
Replace with `assertEquals`?
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559309831
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1415,15 +1415,15 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
assertEquals(expectedEntityName, entity.fullSanitizedName)
}
+ def checkArgs(entityType: String, entityName: Option[String], otherArgs: Array[String]): Unit = {
Review comment:
Should this be `checkInvalidArgs`? Since it passes when `checkArgs` fails.
##########
File path: core/src/test/scala/integration/kafka/api/ConsumerTopicCreationTest.scala
##########
@@ -17,82 +17,88 @@
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.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 {
- override protected def brokerCount: Int = 1
+class ConsumerTopicCreationTest {
+ @ParameterizedTest
+ @MethodSource(Array("parameters"))
+ def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
+ val testCase = new ConsumerTopicCreationTest.TestCase(brokerAutoTopicCreationEnable, consumerAllowAutoCreateTopics)
+ testCase.setUp()
+ try testCase.test() finally testCase.tearDown()
+ }
+}
+
+object ConsumerTopicCreationTest {
+
+ class TestCase(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean) extends IntegrationTestHarness {
+ private[this] val topic_1 = "topic-1"
+ private[this] val topic_2 = "topic-2"
+ private[this] val producerClientId = "ConsumerTestProducer"
+ private[this] val consumerClientId = "ConsumerTestConsumer"
Review comment:
`private[this]` seems a bit unnecessary for a test, no? Seems like `private` is good enough.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558338474
##########
File path: core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala
##########
@@ -1228,10 +1223,10 @@ class PlaintextConsumerTest extends BaseConsumerTest {
assertEquals(20, timestampTopic1P1.timestamp)
assertEquals(Optional.of(0), timestampTopic1P1.leaderEpoch)
- assertEquals("null should be returned when message format is 0.9.0",
- null, timestampOffsets.get(new TopicPartition(topic2, 0)))
- assertEquals("null should be returned when message format is 0.9.0",
- null, timestampOffsets.get(new TopicPartition(topic2, 1)))
+ assertEquals(null, timestampOffsets.get(new TopicPartition(topic2, 0)),
Review comment:
We could use `assertNull` in a few of these cases.
##########
File path: core/src/test/scala/unit/kafka/utils/TestUtils.scala
##########
@@ -459,41 +459,9 @@ object TestUtils extends Logging {
* Check that the buffer content from buffer.position() to buffer.limit() is equal
*/
def checkEquals(b1: ByteBuffer, b2: ByteBuffer): Unit = {
- assertEquals("Buffers should have equal length", b1.limit() - b1.position(), b2.limit() - b2.position())
+ assertEquals(b1.limit() - b1.position(), b2.limit() - b2.position(), "Buffers should have equal length")
for(i <- 0 until b1.limit() - b1.position())
- assertEquals("byte " + i + " byte not equal.", b1.get(b1.position() + i), b2.get(b1.position() + i))
- }
-
- /**
- * Throw an exception if the two iterators are of differing lengths or contain
- * different messages on their Nth element
- */
- def checkEquals[T](expected: Iterator[T], actual: Iterator[T]): Unit = {
Review comment:
This was unused?
##########
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:
Why not use `getTestMethod` instead?
##########
File path: core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala
##########
@@ -1472,7 +1467,7 @@ class PlaintextConsumerTest extends BaseConsumerTest {
val fetchLead = consumer.metrics.get(new MetricName("records-lead", "consumer-fetch-manager-metrics", "", tags))
assertNotNull(fetchLead)
- assertTrue(s"The lead should be ${records.count}", records.count == fetchLead.metricValue())
+ assertEquals(fetchLead.metricValue().asInstanceOf[Double], records.count.asInstanceOf[Double], s"The lead should be ${records.count}")
Review comment:
`toDouble` should be used instead of `asInstanceOf[Double]` for numbers. `asInstanceOf` should only be used for casting. It seems that we don't even need to convert the second parameter though.
##########
File path: core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala
##########
@@ -361,7 +361,7 @@ class DynamicBrokerConfigTest {
try {
kafkaServer.config.dynamicConfig.addReconfigurables(kafkaServer)
} catch {
- case _: Throwable => // We are only testing authorizer reconfiguration, ignore any exceptions due to incomplete mock
+ case _: Throwable => // We are only testing authorizer reconfiguration, Disabled any exceptions due to incomplete mock
Review comment:
Accidental change.
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala
##########
@@ -337,8 +337,8 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
time.sleep(1000)
}
- assertEquals("Should be unthrottled since bursty sample has rolled over",
- 0, maybeRecord(clientQuotaManager, "ANONYMOUS", "unknown", 0))
+ assertEquals(0,
+ maybeRecord(clientQuotaManager, "ANONYMOUS", "unknown", 0), "Should be unthrottled since bursty sample has rolled over")
Review comment:
Nit: formatting looks weird.
##########
File path: core/src/test/scala/unit/kafka/server/ReplicaFetcherThreadTest.scala
##########
@@ -638,19 +638,19 @@ class ReplicaFetcherThreadTest {
thread.doWork()
assertEquals(1, mockNetwork.epochFetchCount)
assertEquals(1, mockNetwork.fetchCount)
- assertEquals("OffsetsForLeaderEpochRequest version.",
- 0, mockNetwork.lastUsedOffsetForLeaderEpochVersion)
+ assertEquals(0,
+ mockNetwork.lastUsedOffsetForLeaderEpochVersion, "OffsetsForLeaderEpochRequest version.")
Review comment:
Nit: formatting looks a bit weird.
##########
File path: core/src/test/scala/unit/kafka/server/FetchRequestMaxBytesTest.scala
##########
@@ -58,11 +58,13 @@ class FetchRequestMaxBytesTest extends BaseRequestTest {
array
}
+ @BeforeEach
Review comment:
Does JUnit 5 not look for the annotation in superclasses? That is, is this for readability or is it required for behavior?
##########
File path: core/src/test/scala/unit/kafka/server/AlterUserScramCredentialsRequestTest.scala
##########
@@ -43,20 +41,27 @@ import scala.jdk.CollectionConverters._
* Also tests the Alter and Describe APIs for the case where credentials are successfully altered/described.
*/
class AlterUserScramCredentialsRequestTest extends BaseRequestTest {
+
+ private[this] var className = classOf[AlterCredentialsTest.TestPrincipalBuilderReturningAuthorized].getName
+
+ override def setUp(): Unit = {
+ // do nothing as we will setup cluster by 'before'
+ }
+
+ @BeforeEach
+ def before(info: TestInfo): Unit = {
+ if (info.getDisplayName.contains("NotAuthorized"))
Review comment:
This is pretty brittle. Why do we need it?
##########
File path: core/src/test/scala/unit/kafka/server/ReplicaAlterLogDirsThreadTest.scala
##########
@@ -342,8 +342,8 @@ class ReplicaAlterLogDirsThreadTest {
.setEndOffset(leoT1p1)
)
- assertEquals("results from leader epoch request should have offset from local replica",
- expected, result)
+ assertEquals(expected,
+ result, "results from leader epoch request should have offset from local replica")
Review comment:
Nit: formattting looks a bit weird.
##########
File path: core/src/test/scala/unit/kafka/server/DescribeUserScramCredentialsRequestTest.scala
##########
@@ -39,31 +37,36 @@ import scala.jdk.CollectionConverters._
* Testing the API for the case where there are actually credentials to describe is performed elsewhere.
*/
class DescribeUserScramCredentialsRequestTest extends BaseRequestTest {
+
+ private[this] var className = classOf[DescribeCredentialsTest.TestPrincipalBuilderReturningAuthorized].getName
+
+ override def setUp(): Unit = {
+ // do nothing as we will setup cluster by 'before'
+ }
+
+ @BeforeEach
+ def before(info: TestInfo): Unit = {
+ if (info.getDisplayName.contains("NotAuthorized"))
Review comment:
Why do we need this? Seems brittle.
##########
File path: core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala
##########
@@ -36,33 +36,32 @@ import scala.jdk.CollectionConverters._
*/
class LogCleanerIntegrationTest extends AbstractLogCleanerIntegrationTest with KafkaMetricsGroup {
- val codec: CompressionType = CompressionType.LZ4
Review comment:
Why did we remove this? Seems worse to repeat it multiple times below.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559312176
##########
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:
Not sure. I think I would prefer 4 hardcoded test methods calling a private method with two booleans. Seems like it does the job in a simpler way.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559185808
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1417,13 +1417,10 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
def checkInvalidEntity(entityType: String, entityName: Option[String], otherArgs: Array[String]): Unit = {
val opts = createOpts(entityType, entityName, otherArgs)
- try {
+ assertThrows(classOf[IllegalArgumentException], () => {
opts.checkArgs()
Review comment:
Both checks are required so it would be better to keep them in this block.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558455483
##########
File path: core/src/test/scala/unit/kafka/utils/TestUtils.scala
##########
@@ -459,41 +459,9 @@ object TestUtils extends Logging {
* Check that the buffer content from buffer.position() to buffer.limit() is equal
*/
def checkEquals(b1: ByteBuffer, b2: ByteBuffer): Unit = {
- assertEquals("Buffers should have equal length", b1.limit() - b1.position(), b2.limit() - b2.position())
+ assertEquals(b1.limit() - b1.position(), b2.limit() - b2.position(), "Buffers should have equal length")
for(i <- 0 until b1.limit() - b1.position())
- assertEquals("byte " + i + " byte not equal.", b1.get(b1.position() + i), b2.get(b1.position() + i))
- }
-
- /**
- * Throw an exception if the two iterators are of differing lengths or contain
- * different messages on their Nth element
- */
- def checkEquals[T](expected: Iterator[T], actual: Iterator[T]): Unit = {
Review comment:
yep
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559056476
##########
File path: core/src/test/scala/unit/kafka/server/ReplicaFetcherThreadTest.scala
##########
@@ -174,16 +174,16 @@ class ReplicaFetcherThreadTest {
val fetchState = fetcher.fetchState(tp).get
assertEquals(
- s"Partition $tp should${if (!shouldBeReadyForFetch) " NOT" else ""} be ready for fetching",
- shouldBeReadyForFetch, fetchState.isReadyForFetch)
+ shouldBeReadyForFetch,
Review comment:
We can probably improve the formatting of these asserts since the description is now at the end.
##########
File path: core/src/test/scala/unit/kafka/admin/TopicCommandWithZKClientTest.scala
##########
@@ -35,16 +34,14 @@ class TopicCommandWithZKClientTest extends ZooKeeperTestHarness with Logging wit
private var topicService: ZookeeperTopicService = _
private var testTopicName: String = _
- private val _testName = new TestName
- @Rule def testName = _testName
-
- @Before
- def setup(): Unit = {
+ @BeforeEach
+ def setup(info: TestInfo): Unit = {
topicService = ZookeeperTopicService(zkClient)
- testTopicName = s"${testName.getMethodName}-${Random.alphanumeric.take(10).mkString}"
+ // the method name in junit 5 ends with "()"
Review comment:
This comment is stale, right?
##########
File path: core/src/test/scala/unit/kafka/server/AlterUserScramCredentialsRequestNotAuthorizedTest.scala
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package unit.kafka.server
+
+import kafka.network.SocketServer
+import kafka.server.{AlterCredentialsTest, BaseRequestTest, KafkaConfig}
+import org.apache.kafka.clients.admin.ScramMechanism
+import org.apache.kafka.common.message.AlterUserScramCredentialsRequestData
+import org.apache.kafka.common.message.AlterUserScramCredentialsResponseData.AlterUserScramCredentialsResult
+import org.apache.kafka.common.protocol.Errors
+import org.apache.kafka.common.requests.{AlterUserScramCredentialsRequest, AlterUserScramCredentialsResponse}
+import org.junit.jupiter.api.Assertions._
+import org.junit.jupiter.api.Test
+
+import java.util
+import java.util.Properties
+import scala.jdk.CollectionConverters._
+
+/**
+ * see AlterUserScramCredentialsRequestTest
+ */
+class AlterUserScramCredentialsRequestNotAuthorizedTest extends BaseRequestTest {
+
+ override def brokerPropertyOverrides(properties: Properties): Unit = {
+ properties.put(KafkaConfig.ControlledShutdownEnableProp, "false")
+ properties.put(KafkaConfig.AuthorizerClassNameProp, classOf[AlterCredentialsTest.TestAuthorizer].getName)
+ properties.put(KafkaConfig.PrincipalBuilderClassProp, classOf[AlterCredentialsTest.TestPrincipalBuilderReturningUnauthorized].getName)
+ }
+
+ private val user1 = "user1"
+ private val user2 = "user2"
+
+ @Test
+ def testAlterNothingNotAuthorized(): Unit = {
+ val request = new AlterUserScramCredentialsRequest.Builder(
+ new AlterUserScramCredentialsRequestData()
+ .setDeletions(new util.ArrayList[AlterUserScramCredentialsRequestData.ScramCredentialDeletion])
+ .setUpsertions(new util.ArrayList[AlterUserScramCredentialsRequestData.ScramCredentialUpsertion])).build()
+ val response = sendAlterUserScramCredentialsRequest(request)
+
+ val results = response.data.results
+ assertEquals(0, results.size)
+ }
+
+ @Test
+ def testAlterSomethingNotAuthorized(): Unit = {
+
Review comment:
Unintentional?
##########
File path: core/src/test/scala/unit/kafka/server/DescribeUserScramCredentialsRequestNotAuthorizedTest.scala
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package unit.kafka.server
Review comment:
There should be no `unit.` here.
##########
File path: core/src/test/scala/unit/kafka/server/LogDirFailureTest.scala
##########
@@ -150,16 +150,11 @@ class LogDirFailureTest extends IntegrationTestHarness {
TestUtils.causeLogDirFailure(failureType, leaderServer, partition)
// send() should fail due to either KafkaStorageException or NotLeaderOrFollowerException
- try {
- producer.send(record).get(6000, TimeUnit.MILLISECONDS)
- fail("send() should fail with either KafkaStorageException or NotLeaderOrFollowerException")
- } catch {
- case e: ExecutionException =>
- e.getCause match {
- case t: KafkaStorageException =>
- case t: NotLeaderOrFollowerException => // This may happen if ProduceRequest version <= 3
- case t: Throwable => fail(s"send() should fail with either KafkaStorageException or NotLeaderOrFollowerException instead of ${t.toString}")
- }
+ val e = assertThrows(classOf[ExecutionException], () => producer.send(record).get(6000, TimeUnit.MILLISECONDS))
+ e.getCause match {
+ case t: KafkaStorageException =>
+ case t: NotLeaderOrFollowerException => // This may happen if ProduceRequest version <= 3
+ case t: Throwable => fail(s"send() should fail with either KafkaStorageException or NotLeaderOrFollowerException instead of ${t.toString}")
Review comment:
Maybe use `assertTrue` here?
##########
File path: core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala
##########
@@ -88,12 +88,12 @@ class DynamicConfigChangeTest extends KafkaServerTestHarness {
(1 to 50).foreach(i => TestUtils.produceMessage(servers, tp.topic, i.toString))
// Verify that the new config is used for all segments
- assertTrue("Log segment size change not applied", log.logSegments.forall(_.size > 1000))
+ assertTrue(log.logSegments.forall(_.size > 1000), "Log segment size change not applied")
}
private def testQuotaConfigChange(user: String, clientId: String, rootEntityType: String, configEntityName: String): Unit = {
- assertTrue("Should contain a ConfigHandler for " + rootEntityType ,
- this.servers.head.dynamicConfigHandlers.contains(rootEntityType))
+ assertTrue(this.servers.head.dynamicConfigHandlers.contains(rootEntityType) ,
+ rootEntityType + "Should contain a ConfigHandler for ")
Review comment:
Looks like something weird happened here.
##########
File path: core/src/test/scala/unit/kafka/admin/ReassignPartitionsUnitTest.scala
##########
@@ -339,12 +334,9 @@ class ReassignPartitionsUnitTest {
// The proposed assignment should only span the provided brokers
proposed.values.foreach {
- replicas => {
- if (!replicas.forall(goalBrokers.contains(_))) {
- Assert.fail(s"Proposed assignment ${proposed} puts replicas on brokers " +
- s"other than ${goalBrokers}")
- }
- }
+ replicas =>
Review comment:
This can be in the line above.
##########
File path: core/src/test/scala/unit/kafka/admin/PreferredReplicaLeaderElectionCommandTest.scala
##########
@@ -373,7 +368,8 @@ class PreferredReplicaLeaderElectionCommandTest extends ZooKeeperTestHarness wit
val preferredReplicaElection = new PreferredReplicaLeaderElectionCommand(zkClient, Set(new TopicPartition(topic, partition)))
preferredReplicaElection.moveLeaderToPreferredReplica()
val newLeader = TestUtils.waitUntilLeaderIsElectedOrChanged(zkClient, topic, partition, oldLeaderOpt = Some(currentLeader))
- assertEquals("Preferred replica election failed", preferredReplica, newLeader)
+ assertEquals(preferredReplica, newLeader,
+ "Preferred replica election failed")
Review comment:
Can probably be all on the same line.
##########
File path: core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala
##########
@@ -261,14 +261,11 @@ class DynamicConfigChangeTest extends KafkaServerTestHarness {
@Test
def testConfigChangeOnNonExistingTopic(): Unit = {
val topic = TestUtils.tempTopic()
- try {
+ assertThrows(classOf[UnknownTopicOrPartitionException], () => {
val logProps = new Properties()
logProps.put(FlushMessagesProp, 10000: java.lang.Integer)
adminZkClient.changeTopicConfig(topic, logProps)
Review comment:
Isn't it better to only have the exception throwing code inside the `assertThrows`?
##########
File path: core/src/test/scala/unit/kafka/coordinator/transaction/TransactionMarkerRequestCompletionHandlerTest.scala
##########
@@ -86,13 +86,8 @@ class TransactionMarkerRequestCompletionHandlerTest {
val response = new WriteTxnMarkersResponse(new java.util.HashMap[java.lang.Long, java.util.Map[TopicPartition, Errors]]())
- try {
- handler.onComplete(new ClientResponse(new RequestHeader(ApiKeys.PRODUCE, 0, "client", 1),
- null, null, 0, 0, false, null, null, response))
- fail("should have thrown illegal argument exception")
- } catch {
- case _: IllegalStateException => // ok
- }
+ assertThrows(classOf[IllegalStateException], () => handler.onComplete(new ClientResponse(new RequestHeader(ApiKeys.PRODUCE, 0, "client", 1),
+ null, null, 0, 0, false, null, null, response)))
Review comment:
Indent?
##########
File path: core/src/test/scala/unit/kafka/security/auth/ZkAuthorizationTest.scala
##########
@@ -70,14 +70,11 @@ class ZkAuthorizationTest extends ZooKeeperTestHarness with Logging {
Configuration.setConfiguration(null)
System.clearProperty(JaasUtils.JAVA_LOGIN_CONFIG_PARAM)
assertFalse(JaasUtils.isZkSaslEnabled())
- try {
+ assertThrows(classOf[KafkaException], () => {
Review comment:
Should `assertThrows` only enclose the last expression?
##########
File path: core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala
##########
@@ -337,8 +337,8 @@ class ClientQuotaManagerTest extends BaseClientQuotaManagerTest {
time.sleep(1000)
}
- assertEquals("Should be unthrottled since bursty sample has rolled over",
- 0, maybeRecord(clientQuotaManager, "ANONYMOUS", "unknown", 0))
+ assertEquals(0, maybeRecord(clientQuotaManager, "ANONYMOUS", "unknown", 0),
+ "Should be unthrottled since bursty sample has rolled over")
Review comment:
Indent.
##########
File path: core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
##########
@@ -263,17 +254,11 @@ class ProducerFailureHandlingTest extends KafkaServerTestHarness {
// shut down one broker
servers.head.shutdown()
servers.head.awaitShutdown()
- try {
- producer3.send(record).get
- fail("Expected exception when producing to topic with fewer brokers than min.insync.replicas")
- } catch {
- case e: ExecutionException =>
- if (!e.getCause.isInstanceOf[NotEnoughReplicasException] &&
- !e.getCause.isInstanceOf[NotEnoughReplicasAfterAppendException] &&
- !e.getCause.isInstanceOf[TimeoutException]) {
- fail("Expected NotEnoughReplicasException or NotEnoughReplicasAfterAppendException when producing to topic " +
- "with fewer brokers than min.insync.replicas, but saw " + e.getCause)
- }
+ assertThrows(classOf[ExecutionException], () => producer3.send(record).get).getCause match {
+ case _ @ (_: NotEnoughReplicasException | _: NotEnoughReplicasAfterAppendException | _: TimeoutException) => // pass
+ case e: Throwable =>
+ fail("Expected NotEnoughReplicasException or NotEnoughReplicasAfterAppendException when producing to topic " +
Review comment:
This message is out of date, I don't think it's worth enumerating the exceptions in the message since people tend to forget to update it. I'd probably just do `assertTrue`.
##########
File path: core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala
##########
@@ -138,37 +138,34 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness {
// send a normal record
val record0 = new ProducerRecord[Array[Byte], Array[Byte]](topic, partition, "key".getBytes(StandardCharsets.UTF_8),
"value".getBytes(StandardCharsets.UTF_8))
- assertEquals("Should have offset 0", 0L, producer.send(record0, callback).get.offset)
+ assertEquals(0L, producer.send(record0, callback).get.offset, "Should have offset 0")
// send a record with null value should be ok
val record1 = new ProducerRecord[Array[Byte], Array[Byte]](topic, partition, "key".getBytes(StandardCharsets.UTF_8), null)
- assertEquals("Should have offset 1", 1L, producer.send(record1, callback).get.offset)
+ assertEquals(1L, producer.send(record1, callback).get.offset, "Should have offset 1")
// send a record with null key should be ok
val record2 = new ProducerRecord[Array[Byte], Array[Byte]](topic, partition, null, "value".getBytes(StandardCharsets.UTF_8))
- assertEquals("Should have offset 2", 2L, producer.send(record2, callback).get.offset)
+ assertEquals(2L, producer.send(record2, callback).get.offset, "Should have offset 2")
// send a record with null part id should be ok
val record3 = new ProducerRecord[Array[Byte], Array[Byte]](topic, null, "key".getBytes(StandardCharsets.UTF_8),
"value".getBytes(StandardCharsets.UTF_8))
- assertEquals("Should have offset 3", 3L, producer.send(record3, callback).get.offset)
+ assertEquals(3L, producer.send(record3, callback).get.offset, "Should have offset 3")
// send a record with null topic should fail
- try {
+ assertThrows(classOf[IllegalArgumentException], () => {
val record4 = new ProducerRecord[Array[Byte], Array[Byte]](null, partition, "key".getBytes(StandardCharsets.UTF_8),
"value".getBytes(StandardCharsets.UTF_8))
Review comment:
Move outside `assertThrows`?
##########
File path: core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
##########
@@ -608,29 +557,21 @@ class PlaintextAdminIntegrationTest extends BaseAdminIntegrationTest {
// assert that the topic1 now has 4 partitions
altered = alterResult.values.get(topic1).get
TestUtils.waitUntilTrue(() => numPartitions(topic1) == 4, "Timed out waiting for new partitions to appear")
- try {
- altered = alterResult.values.get(topic2).get
- } catch {
- case e: ExecutionException =>
- assertTrue(e.getCause.isInstanceOf[InvalidPartitionsException])
- assertEquals("Topic currently has 3 partitions, which is higher than the requested 2.", e.getCause.getMessage)
- // assert that the topic2 still has 3 partitions
- assertEquals(3, numPartitions(topic2))
- }
+ var e = assertThrows(classOf[ExecutionException], () => alterResult.values.get(topic2).get)
+ assertTrue(e.getCause.isInstanceOf[InvalidPartitionsException])
+ assertEquals("Topic currently has 3 partitions, which is higher than the requested 2.", e.getCause.getMessage)
+ // assert that the topic2 still has 3 partitions
Review comment:
Looks like a redundant comment.
##########
File path: core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
##########
@@ -375,16 +364,12 @@ class DeleteTopicTest extends ZooKeeperTestHarness {
val topic = topicPartition.topic
servers = createTestTopicAndCluster(topic)
- try {
+ assertThrows(classOf[TopicAlreadyMarkedForDeletionException], () => {
// start topic deletion
adminZkClient.deleteTopic(topic)
Review comment:
Should this be outside of the assertThrows?
##########
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:
Use `assertNotNull`? There are a few other cases like this in the file.
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1417,13 +1417,10 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
def checkInvalidEntity(entityType: String, entityName: Option[String], otherArgs: Array[String]): Unit = {
val opts = createOpts(entityType, entityName, otherArgs)
- try {
+ assertThrows(classOf[IllegalArgumentException], () => {
opts.checkArgs()
Review comment:
Move this out of the assertThrows block?
##########
File path: core/src/test/scala/unit/kafka/server/AlterUserScramCredentialsRequestTest.scala
##########
@@ -356,17 +320,17 @@ class AlterUserScramCredentialsRequestTest extends BaseRequestTest {
}
private def checkAllErrorsAlteringCredentials(resultsToCheck: util.List[AlterUserScramCredentialsResult], expectedError: Errors, contextMsg: String) = {
Review comment:
Is this still used in this file?
##########
File path: core/src/test/scala/unit/kafka/server/AlterUserScramCredentialsRequestNotAuthorizedTest.scala
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package unit.kafka.server
+
+import kafka.network.SocketServer
+import kafka.server.{AlterCredentialsTest, BaseRequestTest, KafkaConfig}
+import org.apache.kafka.clients.admin.ScramMechanism
+import org.apache.kafka.common.message.AlterUserScramCredentialsRequestData
+import org.apache.kafka.common.message.AlterUserScramCredentialsResponseData.AlterUserScramCredentialsResult
+import org.apache.kafka.common.protocol.Errors
+import org.apache.kafka.common.requests.{AlterUserScramCredentialsRequest, AlterUserScramCredentialsResponse}
+import org.junit.jupiter.api.Assertions._
+import org.junit.jupiter.api.Test
+
+import java.util
+import java.util.Properties
+import scala.jdk.CollectionConverters._
+
+/**
+ * see AlterUserScramCredentialsRequestTest
+ */
+class AlterUserScramCredentialsRequestNotAuthorizedTest extends BaseRequestTest {
+
+ override def brokerPropertyOverrides(properties: Properties): Unit = {
+ properties.put(KafkaConfig.ControlledShutdownEnableProp, "false")
+ properties.put(KafkaConfig.AuthorizerClassNameProp, classOf[AlterCredentialsTest.TestAuthorizer].getName)
+ properties.put(KafkaConfig.PrincipalBuilderClassProp, classOf[AlterCredentialsTest.TestPrincipalBuilderReturningUnauthorized].getName)
+ }
+
+ private val user1 = "user1"
+ private val user2 = "user2"
+
+ @Test
+ def testAlterNothingNotAuthorized(): Unit = {
+ val request = new AlterUserScramCredentialsRequest.Builder(
+ new AlterUserScramCredentialsRequestData()
+ .setDeletions(new util.ArrayList[AlterUserScramCredentialsRequestData.ScramCredentialDeletion])
+ .setUpsertions(new util.ArrayList[AlterUserScramCredentialsRequestData.ScramCredentialUpsertion])).build()
+ val response = sendAlterUserScramCredentialsRequest(request)
+
+ val results = response.data.results
+ assertEquals(0, results.size)
+ }
+
+ @Test
+ def testAlterSomethingNotAuthorized(): Unit = {
+
+ val request = new AlterUserScramCredentialsRequest.Builder(
+ new AlterUserScramCredentialsRequestData()
+ .setDeletions(util.Arrays.asList(new AlterUserScramCredentialsRequestData.ScramCredentialDeletion().setName(user1).setMechanism(ScramMechanism.SCRAM_SHA_256.`type`)))
+ .setUpsertions(util.Arrays.asList(new AlterUserScramCredentialsRequestData.ScramCredentialUpsertion().setName(user2).setMechanism(ScramMechanism.SCRAM_SHA_512.`type`)))).build()
+ val response = sendAlterUserScramCredentialsRequest(request)
+
+ val results = response.data.results
+ assertEquals(2, results.size)
+ checkAllErrorsAlteringCredentials(results, Errors.CLUSTER_AUTHORIZATION_FAILED, "when not authorized")
+ }
+
+ private def sendAlterUserScramCredentialsRequest(request: AlterUserScramCredentialsRequest, socketServer: SocketServer = controllerSocketServer): AlterUserScramCredentialsResponse = {
+ connectAndReceive[AlterUserScramCredentialsResponse](request, destination = socketServer)
+ }
+
+
Review comment:
Unintentional empty line?
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559341297
##########
File path: core/src/test/scala/integration/kafka/api/ConsumerTopicCreationTest.scala
##########
@@ -35,9 +35,21 @@ import java.util.Collections
* Tests behavior of specifying auto topic creation configuration for the consumer and broker
*/
class ConsumerTopicCreationTest {
- @ParameterizedTest
- @MethodSource(Array("parameters"))
- def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
+
+
+ @Test
+ def testAutoCreationEnabled(): Unit = testAutoTopicCreation(true, true)
+
+ @Test
+ def testBrokerAutoCreationEnabled(): Unit = testAutoTopicCreation(true, false)
+
+ @Test
+ def testClientAutoCreationEnabled(): Unit = testAutoTopicCreation(false, true)
+
+ @Test
+ def testAutoCreationDisabled(): Unit = testAutoTopicCreation(false, false)
+
+ private def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
Review comment:
the related code is reverted.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r558976200
##########
File path: core/src/test/scala/integration/kafka/api/BaseProducerSendTest.scala
##########
@@ -375,14 +372,10 @@ abstract class BaseProducerSendTest extends KafkaServerTestHarness {
// Trying to send a record to a partition beyond topic's partition range before adding the partition should fail.
val partition1 = 1
- try {
- producer.send(new ProducerRecord(topic, partition1, null, "value".getBytes(StandardCharsets.UTF_8))).get()
- fail("Should not allow sending a record to a partition not present in the metadata")
- } catch {
- case e: ExecutionException => e.getCause match {
- case _: TimeoutException => // this is ok
- case ex => throw new Exception("Sending to a partition not present in the metadata should result in a TimeoutException", ex)
- }
+ val e = assertThrows(classOf[ExecutionException], () => producer.send(new ProducerRecord(topic, partition1, null, "value".getBytes(StandardCharsets.UTF_8))).get())
+ e.getCause match {
+ case _: TimeoutException => // this is ok
+ case ex => throw new Exception("Sending to a partition not present in the metadata should result in a TimeoutException", ex)
Review comment:
I think you can assert that the cause is TimeoutException instead of this approach.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559325692
##########
File path: core/src/test/scala/integration/kafka/api/ConsumerTopicCreationTest.scala
##########
@@ -35,9 +35,21 @@ import java.util.Collections
* Tests behavior of specifying auto topic creation configuration for the consumer and broker
*/
class ConsumerTopicCreationTest {
- @ParameterizedTest
- @MethodSource(Array("parameters"))
- def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
+
+
+ @Test
+ def testAutoCreationEnabled(): Unit = testAutoTopicCreation(true, true)
+
+ @Test
+ def testBrokerAutoCreationEnabled(): Unit = testAutoTopicCreation(true, false)
+
+ @Test
+ def testClientAutoCreationEnabled(): Unit = testAutoTopicCreation(false, true)
+
+ @Test
+ def testAutoCreationDisabled(): Unit = testAutoTopicCreation(false, false)
+
+ private def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
Review comment:
With this, you don't need the separate TestCase, right? You can rely on standard JUnit lifecycle methods.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559255133
##########
File path: core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala
##########
@@ -325,11 +315,9 @@ class LogCleanerParameterizedIntegrationTest(compressionCodec: String) extends A
}
object LogCleanerParameterizedIntegrationTest {
- @Parameters
- def parameters: java.util.Collection[Array[String]] = {
- val list = new java.util.ArrayList[Array[String]]()
- for (codec <- CompressionType.values)
- list.add(Array(codec.name))
- list
- }
+ def all: java.util.stream.Stream[Arguments] =
+ java.util.Arrays.stream(CompressionType.values.map(codec => Arguments.of(codec)))
+
+ def excludeZstd: java.util.stream.Stream[Arguments] =
Review comment:
Nit: even though this is technically true, it doesn't seem to capture the reason why we exclude zstd. It seems like we basically want to generate the compression types supported by V0 and V1 in this method. That would work better if we add more compression types in the future.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559325928
##########
File path: core/src/test/scala/integration/kafka/api/ConsumerTopicCreationTest.scala
##########
@@ -35,9 +35,21 @@ import java.util.Collections
* Tests behavior of specifying auto topic creation configuration for the consumer and broker
*/
class ConsumerTopicCreationTest {
- @ParameterizedTest
- @MethodSource(Array("parameters"))
- def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
+
+
+ @Test
+ def testAutoCreationEnabled(): Unit = testAutoTopicCreation(true, true)
+
+ @Test
+ def testBrokerAutoCreationEnabled(): Unit = testAutoTopicCreation(true, false)
+
+ @Test
+ def testClientAutoCreationEnabled(): Unit = testAutoTopicCreation(false, true)
+
+ @Test
+ def testAutoCreationDisabled(): Unit = testAutoTopicCreation(false, false)
+
+ private def testAutoTopicCreation(brokerAutoTopicCreationEnable: JBoolean, consumerAllowAutoCreateTopics: JBoolean): Unit = {
Review comment:
Oh, this doesn't solve the problem. In that case, my suggestion was bad and what you had before was better. Sorry about that.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559302936
##########
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:
Same for this one. Did you forget to push some changes?
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#issuecomment-762137291
> Will you go ahead and merge then?
give me one second. the QA is always fails on JDK 8. I'm testing it on my local.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559675513
##########
File path: core/src/test/scala/unit/kafka/cluster/AssignmentStateTest.scala
##########
@@ -29,53 +29,53 @@ object AssignmentStateTest extends AbstractPartitionTest {
Arguments.of(
List[Integer](brokerId, brokerId + 1, brokerId + 2),
List[Integer](brokerId, brokerId + 1, brokerId + 2),
- List.empty[Integer], List.empty[Integer], Seq.empty[Int], false),
+ List.empty[Integer], List.empty[Integer], Seq.empty[Int], Boolean.box(false)),
Review comment:
For the future, I think it's a bit cleaner to do `false: java.lang.Boolean`. The compiler figures out the most appropriate way to convert.
##########
File path: core/src/test/scala/unit/kafka/cluster/AssignmentStateTest.scala
##########
@@ -29,53 +29,53 @@ object AssignmentStateTest extends AbstractPartitionTest {
Arguments.of(
List[Integer](brokerId, brokerId + 1, brokerId + 2),
List[Integer](brokerId, brokerId + 1, brokerId + 2),
- List.empty[Integer], List.empty[Integer], Seq.empty[Int], false),
+ List.empty[Integer], List.empty[Integer], Seq.empty[Int], Boolean.box(false)),
Review comment:
For the future (no need to change right now), I think it's a bit cleaner to do `false: java.lang.Boolean`. The compiler figures out the most appropriate way to convert.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559312401
##########
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:
Basically:
1. `testAutoCreationEnabled`
2. `testBrokerAutoCreationEnabled`
3. `testClientAutoCreationEnabled`
4. `testAutoCreationDisabled`
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#issuecomment-759950942
FYI, I started reviewing this so please no force pushes. :)
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559254941
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1417,13 +1417,10 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
def checkInvalidEntity(entityType: String, entityName: Option[String], otherArgs: Array[String]): Unit = {
val opts = createOpts(entityType, entityName, otherArgs)
- try {
+ assertThrows(classOf[IllegalArgumentException], () => {
opts.checkArgs()
Review comment:
Hmm, do you mean both expressions fail or one or the other fail depending on the usage? It seems pretty unclear as it stands, so we need to improve the code and/or comment it better.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#issuecomment-762125513
```
org.apache.kafka.streams.integration.AdjustStreamThreadCountTest
```
this flaky traced by #9887 and #9888
```
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition()
```
this flaky traced by #9877
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r554628355
##########
File path: core/src/test/scala/integration/kafka/admin/BrokerApiVersionsCommandTest.scala
##########
@@ -19,24 +19,25 @@ package kafka.admin
import java.io.{ByteArrayOutputStream, PrintStream}
import java.nio.charset.StandardCharsets
-
import scala.collection.Seq
-
import kafka.integration.KafkaServerTestHarness
import kafka.server.KafkaConfig
import kafka.utils.TestUtils
import org.apache.kafka.clients.NodeApiVersions
import org.apache.kafka.common.protocol.ApiKeys
-import org.junit.Assert.{assertEquals, assertFalse, assertNotNull, assertTrue}
-import org.junit.Test
+import org.junit.jupiter.api.Assertions.{assertEquals, assertFalse, assertNotNull, assertTrue}
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
+import java.util.concurrent.TimeUnit
import scala.jdk.CollectionConverters._
class BrokerApiVersionsCommandTest extends KafkaServerTestHarness {
def generateConfigs: Seq[KafkaConfig] = TestUtils.createBrokerConfigs(1, zkConnect).map(KafkaConfig.fromProps)
- @Test(timeout=120000)
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
Review comment:
A more concise approoach is `@Timeout(120)`.
##########
File path: core/src/test/scala/integration/kafka/admin/BrokerApiVersionsCommandTest.scala
##########
@@ -19,24 +19,25 @@ package kafka.admin
import java.io.{ByteArrayOutputStream, PrintStream}
import java.nio.charset.StandardCharsets
-
import scala.collection.Seq
-
import kafka.integration.KafkaServerTestHarness
import kafka.server.KafkaConfig
import kafka.utils.TestUtils
import org.apache.kafka.clients.NodeApiVersions
import org.apache.kafka.common.protocol.ApiKeys
-import org.junit.Assert.{assertEquals, assertFalse, assertNotNull, assertTrue}
-import org.junit.Test
+import org.junit.jupiter.api.Assertions.{assertEquals, assertFalse, assertNotNull, assertTrue}
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
+import java.util.concurrent.TimeUnit
import scala.jdk.CollectionConverters._
class BrokerApiVersionsCommandTest extends KafkaServerTestHarness {
def generateConfigs: Seq[KafkaConfig] = TestUtils.createBrokerConfigs(1, zkConnect).map(KafkaConfig.fromProps)
- @Test(timeout=120000)
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
Review comment:
A more concise approach is `@Timeout(120)`.
----------------------------------------------------------------
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
[GitHub] [kafka] ijuma commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559304383
##########
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:
I was thinking we could use dynamic tests instead: https://www.baeldung.com/junit5-dynamic-tests#improving-the-dynamictest-using-java-8-features
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r557411439
##########
File path: build.gradle
##########
@@ -787,9 +787,7 @@ project(':core') {
testCompile libs.apachedsLdifPartition
testCompile libs.apachedsMavibotPartition
testCompile libs.apachedsJdbmPartition
- testCompile libs.junitJupiterApi
- testCompile libs.junitVintageEngine
- testCompile libs.scalatest
Review comment:
```streams-scala``` still depends on that so #9858 will remove the dependency.
----------------------------------------------------------------
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9855: KAFKA-7341 Migrate core module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9855:
URL: https://github.com/apache/kafka/pull/9855#discussion_r559308504
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1417,13 +1417,10 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
def checkInvalidEntity(entityType: String, entityName: Option[String], otherArgs: Array[String]): Unit = {
val opts = createOpts(entityType, entityName, otherArgs)
- try {
+ assertThrows(classOf[IllegalArgumentException], () => {
opts.checkArgs()
Review comment:
I rewrite the assert to make it more accurate.
----------------------------------------------------------------
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