Edward Fancher 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 10: (9 comments) 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.0 spec. > In your comment, please explain why we need this. Also, punctuation here an Done http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@39 PS7, Line 39: y unhel > This is a very mechanical name for a variable, almost as unhelpful as namin Done http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@40 PS7, Line 40: # so we need to strip these characters out from the gtest output before > Add docstring here and for process_logs. Done http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@46 PS7, Line 46: Given a suite name and test name, will walk the juni tree to find the > We like to keep our code comments clean, please start with uppercase and en Done http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@49 PS7, Line 49: here. > nit: Mind using "stdout" instead of system-out? I think stdout will be very system-out is what's mentioned in the junit jenkins schema: https://github.com/junit-team/junit5/blob/master/platform-tests/src/test/resources/jenkins-junit.xsd#L70 (That may or may not be authoritative, but it was the best I could find.) http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@63 PS7, Line 63: child.text = ''.join(system_out_node_text_lines) > Mind adding a comment here explaining what this loop is trying to accomplis Done http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@64 PS7, Line 64: > style nit: In comments, capitalize your sentences and end the comment with Done http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@68 PS7, Line 68: obs for *.gz ands *. xml l > what does < mean here? do you mean len(gz_filename) < len(xml_filename) ? I added a comment for that. Basically, since they are sorted lexicographically, I'm using it to keep two running pointers between sorted lists to compare them one at a time, the same way you would for combining two sorted lists. http://gerrit.cloudera.org:8080/#/c/8757/7/build-support/jenkins/add_std_out_to_junit.py@69 PS7, Line 69: lphabetically. I use t > nit: prefer the idiom gz_index += 1 Done -- 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: 10 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: Wed, 13 Dec 2017 23:25:13 +0000 Gerrit-HasComments: Yes
