You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2018/12/06 03:59:36 UTC
[kudu-CR] java: add support for flaky test reporting
Hello Adar Dembo, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12042
to review the following change.
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate
TestResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
This appears to work after testing it manually. Example:
http://dist-test.cloudera.org:8080/test_drilldown?test_name=testScanWithLimit%28org.apache.kudu.client.TestKuduClient%29
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
5 files changed, 358 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/1
--
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: newchange
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 3:
(3 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:
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: public Options revision(String revision) {
> What's your expected behavior if KUDU_REPORT_TEST_RESULTS is set but one of
Good question. I'd like to fail as fast and hard as possible. Meaning, rather than running through each and every test and failing it, if we could fail the entire test run that'd be ideal.
If not, failing each and every test is a reasonable fallback.
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: }
> https://stackoverflow.com/questions/7348711 seems to be saying this is like
Where do you see that? The top-voted answer describes how InetAddress.getLocalHost().getHostName() does reverse DNS and generally won't produce the same result as running `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: IsVarSetAndNonEmpty(REPORT_TIMEOUT_VAR) &&
> I'm fine with doing this before we push the series but I'd like to do it in
That's fine. Let's explore options and see if we can't get this working with the first cut of this feature, but if it's too hard I'm also OK with enabling flaky test tracking without log files initially.
--
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: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 21:24:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Dec 2018 16:00:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+2
--
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: 8
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Apr 2019 13:17:49 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12042/7/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/7/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@173
PS7, Line 173: if (result == ResultReporter.Result.SUCCESS) {
> Is there a reason that we create a test log on success instead of failure?
Whoops, that was just a mistake on my part. I'll fix it.
--
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: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Apr 2019 00:55:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Dec 2018 18:52:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Patch Set 1:
(19 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
Done
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.
Done
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 {
> I was just curious the reasoning since it's a different pattern than the ot
I don't have a strong opinion on this so I am leaving it as is unless someone speaks up again.
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
It's more intuitive to report results if and only if there's a ResultReporter, but if we create a reporter only when results are being reported to some remote server then the value of the ResultReporter member in RetryRule will be null when results aren't being reported. I'd rather embrace the idea that a ResultReporter can no-op report results and gain the guarantee that the ResultReporter member won't be null rather than do the most intuitive thing.
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 vali
What's your expected behavior if KUDU_REPORT_TEST_RESULTS is set but one of the required env variables isn't set? Throw?
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() {
> Yes, for consistency. Is there a Java SDK call for that?
https://stackoverflow.com/questions/7348711 seems to be saying this is like calling `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'm worried that not doing this now will yield two sets of data points: one
I'm fine with doing this before we push the series but I'd like to do it in a separate patch.
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
I don't think this really matters. There isn't any significant amount of work that we'd be caching. The hostname, which does involve work to get, is cached.
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
Done
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.
Done
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.
Done
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?
Done
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:
Done
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.
Done
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.
Done
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?
Done
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?
Done
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
`tryReportResult` ensures that failing to report a result doesn't fail the test.
I think potentially slowing down the tests is necessary because we have to try to connect to the flaky test service, and we might need to wait the timeout each time. We'd have to refactor the thing to aggregate results and bulk report them at the end. I'd prefer to wait to do that until we have problems (perfect is the enemy of good).
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")
> The song was stuck in my head yesterday... I've got a niece that's into it
/|
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 21:00:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#4) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate
TestResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
7 files changed, 608 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/4
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#5) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate
TestResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
8 files changed, 672 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/5
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 5
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#2) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate
TestResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
This appears to work after testing it manually. Example:
http://dist-test.cloudera.org:8080/test_drilldown?test_name=testScanWithLimit%28org.apache.kudu.client.TestKuduClient%29
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
5 files changed, 356 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/2
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 9: Code-Review+2
--
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: 9
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Apr 2019 13:35:45 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 4:
I manually tested the following:
- Reporting off.
- Reporting on, unreachable test result server (tests passed but had ugly exceptions in their log output).
- Reporting on, local test result server.
- Reporting on, remote test result server.
- Reporting on both C++ and Java tests, remote result server.
I feel fairly confident about it, but I'm sure we'll need to iron out some issues once it's been enabled.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 00:51:58 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12042/7/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/7/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@173
PS7, Line 173: if (result == ResultReporter.Result.SUCCESS) {
Is there a reason that we create a test log on success instead of failure? We are doing the opposite in the appender but I guess it doesn't really matter in this case since we are testing both paths and thus have coverage.
--
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: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 21:58:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
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: deleteReviewer
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 9
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Patch Set 7: Code-Review+2
--
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: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Mar 2019 04:20:56 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 06 Dec 2018 19:36:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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 {
> It's an options class with a fluent API. It does not have a build() method
I was just curious the reasoning since it's a different pattern than the other builders in the repo.
There are some minor trade offs, but I don't feel strongly one way or the other.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 06 Dec 2018 19:46:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Patch Set 2:
Updating with a rebase on master. No other changes intended.
--
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: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 17:30:45 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#9) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate ResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
8 files changed, 721 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/9
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 9
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#7) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate ResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
8 files changed, 720 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/7
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 02:30:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Patch Set 5:
(5 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
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?
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?
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 properly configured?
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 further logging before reporting a failure?
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 18:15:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+2
--
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: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Mar 2019 14:02:48 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 9: Verified+1
Overriding Jenkins, unrelated test failure.
--
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: 9
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Apr 2019 03:25:08 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#3) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate
TestResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
This appears to work after testing it manually. Example:
http://dist-test.cloudera.org:8080/test_drilldown?test_name=testScanWithLimit%28org.apache.kudu.client.TestKuduClient%29
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
5 files changed, 425 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/3
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#6) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate ResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
8 files changed, 720 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/6
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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:
(2 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:
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() {
> So you think we should just get this from `hostname` ?
Yes, for consistency. Is there a Java SDK call for that?
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.
> Yeah that's intended as a follow-on. The statistics seem useful to me, even
I'm worried that not doing this now will yield two sets of data points: one with logs and one without. It'll be annoying to deal with in the future.
If it's not much work I'd rather just see it done now.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 06 Dec 2018 19:58:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#8) to the change originally created by Mike Percy. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate ResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
8 files changed, 720 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12042/8
--
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: newpatchset
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 8
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+2
--
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: 8
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Apr 2019 01:41:47 +0000
Gerrit-HasComments: No
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
java: add support for flaky test reporting
This patch hooks into the existing RetryRule to report test results to
the flaky test server inline as the tests are executed. All of the
actual reporting logic is factored out into a separate ResultReporter class.
The interface for the test reporter to pass relevant information about
the build environment to the flaky test server is based on environment
variables. This includes configuration and build metadata such as flaky
test server address, git revision, build id, build host, and build type.
This patch also includes a simple integration test for the reporter
using a mocked-up flaky test server HTTP endpoint.
This patch does not integrate the above functionality into the build.
That will happen in a follow-up patch.
Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Reviewed-on: http://gerrit.cloudera.org:8080/12042
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M java/gradle/dependencies.gradle
M java/kudu-test-utils/build.gradle
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java
M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java
8 files changed, 721 insertions(+), 18 deletions(-)
Approvals:
Adar Dembo: Verified
Grant Henke: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74
Gerrit-Change-Number: 12042
Gerrit-PatchSet: 10
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 22:08:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Patch Set 1:
(1 comment)
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@106
PS1, Line 106: private static String tryLookupHostname() {
> Where do you see that? The top-voted answer describes how InetAddress.getLo
I think that answer is not clear at all about what the code does. It just points out some ways that some particular hostname may not be good in all circumstances while kinda-maybe-sorta hinting that it thinks that the code does DNS. A comment on that answer says if you look at the code it actually does the equivalent of the `hostname` command.
The takeaway I got from the entire bunch of answers and comments is that the situation is ¯\_(?)_/¯ and that the way it's done here is the most portable and is somewhat reliable. So we ought to stick with it until it's a problem, I think.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Mar 2019 21:33:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2
(1 comment)
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
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 17:04:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 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 app
I wasn't sure whether we wanted to explicitly list all direct dependencies, or whether we're comfortable consuming them transitively. I'll remove this.
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?
No, which is yet another reason why separating the file logic into a separate appender is compelling.
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
I went back and forth on this. IIRC I settled on the combined appender because I had a case where I always wanted to set it up, but I only sometimes wanted to log to a file.
But, I ended up cleaning up ResultReporter further, so that case no longer exists. I'll split this up.
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.
Done
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.
Done
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
I think Will may have added them but forgot to use them. Yeah, I can wire them up to KUDU_REPORT_TEST_RESULTS_VAR and TEST_RESULT_SERVER_VAR.
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()`
See previous discussion in this change between Will and myself. tl;dr: it does a reverse DNS lookup, which is very different than calling `hostname`.
I'll add a comment.
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.
Oh yeah, at some point I had expressed this via @InterfaceAudience but then lost that. Will fix.
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 buil
Done
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/dis
Done
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?
It doesn't really matter since it'll have a different filename when sent to the server, but sure I can add it.
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?
Oops, 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: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 04:24:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12042/6/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/6/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@58
PS6, Line 58: skipReporting
> nit: Might be missing something, shouldn't this be reversed? I.e.
You're not missing anything; I'm missing a grasp of English. 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: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Mar 2019 04:19:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting
......................................................................
Patch Set 6:
(4 comments)
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: finish(): all events previously captured in append() are now guaranteed to
: * be on disk and visible to readers
> I don't think it matters. In fact, the CapturingLogAppender use case is one
Ack
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;
> The default value doesn't really matter, because consider how the ResultRep
Makes sense, thanks for the explanation
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: throw t;
> This whole thing is kind of funky.
I see, yeah I wasn't sure about how this closeable worked, but i think it is more understandable now.
http://gerrit.cloudera.org:8080/#/c/12042/6/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/6/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@58
PS6, Line 58: skipReporting
nit: Might be missing something, shouldn't this be reversed? I.e.
skipReporting ? null : new ResultReporter()
Seems you're using it as enableReporting, but the name seems a bit confusing.
--
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: 6
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Mar 2019 00:08:52 +0000
Gerrit-HasComments: Yes