impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4735: Upgrade pytest in python env to version 2.9.2.
Date Fri, 27 Jan 2017 16:52:12 GMT
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:

File tests/

PS6, Line 151: 
Why did you get rid of this string replacement?

PS6, Line 102:
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

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 yes, could that
result in:

If you have py.test --foo --bar 1,

Then in the first iteration:
arg will be foo, 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
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