Michael Brown has posted comments on this change. Change subject: IMPALA-3608: Allow multiple different exception messages ......................................................................
Patch Set 1: (6 comments) You ran a private build to make sure .test file parsing still works, right? http://gerrit.cloudera.org:8080/#/c/3205/1//COMMIT_MSG Commit Message: Line 7: IMPALA-3608: Allow multiple different exception messages Can you specify this change is for test infrastructure, or end to end tests, or similar? http://gerrit.cloudera.org:8080/#/c/3205/1/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: Line 192: def verify_exceptions(self, expected_strs, actual_str, use_db): I think this should be a private method (prefixed with _), or perhaps scoped within run_test_case? Line 197: actual_str = actual_str.replace('\n', '') Why not either of: actual_str.strip() # white space is default or actual_str.strip('\n') Line 199: """ : In error messages, some paths are always qualified and some are not. : So, allow both $NAMENODE and $FILESYSTEM_PREFIX to be used in CATCH. : """ This should be a comment, not a docstring. http://gerrit.cloudera.org:8080/#/c/3205/1/tests/util/test_file_parser.py File tests/util/test_file_parser.py: Line 177: subsection_str = '\n'.join([line for line in lines[1:-1]]) It would add some readability to store lines[1:-1] as a variable. Plus you use it below as well. Line 204: for line in lines[1:-1]: : parsed_sections['CATCH'].append(line) What about: parsed_sections['CATCH'].extend(lines[1:-1]) -- To view, visit http://gerrit.cloudera.org:8080/3205 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie6d81fd3ae601f565b575edfeefff7c5a6c07974 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
