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