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 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/8757/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8757/7//COMMIT_MSG@10 PS7, Line 10: The gtest junit output doesn't include the output from the gtest run. Hmm... do we want *all* of the stdout? Isn't that many megabytes in some cases? Are we sure this is what we want? I see that you filed a JIRA to track this. Can you explain why this is useful? http://gerrit.cloudera.org:8080/#/c/8757/7/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/7/build-support/jenkins/add_std_out_to_junit.py@36 PS7, Line 36: # Matches illegal characters as of the XML 1.1 spec In your comment, please explain why we need this. Also, punctuation here and elsewhere. http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@39 PS7, Line 39: accumul This is a very mechanical name for a variable, almost as unhelpful as naming something "x" or "n". Can you give it a more semantic name? i.e. on a car, instead of naming something "nut", it's better to name it "axle nut". http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@49 PS7, Line 49: system-out nit: Mind using "stdout" instead of system-out? I think stdout will be very clear what it is. http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@63 PS7, Line 63: while gz_index < len_gz and xml_index < len_xml: Mind adding a comment here explaining what this loop is trying to accomplish? Like: # Loop over all of the gz and xml files and extract their contents for inclusion into junit results. or something http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@64 PS7, Line 64: remove the extensions style nit: In comments, capitalize your sentences and end the comment with punctuation such as a period. http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@68 PS7, Line 68: gz_filename < xml_filename what does < mean here? do you mean len(gz_filename) < len(xml_filename) ? http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@69 PS7, Line 69: gz_index = gz_index +1 nit: prefer the idiom gz_index += 1 -- 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: 7 Gerrit-Owner: Edward Fancher <e...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 12 Dec 2017 21:53:24 +0000 Gerrit-HasComments: Yes
