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 2: (11 comments) Thanks. This largely looks fine. I've got some nits below, and a question about the naming, but nothing really interesting. The only thing I'm worried about is the dependencies. The goal here, I suspect, is to make it so that if various "interesting" parts of the build fail, we can generate a JUnit XML file for them. If the build fails before we can install this tool, we're out of luck. So, the question is whether we make this tool simpler, by just templating over a fixed XML file, and using Python's built-in capabilities to quote XML (I'm pretty sure there are some) as opposed to using the junit-xml library. I don't think this is a blocking worry. If we find we want this capability earlier in the build, we can write the poor man's shell script that does that or remove the dependency later. 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 You're going to update bootstrap* in a separate commit? This doesn't currently get installed anywhere, I think. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py File lib/python/jenkins/impala_junitxml_logger.py: http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@1 PS2, Line 1: #!/usr/bin/env python Bike shed on the naming of "jenkins_junitxml_logger.py" I don't think this has anything to do with Jenkins. I.e., it largely makes sense (and will get invoked) even outside of Jenkins. I think this could just be "generate-junitxml" as a script name. (I.e., I don't think we need to prefix things with "impala-". http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@36 PS2, Line 36: # The equivalent of mkdir -p : try: : os.makedirs(JUNITXML_LOGDIR) : except OSError as e: : if e.errno == errno.EEXIST and os.path.isdir(JUNITXML_LOGDIR): : pass : else: : raise Please put this into main(). http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@60 PS2, Line 60: with open(file_or_string, 'r') as f: : content = f.read() This is shorter: "return open(file_or_string).read()" http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@82 PS2, Line 82: raise OSError("File already exists: {}".format(junit_log_file)) There are scenarios where this is too picky. When I run unit tests over and over on my laptop, I don't have to clean up the intermediate outputs. JUnit either overwrites them or adds new ones. I think the behavior here should be similarly permissive, either by creating a unique filename (if one already exists) or simply overwriting. Either is fine by me. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@97 PS2, Line 97: help="General build phase or script [default = buildall.") nit: [default = buildall]. Secondary nit: You can possibly use argparse.ArgumentParser(formatter=argparse.ArgumentDefaultsHelpFormatter) to avoid this. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@107 PS2, Line 107: help="If known, the amount of elapsed time for this step.") Mention that this is in seconds. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@112 PS2, Line 112: either a plain string, or the path to a file print out.""") nit: "Can be either a string or the path to a file." http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@140 PS2, Line 140: equivalent to a "test case", must be unique. If an output file already exists because this is invalid, do you want to create a unique file anyway? (Given the context, I don't think failing this script is very useful.) http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@160 PS2, Line 160: xml_report = generate_xml_file(testsuite) I would pass in the root directory here too instead of having a global. You're only saving yourself an argument in two places in exchange for a global; I think not having the global is probably better. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@161 PS2, Line 161: print("Generated: {}".format(xml_report)) Does this need a "from future import print_function" thingy? -- 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: 2 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: Mon, 06 Aug 2018 17:01:06 +0000 Gerrit-HasComments: Yes
