Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8757 )
Change subject: KUDU-2216. Post process gtest generated xml to include the output from the *.txt files ...................................................................... Patch Set 12: (12 comments) http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py File build-support/jenkins/add_std_out_to_junit.py: http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@1 PS12, Line 1: #!/usr/bin/env python nit: rename this file to add_gtest_stdout_to_junit_xml.py http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@66 PS12, Line 66: log_location nit: s/log_location/log_path/ since path is understood to be a string and location is vague and the object could be something fancy http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@76 PS12, Line 76: "{0}".format(log_location) seems like a pointless format() call? http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@86 PS12, Line 86: while gz_index < len_gz and xml_index < len_xml: This lexicographic comparison logic is hard for a reader of the code to follow in my opinion, even with the code comments. Something along these lines would reduce the line count and complexity considerably, and be straightforward to follow: for xml_file_path in sorted(glob.glob(os.path.join(log_location, "*.xml"))): # Remove the extension. file_path_no_ext = os.path.splitext(xml_file_path)[0] gz_file_path = "%s.gz" % (file_path_no_ext,) if not os.path.exists(gz_file_path): continue tree = ET.parse(xml_filename) root = tree.getroot() ... http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@90 PS12, Line 90: print ("gz: {0}, xml: {1}".format(gz_filename, xml_filename)) is this for debugging? if so we typically remove these before checking in the code http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@108 PS12, Line 108: try: consider using the "with" idiom to auto-close the files http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@109 PS12, Line 109: = nit: missing space before equals sign http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@135 PS12, Line 135: "{0}".format(xml_files[xml_index]) what is the purpose of using format() here? can't we just pass in xml_file_path (in the context of my above suggestion) http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@141 PS12, Line 141: # Write out the updated file. this comment is out of place http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@148 PS12, Line 148: / nit: remove the trailing slash because we add a slash later when joining the path segments http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/build-and-test.sh@585 PS12, Line 585: t nit: Finish sentences in code comments with punctuation. http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/build-and-test.sh@586 PS12, Line 586: nit: extra space -- To view, visit http://gerrit.cloudera.org:8080/8757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f4a9a147f29e37e380cb32ff5af43e1290a1a70 Gerrit-Change-Number: 8757 Gerrit-PatchSet: 12 Gerrit-Owner: Edward Fancher <[email protected]> Gerrit-Reviewer: Edward Fancher <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 15 Dec 2017 00:01:31 +0000 Gerrit-HasComments: Yes
