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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 06 Jan 2020 21:46:43 +0000 Gerrit-HasComments: Yes
