Michael Brown has posted comments on this change. Change subject: IMPALA-4735: Upgrade pytest in python env to version 2.9.2. ......................................................................
Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/5640/6/tests/run-tests.py File tests/run-tests.py: PS6, Line 151: Why did you get rid of this string replacement? PS6, Line 102: https://issues.cloudera.org/browse/IMPALA-4510 Just say IMPALA-4510 so that when the Jira site is moved from Cloudera to ASF, the comment is still valid. PS6, Line 119: logging_args += (arg, os.path.join(RESULT_DIR, log.format(base_name))) I think it would be more readable to use logging_args.extend() in this case. I spent time in this review trying to see if you unpacked what I assumed were tuples, because I thought you were apending these as tuples. PS6, Line 125: verifcation verification PS6, Line 128: for arg in commandline_args: : try: : pytest.config.getvalue(arg.strip('-')) # Raises ValueError if invalid arg : kept_args += [arg, str(commandline_args.next())] Cheeky. Are there any py.test config options that don't take arguments? If yes, could that result in: If you have py.test --foo --bar 1, Then in the first iteration: arg will be foo, commandline_args.next() will be bar In the second iteration: arg will be 1. You'll do a getvalue() on it, raise ValueError, and ultimately 1 won't be applied to bar. -- To view, visit http://gerrit.cloudera.org:8080/5640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40d129e0e63ca5bee126bac6ac923abb3c7e0a67 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
