[GitHub] [hadoop-ozone] fapifta commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry

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

fapifta commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r466381992



##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -158,20 +184,34 @@ private void verifyLog(String expected) throws IOException {
       }
       i++;
     }
-
-    // When log entry is expected, the log file will contain one line and
-    // that must be equal to the expected string
-    assertTrue(lines.size() != 0);
-    assertTrue(expected.equalsIgnoreCase(lines.get(0)));
+    //check if every expected string can be found in the log entry
+    for(String expected : expectedStrings) {
+      assertTrue(contains(lines, expected));

Review comment:
       instead of writing a contains method we can use the following combination of assertThat and matchers to gain the same functionality:
   ```java
   assertThat(lines, hasItem(containsString(expected)));
   ```
   
   If we change to check that lines starts with the lines specified in expectedStrings in order, then an other approach like this solves the problem:
   ```java
   assertThat(
       lines.subList(0,expectedStrings.length),
       containsInOrder(expectedStrings)
   );
   ```
   
   where containsInOrder is defined as:
   ```java
   private Matcher<Iterable<? extends String>> containsInOrder(
       String[] expectedStrings) {
     return IsIterableContainingInOrder.contains(
         Arrays.stream(expectedStrings)
             .map(str -> containsString(str))
             .collect(Collectors.toList())
     );
   }
   ```
   
   In order for this to work it is necessary to add org-harmcrest:harmcrest-all as a test scope dependency to the hdds-common module, and change the messageIncludesMultilineException test, to call verifyLog as:
   ```java
   verifyLog(
       "ERROR | OMAudit | user=john | ip=192.168.0.1 | op=CREATE_VOLUME " +
           "{key1=value1, key2=value2} | ret=FAILURE",,
       "TestException: " + exceptionMsg,
       "org.apache.hadoop.ozone.audit."
           + "TestOzoneAuditLogger."
           + "messageIncludesMultilineException"
   );
   ```
   assuming that the message text given to the TestException is extracted to a local variable called exceptionMsg ;)

##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -143,7 +143,33 @@ public void notLogReadEvents() throws IOException {
     verifyNoLog();
   }
 
-  private void verifyLog(String expected) throws IOException {
+  /**
+   * Test to verify if multiline entries can be checked.
+   */
+
+  @Test
+  public void messageIncludesMultilineException() throws IOException {
+    try{
+      throw new TestException("Dummy exception");

Review comment:
       One don't need to throw the exception in order to pass it to the withException method and log it correctly, so the test could be simpler by just specifying a new TestExecption("Dummy") to the withException method on the builder.




----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org