Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12885 )
Change subject: IMPALA-8371: Return appropriate error code for unified backend tests ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py File bin/gen-backend-test-script.py: http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@20 PS2, Line 20: take nit: spelling http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@26 PS2, Line 26: from optparse import OptionParser optparse has been deprecated since python 2.7, use argparse instead? http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@37 PS2, Line 37: """).lstrip() Is the lstrip needed? http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@41 PS2, Line 41: """ If you prefix this with an r"""...""" you get a literal string and don't need to escape the backslashes http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@63 PS2, Line 63: parser = OptionParser() indent 2 spaces http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@67 PS2, Line 67: if options.gtest_filter is None or options.test_script_output is None: You can mark options as required in argparse http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@77 PS2, Line 77: subprocess.check_call(["chmod", "+x", options.test_script_output]) Could also use os.chmod() http://gerrit.cloudera.org:8080/#/c/12885/2/bin/gen-backend-test-script.py@80 PS2, Line 80: if __name__ == "__main__": main() nit: we usually would put a newline after the : -- To view, visit http://gerrit.cloudera.org:8080/12885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia146d026d42f76d5ea12d92798f299182de03eef Gerrit-Change-Number: 12885 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Comment-Date: Fri, 05 Apr 2019 17:24:57 +0000 Gerrit-HasComments: Yes
