You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/09/03 17:03:31 UTC
[GitHub] [cassandra] dcapwell commented on a change in pull request #704: Cassandra 15991
dcapwell commented on a change in pull request #704:
URL: https://github.com/apache/cassandra/pull/704#discussion_r483118732
##########
File path: test/unit/org/apache/cassandra/tools/AuditLogViewerTest.java
##########
@@ -61,6 +73,75 @@ public void tearDown() throws IOException
}
}
+ @Test
+ public void testNoArgs()
+ {
+ try (ToolRunner tool = runner.invokeTool(toolPath))
Review comment:
do we need to close? test blocks on the exit, so the force delete process doesn't seem needed?
##########
File path: test/unit/org/apache/cassandra/tools/AuditLogViewerTest.java
##########
@@ -61,6 +73,75 @@ public void tearDown() throws IOException
}
}
+ @Test
+ public void testNoArgs()
+ {
+ try (ToolRunner tool = runner.invokeTool(toolPath))
+ {
+ assertThat(tool.getStdout(), CoreMatchers.containsStringIgnoringCase("usage:"));
+ assertThat(tool.getCleanedStderr(), CoreMatchers.containsStringIgnoringCase("Audit log files directory path is a required argument."));
+ assertEquals(1, tool.getExitCode());
+ }
+ }
+
+ @Test
+ public void testMaybeChangeDocs()
+ {
+ // If you added, modified options or help, please update docs if necessary
+ try (ToolRunner tool = runner.invokeTool(toolPath, "-h"))
+ {
+ assertEquals("34aea94756d4f6760b836dfbfa7bbf06", DigestUtils.md5Hex(tool.getStdout()));
Review comment:
For some reason https://github.com/apache/cassandra/pull/704#discussion_r480362270 got lost so adding back...
Even though it is more verbose I am worried about maintenance. If someone adds a new argument or a new command then they get an error that isn't actionable, they then need to read the test, understand what is going on, then regenerate a new hash; it is easier for the author to make changes to the expected string.
##########
File path: test/unit/org/apache/cassandra/tools/AuditLogViewerTest.java
##########
@@ -61,6 +71,65 @@ public void tearDown() throws IOException
}
}
+ @Test
+ public void testNoArgs()
+ {
+ ToolRunner tool = runner.invokeTool(toolPath);
+ assertTrue(tool.getStdout(), tool.getStdout().contains("usage:"));
+ assertTrue(tool.getCleanedStderr(), tool.getCleanedStderr().contains("Audit log files directory path is a required argument."));
+ assertEquals(1, tool.getExitCode());
+ }
+
+ @Test
+ public void testMaybeChangeDocs()
+ {
+ // If you added, modified options or help, please update docs if necessary
+ ToolRunner tool = runner.invokeTool(toolPath, "-h");
+ assertEquals("34aea94756d4f6760b836dfbfa7bbf06", DigestUtils.md5Hex(tool.getStdout()));
+ }
+
+ @Test
+ public void testHelpArg()
+ {
+ Arrays.asList("-h", "--help").stream().forEach(arg -> {
+ ToolRunner tool = runner.invokeTool(toolPath, arg);
+ assertTrue(tool.getStdout(), tool.getStdout().contains("usage:"));
+ assertTrue(tool.getCleanedStderr(),tool.getCleanedStderr().isEmpty());
+ assertEquals(0, tool.getExitCode());
+ });
+ }
+
+ @Test
+ public void testIgnoreArg()
+ {
+ Arrays.asList("-i", "--ignore").stream().forEach(arg -> {
+ ToolRunner tool = runner.invokeTool(toolPath, path.toAbsolutePath().toString(), arg);
+ assertTrue(tool.getStdout(), tool.getStdout().isEmpty());
+ assertTrue(tool.getCleanedStderr(),
+ tool.getCleanedStderr().isEmpty() // j8 is fine
+ || tool.getCleanedStderr().startsWith("WARNING: An illegal reflective access operation has occurred")); //j11 throws an error
Review comment:
If you are planning to fix the jdk11 bug in a different ticket, can we move these tests to those tickets? We can also leave it without the WARNING exclusion and mark the test as ignored (such as `org.apache.cassandra.distributed.test.RepairCoordinatorFailingMessageTest`).
##########
File path: test/unit/org/apache/cassandra/cql3/CQLTester.java
##########
@@ -451,19 +451,43 @@ public void run()
}
});
}
-
+
public static List<String> buildNodetoolArgs(List<String> args)
{
List<String> allArgs = new ArrayList<>();
allArgs.add("bin/nodetool");
allArgs.add("-p");
allArgs.add(Integer.toString(jmxPort));
allArgs.add("-h");
- allArgs.add(jmxHost);
+ allArgs.add(jmxHost == null ? "127.0.0.1" : jmxHost);
Review comment:
if null should we fail? this is defined if `jmxServer = JMXServerUtils.createJMXServer(jmxPort, true);` is setup and running, so if not present then doesn't this mean we didn't start the xmx server?
##########
File path: test/unit/org/apache/cassandra/tools/AuditLogViewerTest.java
##########
@@ -61,6 +73,75 @@ public void tearDown() throws IOException
}
}
+ @Test
+ public void testNoArgs()
+ {
+ try (ToolRunner tool = runner.invokeTool(toolPath))
+ {
+ assertThat(tool.getStdout(), CoreMatchers.containsStringIgnoringCase("usage:"));
+ assertThat(tool.getCleanedStderr(), CoreMatchers.containsStringIgnoringCase("Audit log files directory path is a required argument."));
+ assertEquals(1, tool.getExitCode());
+ }
+ }
+
+ @Test
+ public void testMaybeChangeDocs()
+ {
+ // If you added, modified options or help, please update docs if necessary
+ try (ToolRunner tool = runner.invokeTool(toolPath, "-h"))
+ {
+ assertEquals("34aea94756d4f6760b836dfbfa7bbf06", DigestUtils.md5Hex(tool.getStdout()));
+ }
+ }
+
+ @Test
+ public void testHelpArg()
+ {
+ Arrays.asList("-h", "--help").forEach(arg -> {
+ try (ToolRunner tool = runner.invokeTool(toolPath, arg))
+ {
+ assertThat(tool.getStdout(), CoreMatchers.containsStringIgnoringCase("usage:"));
+ assertTrue(tool.getCleanedStderr(),tool.getCleanedStderr().isEmpty());
+ assertEquals(0, tool.getExitCode());
+ }
+ });
+ }
+
+ @Test
+ public void testIgnoreArg()
+ {
+ Arrays.asList("-i", "--ignore").forEach(arg -> {
+ try (ToolRunner tool = runner.invokeTool(toolPath, path.toAbsolutePath().toString(), arg))
+ {
+ assertTrue(tool.getStdout(), tool.getStdout().isEmpty());
+ assertTrue(tool.getCleanedStderr(),
+ tool.getCleanedStderr().isEmpty() // j8 is fine
+ || tool.getCleanedStderr().startsWith("WARNING: An illegal reflective access operation has occurred")); //j11 throws an error
+ assertEquals(0, tool.getExitCode());
Review comment:
should we use `tool.assertOnExitCode()` as it can provide a better message?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org