You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2019/12/20 17:13:29 UTC
[kudu-CR] [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14938
Change subject: [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
......................................................................
[WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
This patch adds a Junit `RunListener` that will generate a Jenkins
compliant Junit XML file in the `TEST_LOGDIR` directory. It also
adds the JunitXMLCore classwhich is a wrapper around the original
JunitCore, which was called by dist-test, to add the listener.
The dist-test scripts were updated to set `TEST_LOGDIR` to a
location similar to local java runs so that globing the test results
on Jenkins works in either local or dist-test. Additionally the
generated dist-test isolate files now call JunitXMLCore instead
of JunitCore.
Change-Id: I7f65e0ef0a4c1ed2e79460f74bea6e725643c2ac
---
M build-support/run_dist_test.py
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/TempDirUtils.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXMLCore.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/FailureTest.java
6 files changed, 372 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/14938/1
--
To view, visit http://gerrit.cloudera.org:8080/14938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7f65e0ef0a4c1ed2e79460f74bea6e725643c2ac
Gerrit-Change-Number: 14938
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14938
to look at the new patch set (#2).
Change subject: [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
......................................................................
[WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
This patch adds a Junit `RunListener` that will generate a Jenkins
compliant Junit XML file in the `TEST_LOGDIR` directory. It also
adds the JunitXMLCore classwhich is a wrapper around the original
JunitCore, which was called by dist-test, to add the listener.
The dist-test scripts were updated to set `TEST_LOGDIR` to a
location similar to local java runs so that globing the test results
on Jenkins works in either local or dist-test. Additionally the
generated dist-test isolate files now call JunitXMLCore instead
of JunitCore.
Change-Id: I7f65e0ef0a4c1ed2e79460f74bea6e725643c2ac
---
M build-support/run_dist_test.py
M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/TempDirUtils.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXMLCore.java
A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java
A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/FailureTest.java
6 files changed, 368 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/14938/2
--
To view, visit http://gerrit.cloudera.org:8080/14938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f65e0ef0a4c1ed2e79460f74bea6e725643c2ac
Gerrit-Change-Number: 14938
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14938 )
Change subject: [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
......................................................................
Patch Set 2:
(15 comments)
So I think the use of a JUnit listener makes sense, but in the spirit of reusing existing code, what do you think about this:
1. Replacing the XML writing with emission of special "test start/finish" markers into stdout, and
2. Reusing build-support/parse_test_failure.py to generate the XML files.
That way we have only one place (that Python script) that writes this special XML syntax instead of having both Java and Python implementations. Should be easier to maintain.
http://gerrit.cloudera.org:8080/#/c/14938/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14938/2//COMMIT_MSG@7
PS2, Line 7: [WIP] KUDU-2820: Generate JUnit XML reports for Java test run via dist-test
Have you compared the generated reports with what Gradle would have generated when run locally? Would be interesting to see if there are any meaningful differences.
http://gerrit.cloudera.org:8080/#/c/14938/2//COMMIT_MSG@15
PS2, Line 15: globing
Nit: globbing
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXMLCore.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXMLCore.java:
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXMLCore.java@54
PS2, Line 54: classes.add(Class.forName(arg));
Are we guaranteed that the args are class names? What about when you use the --pattern argument to dist_test.py?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java
File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java:
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@62
PS2, Line 62: public final SimpleDateFormat iso8601DatetimeFormat =
Could also be static.
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@67
PS2, Line 67: private Document document;
Could be final?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@70
PS2, Line 70: private Map<Description, Long> testStarts = new HashMap<>();
: private Map<Description, Failure> testFailures = new HashMap<>();
Kind of surprised these need to be maps; isn't there just one outstanding test at a time?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@100
PS2, Line 100: DocumentBuilder documentBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
: return documentBuilder.newDocument();
Nit: could combine.
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@108
PS2, Line 108: public void testRunStarted(Description description) throws Exception {
Would be helpful to doc somewhere the state machine and all possible transitions, between the states "run started", "run finished", "started", "finished", "failure" and "assumption failure". It's not obvious to me and it'd make it a lot easier to verify the correctness of the handling of the listener's own state.
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@114
PS2, Line 114: name == null
When is this the case?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@190
PS2, Line 190: if (message != null && message.length() > 0) {
Is this really a tri-state (i.e. null, zero-length, and non-zero-length)? I'd be surprised if both null and zero-length were possible.
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@200
PS2, Line 200: Element stdoutElement = document.createElement("system-out");
: testCase.appendChild(stdoutElement);
: Text outData = document.createCDATASection(stdout.toString());
: stdoutElement.appendChild(outData);
:
: Element sterrElement = document.createElement("system-err");
: testCase.appendChild(sterrElement);
: Text errData = document.createCDATASection(stderr.toString());
: sterrElement.appendChild(errData);
Should we skip these if stdout/stderr are empty? Or emit empty elements?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@233
PS2, Line 233: true
Nit: annotate with /* autoFlush= */ or something?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@247
PS2, Line 247: addTimeToTestCaseElement(testCase, description);
Is there a testStarted for every testIgnored? If not, maybe we skip this altogether?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@254
PS2, Line 254: String testName = description.getMethodName();
: testcase.setAttribute("name", testName == null ? "unknown" : testName);
:
: String className = description.getClassName();
: testcase.setAttribute("classname", className == null ? "unknown" : className);
When are testName or className null?
http://gerrit.cloudera.org:8080/#/c/14938/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/JUnitXmlRunListener.java@265
PS2, Line 265: long startTime = testStarts.getOrDefault(description, 0L);
Under what circumstances would description be missing from the map? Doesn't seem like the computed value would be correct.
--
To view, visit http://gerrit.cloudera.org:8080/14938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f65e0ef0a4c1ed2e79460f74bea6e725643c2ac
Gerrit-Change-Number: 14938
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 06 Jan 2020 21:46:43 +0000
Gerrit-HasComments: Yes