Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting ...................................................................... Patch Set 1: (7 comments) 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. thanks for the catch 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 { > It behaves as a builder though. Maybe it should be renamed? It's an options class with a fluent API. It does not have a build() method so I would not call it a builder. I find it pretty friendly to use. Do you find it confusing? 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. So you think we should just get this from `hostname` ? 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. > I think it needs to go in with the first cut, otherwise the test drilldown Yeah that's intended as a follow-on. The statistics seem useful to me, even without the logs. 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-clien I was trying to avoid pulling in extra library dependencies, but we can shade it. 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; > On that note, AFAIK RetryRule still isn't universal. It should be, as shoul I think we're going to want to use the flaky test list to determine whether we should retry a Java test or fail fast, like we do with the C++ tests. If we implement that logic in the reporter then it should probably stay composed, something like this. 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: http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@118 PS1, Line 118: .buildId("shark") > Is this a baby shark kids song reference? lol The song was stuck in my head yesterday... I've got a niece that's into it right now haha -- 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-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 06 Dec 2018 19:36:53 +0000 Gerrit-HasComments: Yes
