impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
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:

(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 <dknupp@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message