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 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/build.gradle File java/kudu-test-utils/build.gradle: http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/build.gradle@35 PS1, Line 35: testCompile libs.junit junit is already a compile time dependency so it's not needed here. I think the same is true for log4j and slf4jLog4j12 given they are optional. 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: 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? 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? 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 KuduTestHarness? Mainly for composition and separation of concerns. 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@91 PS1, Line 91: private static final String bindAddr = "127.0.0.1"; nit: Should this be all caps? http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@111 PS1, Line 111: public void test() throws IOException { I think a test that ensures a downed flaky test endpoint doesn't fail tests or make them extremely slow could be useful. 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 -- 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 16:00:55 +0000 Gerrit-HasComments: Yes
