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-4510: Selectively filter args for metric verification tests
Date Mon, 21 Nov 2016 21:49:21 GMT
David Knupp has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 2:

(7 comments)

Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/5135/2//COMMIT_MSG
Commit Message:

PS2, Line 9: impala-pytest
> impala-py.test
Done


PS2, Line 10:  separate individual
> You can just use one of these two words. :)
Done


Line 28: 
> Please mention the testing you performed for this patch.
Done


http://gerrit.cloudera.org:8080/#/c/5135/2/tests/run-tests.py
File tests/run-tests.py:

Line 73:   Modify and return the command line arguments that will be passed to pytest.
> Can you also talk about what log_base_name and valid_dirs need to be for a 
Done


PS2, Line 107:     # We need to account for the fact that '--foo bar' and '--foo=bar' might
             :     # be supplied by the user.
             :     raw_args = itertools.chain(*[arg.split('=') for arg in sys.argv[1:]])
> Crafty. It would help a reader to show a simple before/after example of wha
Done


PS2, Line 110:     filtered_args = []
> The name here is strange to me. These args aren't being filtered, but in fa
Done


PS2, Line 113:       try:
             :         pytest.config.getvalue(arg.strip('-'))  # Raises ValueError if invalid
arg
             :         filtered_args += [arg, str(raw_args.next())]
             :       except ValueError:
             :         continue
> This is related to the L75 docstring comment above: it might make more sens
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message