Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11235 )
Change subject: IMPALA-7399: Remove third-party dependencies from junit xml script ...................................................................... Patch Set 4: (6 comments) Thanks. The general idea makes sense to me. It's a matter of style, and this is pretty functionally contained, but it seems like the interface here is: junit_report_string(name, elapsed_time, etc.) which is to say that saving this to a file should be the caller's job. Moreover, I don't think having the class for the JunitReport here adds much, since the only reasonable thing to do is to_file(). Again, it's still a matter of style, but I don't like the @property annotations. You're generating an annoying XML structure, and I think that's best done imperatively, in one boring function. We don't need the lazy-loading or the @property stuff. It obscures the simplicity here. I don't feel all that strongly about this stuff, but let me know what you think. http://gerrit.cloudera.org:8080/#/c/11235/4/bin/rat_exclude_files.txt File bin/rat_exclude_files.txt: http://gerrit.cloudera.org:8080/#/c/11235/4/bin/rat_exclude_files.txt@13 PS4, Line 13: bin/generate_junitxml.py Does rat not ignore symlinks by default? http://gerrit.cloudera.org:8080/#/c/11235/4/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/11235/4/lib/python/impala_py_lib/jenkins/generate_junitxml.py@38 PS4, Line 38: """A Junit XML style report parseable by Jenkins for reporting build status. add a comment here about how to use this? http://gerrit.cloudera.org:8080/#/c/11235/4/lib/python/impala_py_lib/jenkins/generate_junitxml.py@64 PS4, Line 64: # Create the testcase element (if it hasn't already been created) : self.testcase_element This is too fancy for my tastes. Perhaps just create the structures at the beginning here and be done with it? http://gerrit.cloudera.org:8080/#/c/11235/4/lib/python/impala_py_lib/jenkins/generate_junitxml.py@69 PS4, Line 69: """Return the testsuites root element, creating it if it doesn't exist.""" I think this would be clearer if you just created this in init(). http://gerrit.cloudera.org:8080/#/c/11235/4/lib/python/impala_py_lib/jenkins/generate_junitxml.py@79 PS4, Line 79: for k, v in base_attributes.iteritems(): : self._root_element.set(k, v) This is just: self._root_element("time", elapsed_time) self._root_element("tests", 1) and so on. It's 4 lines, and you have the four lines on lines 72-75 anyway. I think this would be clearer without the extra loop and just setting these directly. http://gerrit.cloudera.org:8080/#/c/11235/4/lib/python/impala_py_lib/jenkins/generate_junitxml.py@102 PS4, Line 102: self._testsuite_element = ET.SubElement(self.root_element, "testsuite") : for k, v in base_attributes.iteritems(): : self._testsuite_element.set(k, v) Same here: you don't re-use base_attributes ever, so I think it makes sense to just inline this. -- To view, visit http://gerrit.cloudera.org:8080/11235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I958ee0d8420b6a4197aaf0a7e0538a566332ea97 Gerrit-Change-Number: 11235 Gerrit-PatchSet: 4 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: Nithya Janarthanan <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Wed, 15 Aug 2018 21:35:16 +0000 Gerrit-HasComments: Yes
