AndrewJSchofield commented on code in PR #23349:
URL: https://github.com/apache/kafka/pull/23349#discussion_r4349297171
##########
tools/src/test/java/org/apache/kafka/tools/GroupsCommandTest.java:
##########
@@ -463,6 +467,43 @@ public void testListGroupsFailsWithException() {
)));
}
+ @Test
+ public void testMainNoExitReturnsNonZeroWithoutExitingOnFailure() throws
Exception {
+ // An invalid security protocol makes the admin client creation inside
execute() fail.
+ // mainNoExit must report that failure by returning a non-zero code,
and execute() must
+ // not terminate the JVM itself.
+ assertEquals(1, GroupsCommand.mainNoExit(
+ "--bootstrap-server", bootstrapServer,
+ "--list",
+ "--command-config", invalidCommandConfig()));
+ assertFalse(exitProcedure.hasExited(), "execute() must not call
Exit.exit(); only main() may exit");
+ }
+
+ @Test
+ public void testExecuteDoesNotExitOnFailure() throws Exception {
+ // execute() propagates the failure to its caller rather than
terminating the JVM.
+ String commandConfig = invalidCommandConfig();
+ assertThrows(Exception.class, () -> GroupsCommand.execute(
+ "--bootstrap-server", bootstrapServer,
+ "--list",
+ "--command-config", commandConfig));
+ assertFalse(exitProcedure.hasExited(), "execute() must not call
Exit.exit(); only main() may exit");
+ }
+
+ /**
+ * Writes a command config file which parses correctly but makes the admin
client creation
+ * inside {@link GroupsCommand#execute} fail.
+ */
+ private String invalidCommandConfig() throws IOException {
Review Comment:
Could you use `ToolsTestUtils.tempPropertiesFile` instead like
`ConfigCommandTest`?
##########
tools/src/main/java/org/apache/kafka/tools/GroupsCommand.java:
##########
@@ -57,8 +56,7 @@ static int mainNoExit(String... args) {
execute(args);
return 0;
} catch (Throwable e) {
- System.err.println(e.getMessage());
Review Comment:
There's a behaviour change here. Formerly, the exception information was
written to stderr, but now it's stdout.
##########
tools/src/test/java/org/apache/kafka/tools/GroupsCommandTest.java:
##########
@@ -463,6 +467,43 @@ public void testListGroupsFailsWithException() {
)));
}
+ @Test
+ public void testMainNoExitReturnsNonZeroWithoutExitingOnFailure() throws
Exception {
+ // An invalid security protocol makes the admin client creation inside
execute() fail.
+ // mainNoExit must report that failure by returning a non-zero code,
and execute() must
+ // not terminate the JVM itself.
+ assertEquals(1, GroupsCommand.mainNoExit(
+ "--bootstrap-server", bootstrapServer,
+ "--list",
+ "--command-config", invalidCommandConfig()));
+ assertFalse(exitProcedure.hasExited(), "execute() must not call
Exit.exit(); only main() may exit");
+ }
+
+ @Test
+ public void testExecuteDoesNotExitOnFailure() throws Exception {
+ // execute() propagates the failure to its caller rather than
terminating the JVM.
+ String commandConfig = invalidCommandConfig();
+ assertThrows(Exception.class, () -> GroupsCommand.execute(
Review Comment:
Can you assert on the specific exception class please?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
