Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/12042/4/java/gradle/dependencies.gradle File java/gradle/dependencies.gradle: http://gerrit.cloudera.org:8080/#/c/12042/4/java/gradle/dependencies.gradle@96 PS4, Line 96: httpCore : "org.apache.httpcomponents:httpcore:$versions.httpCore", I don't think you need this, because httpClient will always pull in the appropriate version of core. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java: http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java@99 PS4, Line 99: return appended.toString(); This won't return the text in the case of a file? http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java@121 PS4, Line 121: public void setWriteToFile(File fileName, boolean useGzip) throws IOException { I wonder if this should be a new class instead of trying to fit file logic into CapturingLogAppender. This could be the constructor in that case. Even if sharing the logic, since WriteToFile and AppendToString are mutually exclusive, using different constructors would make reasoning about the state more straightforward. http://gerrit.cloudera.org:8080/#/c/12042/4/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/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@44 PS4, Line 44: public class ResultReporter { Add audience annotations. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@90 PS4, Line 90: private static final Logger LOG = LoggerFactory.getLogger(ResultReporter.class); nit: I usually put logging at the very top. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@142 PS4, Line 142: private static String getEnvStringWithDefault(String name, String defaultValue) { Are these withDefault methods used? Could/Should the be used on KUDU_REPORT_TEST_RESULTS_VAR above? http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@156 PS4, Line 156: static String getLocalHostname() { I am curious why you didn't use `InetAddress.getLocalHost().getHostName()` http://gerrit.cloudera.org:8080/#/c/12042/4/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/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@a44 PS4, Line 44: I think this comment is still true. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@35 PS4, Line 35: * We use this with Gradle because it doesn't support I suppose we could update this to remove "with Gradle" since the Maven build doesn't exist and add a description that includes the benefit of test reporting. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@56 PS4, Line 56: public RetryRule(int retryCount, boolean enableReporting) { Can this be called skipReporting? Since the reporting itself is enabled/disabled in the reporter. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@88 PS4, Line 88: File tempLogFile = File.createTempFile("test_attempt", ".txt.gz"); Should the filename include the attempt number? http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@91 PS4, Line 91: capturer.setLayout(new PatternLayout("%d{HH:mm:ss.SSS} [%p - %t] (%F:%L) %m%n")); use the PATTERN_LAYOUT variable above? -- 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: 4 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[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 02:30:34 +0000 Gerrit-HasComments: Yes
