impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4735: Upgrade pytest in python env to version 2.9.2.
Date Sat, 28 Jan 2017 02:30:29 GMT
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:

File tests/

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:
> Just say IMPALA-4510 so that when the Jira site is moved from Cloudera to A

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

PS6, Line 128:     for arg in commandline_args:
             :       try:
             :         pytest.config.getvalue(arg.strip('-'))  # Raises ValueError if invalid
             :         kept_args += [arg, str(]
> 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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I40d129e0e63ca5bee126bac6ac923abb3c7e0a67
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-HasComments: Yes

View raw message