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 1: (16 comments) ms http://gerrit.cloudera.org:8080/#/c/12042/1/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: PS1: License header. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@23 PS1, Line 23: public static class Options { > Any reason you went with an options class vs a builder pattern? It behaves as a builder though. Maybe it should be renamed? http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@69 PS1, Line 69: .reportResults(isReportingEnabled()) Why not just not create a ResultReporter if this is false? Seems like it'd be more intuitive that way: if there's a ResultReporter, we report results. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@70 PS1, Line 70: .httpEndpoint(System.getenv("TEST_RESULT_SERVER")) For this and the rest of the required environment variables, we should validate that they're set if KUDU_REPORT_TEST_RESULTS is non-zero. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@106 PS1, Line 106: private static String tryLookupHostname() { This is different from $(hostname) though; it'll do a reverse DNS lookup. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@120 PS1, Line 120: // TODO(mpercy): add support for uploading the test log to the server. > Is this follow on work? I think it needs to go in with the first cut, otherwise the test drilldown won't be particularly useful. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@124 PS1, Line 124: // Construct the HTTP endpoint URL. This (and the majority of params) is unknown at construction time. We needn't repeat all of this work for each reported result. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@142 PS1, Line 142: for (Map.Entry<String, String> entry : params.entrySet()) { Can you use something URIBuilder (http://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/client/utils/URIBuilder.html) to construct this? http://gerrit.cloudera.org:8080/#/c/12042/1/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/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@44 PS1, Line 44: private final ResultReporter reporter; > Could the reporting rule be a separate rule that is then used/called in the On that note, AFAIK RetryRule still isn't universal. It should be, as should the reporting. The test harness isn't necessary everywhere though. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java File java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java: PS1: License header. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@27 PS1, Line 27: // Unit test for ResultReporter. Wrong comment style. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@49 PS1, Line 49: StringBuilder sb = new StringBuilder(); : for (String s : new String[] { testName, buildId, revision, hostname, : buildConfig, Integer.toString(status) }) { : sb.append(s); : sb.append(" "); : } Maybe use a Guava Joiner instead? Or Objects.toStringHelper? http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@61 PS1, Line 61: private static final Logger LOGGER = LoggerFactory.getLogger(MockFlakyTestServiceHandler.class); FYI, our naming convention for Loggers is LOG: $ git grep "Logger LOG " | wc -l 43 $ git grep "Logger LOG[^ ]" | wc -l 1 http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@62 PS1, Line 62: private List<TestRecord> records = new ArrayList<>(); Could be final. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@75 PS1, Line 75: String[] argPairs = request.getQueryString().split("&"); Can merge this into the following line. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@78 PS1, Line 78: if (keyVal.length == 2) { Should we throw if it's not 2? It has to be, right? -- 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: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Dec 2018 18:52:01 +0000 Gerrit-HasComments: Yes
