[GitHub] [kafka] tombentley commented on a change in pull request #8808: KAFKA-10109: Fix double AdminClient creation in AclCommand

[GitBox <gi...@apache.org>]

tombentley commented on a change in pull request #8808:
URL: https://github.com/apache/kafka/pull/8808#discussion_r447002522



##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -130,22 +132,61 @@ class AclCommandTest extends ZooKeeperTestHarness with Logging {
   private def createServer(): Unit = {
     servers = Seq(TestUtils.createServer(KafkaConfig.fromProps(brokerProps)))
     val listenerName = ListenerName.forSecurityProtocol(SecurityProtocol.PLAINTEXT)
-    adminArgs = Array("--bootstrap-server", TestUtils.bootstrapServers(servers, listenerName))
+
+    val tmp = File.createTempFile(classOf[AclCommandTest].getName, "createServer")
+    tmp.deleteOnExit()
+    val pw = new PrintWriter(tmp)
+    pw.println("client.id=my-client")
+    pw.close()
+
+    adminArgs = Array("--bootstrap-server", TestUtils.bootstrapServers(servers, listenerName),
+      "--command-config", tmp.getAbsolutePath)
+  }
+
+  private def callMain(args: Array[String]): (String, String) = {
+    val appender = LogCaptureAppender.createAndRegister()
+    val previousLevel = LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], Level.WARN)
+    val outErr = TestUtils.grabConsoleOutputAndError(AclCommand.main(args))
+    LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], previousLevel)
+    LogCaptureAppender.unregister(appender)
+    Assert.assertEquals("Command should execute without logging errors or warnings",
+      "",
+      appender.getMessages.map{event => s"${event.getLevel} ${event.getMessage}" }.mkString("\n"))
+    outErr
   }
 
   private def testAclCli(cmdArgs: Array[String]): Unit = {
+
     for ((resources, resourceCmd) <- ResourceToCommand) {
       for (permissionType <- Set(ALLOW, DENY)) {
         val operationToCmd = ResourceToOperations(resources)
         val (acls, cmd) = getAclToCommand(permissionType, operationToCmd._1)
-          AclCommand.main(cmdArgs ++ cmd ++ resourceCmd ++ operationToCmd._2 :+ "--add")
-          for (resource <- resources) {
-            withAuthorizer() { authorizer =>
-              TestUtils.waitAndVerifyAcls(acls, authorizer, resource)
-            }
+        val (addOut, addErr) = callMain(cmdArgs ++ cmd ++ resourceCmd ++ operationToCmd._2 :+ "--add")
+        assertOutputContains("Adding ACLs", resources, resourceCmd, addOut)

Review comment:
       @dajac I'm not completely sure what you mean. You're suggesting we just test the output for, say, the cross product `(--topic foo, --group bar)` × `(--add, --list, --remove)` in a new test case?
   
   It do think that currently it's difficult to construct the expected output correctly, and that makes the test harder to understand.

##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -248,7 +289,9 @@ class AclCommandTest extends ZooKeeperTestHarness with Logging {
 
   private def testRemove(cmdArgs: Array[String], resources: Set[ResourcePattern], resourceCmd: Array[String]): Unit = {
     for (resource <- resources) {
-      AclCommand.main(cmdArgs ++ resourceCmd :+ "--remove" :+ "--force")
+      val (out, err) = callMain(cmdArgs ++ resourceCmd :+ "--remove" :+ "--force")
+      Assert.assertEquals("", out)
+      Assert.assertEquals("", err)

Review comment:
       > At the moment, it seems that we always use this helper to remove all ACLs so it seems to work OK. Is it always the case?
   
   Yes, `testRemove()` is currently always paired with `--add`. Of course that might not always be the case. I guess this issue would go aware if we removed the output checking here and had a dedicated test for it (assuming my interpretation of your other comment is correct).




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