Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11128 )
Change subject: IMPALA-7399: Add script in lib/python to generate junit XML. ...................................................................... Patch Set 5: Code-Review+2 (5 comments) I was able to cherrypick this change and run the tool. It looked to me like it was doing what it was promising to do. I think it's fine as is, though I have a few minor things that you could fix below. http://gerrit.cloudera.org:8080/#/c/11128/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11128/2//COMMIT_MSG@11 PS2, Line 11: python library for Impala development that can be pip installed into > I think we have some options with regard to usage. I was undecided about wh Ok. I'm not overly fond of having multiple virtualenvs around (it's very confusing), but we could probably also seed a virtualenv with this stuff first, and then add the rest of impala-python to it during the build. Anyway, it's not particularly germane here yet. http://gerrit.cloudera.org:8080/#/c/11128/5/lib/python/impala_py_lib/jenkins/generate_junitxml.py File lib/python/impala_py_lib/jenkins/generate_junitxml.py: http://gerrit.cloudera.org:8080/#/c/11128/5/lib/python/impala_py_lib/jenkins/generate_junitxml.py@49 PS5, Line 49: elif os.path.isfile(file_or_string): This is funky, but you want "os.path.exists" here. It breaks magic things like /dev/fd/63 here, and you probably don't want that. $z/bin/generate-junitxml --step hey --error hey --stdout <(echo hi) Generated: ./logs/extra_junit_xml_logs/generate_junitxml.buildall.hey.20180807_22_33_01.xml 15:33:01 mystery impala [master ?] ~/src/impala $cat ./logs/extra_junit_xml_logs/generate_junitxml.buildall.hey.20180807_22_33_01.xml <?xml version="1.0" ?> <testsuites disabled="0" errors="1" failures="0" tests="1" time="0.0"> <testsuite disabled="0" errors="1" failures="0" file="None" log="None" name="generate_junitxml.buildall.hey" skipped="0" tests="1" time="0" timestamp="2018-08-07 22:33:01.010852+00:00" url="None"> <testcase classname="generate_junitxml.buildall" name="hey"> <error message="hey" type="error"/> <system-out>/dev/fd/63</system-out> </testcase> </testsuite> </testsuites> http://gerrit.cloudera.org:8080/#/c/11128/5/lib/python/impala_py_lib/jenkins/generate_junitxml.py@89 PS5, Line 89: """[Required] Specific build step or child script being run. I tend to find the [Required] as a bit redundant given the existing usage: usage: generate-junitxml [-h] [--phase PHASE] --step STEP [-t TIME] [--stdout STDOUT] [--stderr STDERR] [--error ERROR] optional arguments: -h, --help show this help message and exit --phase PHASE General build phase or script. (default: buildall) --step STEP [Required] Specific build step or child script being run. Each step must be unique for the given build phase. (default: None) -t TIME, --time TIME If known, the elapsed time in seconds for this step. (default: 0) --stdout STDOUT Standard output to include in the XML report. Can be either a plain string, or the path to a file print out. (default: None) --stderr STDERR Standard error to include in the XML report. Can be either a string, or the path to a file. (default: None) --error ERROR If specified, the XML report will mark this as an error. This should be a brief explanation for the error. (default: None) http://gerrit.cloudera.org:8080/#/c/11128/5/lib/python/impala_py_lib/jenkins/generate_junitxml.py@99 PS5, Line 99: either a plain string, or the path to a file print out.""") nit: "either a plain string or a path" You don't need the comma before "," I think. http://gerrit.cloudera.org:8080/#/c/11128/5/lib/python/impala_py_lib/jenkins/generate_junitxml.py@104 PS5, Line 104: either a string, or the path to a file.""") same nit: I don't think you need the comma here. -- To view, visit http://gerrit.cloudera.org:8080/11128 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6024d74075ea69b8ee20d1fc3cc9c1ff821ba5b Gerrit-Change-Number: 11128 Gerrit-PatchSet: 5 Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Tue, 07 Aug 2018 22:37:01 +0000 Gerrit-HasComments: Yes
