lianetm commented on code in PR #18668:
URL: https://github.com/apache/kafka/pull/18668#discussion_r1927509359
##########
core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala:
##########
@@ -112,7 +112,7 @@ class SaslClientsWithInvalidCredentialsTest extends
AbstractSaslTest {
}
@ParameterizedTest(name =
TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
-
@MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly"))
+ @MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
def testTransactionalProducerWithAuthenticationFailure(quorum: String,
groupProtocol: String): Unit = {
Review Comment:
Looks like it does not use a consumer. Could you double check and if so,
then no need to run it for both consumers.
##########
core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala:
##########
@@ -222,6 +222,18 @@ class SaslClientsWithInvalidCredentialsTest extends
AbstractSaslTest {
}, s"Operation did not succeed within timeout after $attempts")
}
+ private def verifyWithRetryPredicate(predicate: => Boolean): Unit = {
+ var attempts = 0
+ TestUtils.waitUntilTrue(() => {
+ try {
+ attempts += 1
+ predicate
+ } catch {
+ case _: SaslAuthenticationException => false
+ }
+ }, s"Operation did not succeed within timeout after $attempts")
+ }
Review Comment:
interesting, makes sense to me.
But then, couldn't we just change the
```
verifyWithRetry(assertEquals(1,
consumer.poll(Duration.ofMillis(1000)).count))
```
to
```
verifyWithRetry(1, consumer.poll(Duration.ofMillis(1000)).count == 1)
```
(just to avoid introducing this new `verifyWithRetryPredicate`)
##########
core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala:
##########
@@ -154,11 +154,11 @@ class SaslClientsWithInvalidCredentialsTest extends
AbstractSaslTest {
createClientCredential()
val producer = createProducer()
verifyWithRetry(sendOneRecord(producer))
- verifyWithRetry(assertEquals(1,
consumer.poll(Duration.ofMillis(1000)).count))
+ verifyWithRetryPredicate(consumer.poll(Duration.ofMillis(1000)).count == 1)
}
@ParameterizedTest(name =
TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
-
@MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly"))
+ @MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
def testKafkaAdminClientWithAuthenticationFailure(quorum: String,
groupProtocol: String): Unit = {
Review Comment:
similar as above, looks like it does need to run for both consumers?
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
