Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/12042/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12042/5//COMMIT_MSG@12 PS5, Line 12: TestResultReporter > ResultReporter Done http://gerrit.cloudera.org:8080/#/c/12042/5/java/gradle/dependencies.gradle File java/gradle/dependencies.gradle: http://gerrit.cloudera.org:8080/#/c/12042/5/java/gradle/dependencies.gradle@45 PS5, Line 45: v20181114", > Is this preferred over 9.4.15 because we're on JDK8? No, I just picked the wrong version apparently. Will update. http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java: http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java@49 PS5, Line 49: private static final Layout LAYOUT = new PatternLayout( : "%d{HH:mm:ss.SSS} [%p - %t] (%F:%L) %m%n"); > Is it worth using this in CapturingLogAppender too? I don't think it matters. In fact, the CapturingLogAppender use case is one where we're programmatically asserting on logged contents, so a complex layout could get in the way. If someone needs a better layout, it's very easy to extend it at that time. http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java: http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@54 PS5, Line 54: private boolean reportResults = true; > Why this default? What happens if we try reporting results but aren't prope The default value doesn't really matter, because consider how the ResultReporter constructors currently work: 1. One is public but customizes reportResults() every time. 2. One is test-only, and tests are expected to customize the options themselves. The first constructor is what's used 99.9% of the time, and it always overwrites the default value. http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java: http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@110 PS5, Line 110: capturer.close(); > Would this make sense in the `finally` block? Or is it only here to quiesce This whole thing is kind of funky. The capturer is declared in the outer try-with-resources, which means it'll go out of scope (and autoclose) _before_ the 'finally' block. However, we report in the 'catch' block, at a time that the capturer's file is open and unflushed. To keep things simple, I decided to implementing flushing via close(), which is why we explicitly close() here before reporting. But this means that the autoclose done by try-with-resources is a no-op if we've caught an exception; it's only actually useful if the test succeeded. I adjusted the CapturingToFileLogAppender lifecycle slightly; does this look better? http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java File java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java: http://gerrit.cloudera.org:8080/#/c/12042/5/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java@34 PS5, Line 34: public RetryRule retryRule = new RetryRule(MAX_FAILURES, /*enableReporting=*/ false); > nit: skipReporting Done -- To view, visit http://gerrit.cloudera.org:8080/12042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74 Gerrit-Change-Number: 12042 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 28 Mar 2019 22:08:00 +0000 Gerrit-HasComments: Yes
