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/08/31 20:04:02 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_r480365473
##########
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
+ assertEquals(0, tool.getExitCode());
+ });
+ }
+
+ @Test
+ public void testFollowNRollArgs()
+ {
+ Arrays.asList(Pair.of("-f", "-r"), Pair.of("--follow", "--roll_cycle")).stream().forEach(arg -> {
Review comment:
nit: should do `Lists.cartesianProduct(Arrays.asList("-f", "--follow"), Arrays.asList("-r", "--roll_cycle")).forEach` so we cover all cases
##########
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:"));
Review comment:
should use `org.assertj.core.api.Assertions#assertThat(java.lang.String)`. `assertTrue` doesn't have a useful error message so if someone does something like replace `usage:` with `Usage:` then the error is "there is an error"; by using `Assertions.assertThat(tool.getStdout()).contains("usage:")` you get a much more useful error message.
##########
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 -> {
Review comment:
remove `.stream()`, forEach exists on list as well.
##########
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()
Review comment:
should merge this test with `testMaybeChangeDocs`
##########
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."));
Review comment:
same comment as above
##########
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()));
Review comment:
would be cleaner to copy the current stdout text and use in the test rather than md5. If you use `Assertions.assertThat(tool.getStdout()).isEqualTo(expectedHelp)` you get a very useful error message showing what failed; with md5 we need to spend more time debugging.
##########
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:
This looks like a bug, do we not include the flag to disable this?
##########
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 -> {
Review comment:
remove `.stream()`
##########
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
+ assertEquals(0, tool.getExitCode());
+ });
+ }
+
+ @Test
+ public void testFollowNRollArgs()
+ {
+ Arrays.asList(Pair.of("-f", "-r"), Pair.of("--follow", "--roll_cycle")).stream().forEach(arg -> {
+ ToolRunner tool = runner.invokeToolNoWait(Arrays.asList(toolPath,
+ path.toAbsolutePath().toString(),
+ arg.getLeft(),
+ arg.getRight(),
+ "TEST_SECONDLY"));
+ // Tool is running in the background 'following' so we have to kill it
+ assertFalse(tool.waitFor(3, TimeUnit.SECONDS));
Review comment:
so you want to validate that we are still following? Where do you actually kill it the process?
##########
File path: test/unit/org/apache/cassandra/tools/OfflineToolUtils.java
##########
@@ -110,7 +110,7 @@ public void assertNoUnexpectedThreadsStarted(String[] expectedThreadNames, Strin
.filter(threadName -> optional.stream().anyMatch(pattern -> pattern.matcher(threadName).matches()))
.collect(Collectors.toSet());
- if (!current.isEmpty())
+ if (!remain.isEmpty())
Review comment:
for me: confirmed that remaining should have been used
----------------------------------------------------------------
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