David Knupp 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? Because the primary goal of this change is to pass the args as a list, not as one big monolithic string. We only needed to do this weird string replacement because we were building that one long string. 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 A Done 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 That's fair. I think I decided against .extend([...]) simply to reduce the visual clutter to deeply nested parens/brackets. PS6, Line 125: verifcation > verification Done 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 Note that I didn't actually change the existing logic here, I just changed the name of 'raw_args' to the more descriptive 'commandline_args'. That said, this seems like a valid point. I poked around in pytest's source code for a while, and it appears that any tests modules, classes, and functions explicitly called out on the command line can be retrieved by calling pytest.config.getoption('file_or_dir'). We can use this fact to know which items to remove from from commandline_args. -- 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 <dkn...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-HasComments: Yes
