Michael Brown has posted comments on this change. Change subject: IMPALA-4510: Selectively filter args for metric verification tests ......................................................................
Patch Set 2: (8 comments) I understand the problem, and I think I see what you're doing to fix it, but it might make things a tad clearer if you address my comments below. Thanks! http://gerrit.cloudera.org:8080/#/c/5135/2//COMMIT_MSG Commit Message: PS2, Line 9: impala-pytest impala-py.test PS2, Line 10: separate individual You can just use one of these two words. :) Line 28: Please mention the testing you performed for this patch. 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 caller? Line 75: The raw command line arguments need to be modified because of the way our Should all of this be in the docstring? It seems like you could instead inline some of this information in relevant places below. 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 what sys.argv[1:] starts as, and then what the resulting raw_args is. PS2, Line 110: filtered_args = [] The name here is strange to me. These args aren't being filtered, but in fact they are getting through your filter, right? What's being filtered actually the options that raise ValueError below, right? 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 sense for a reader to see here that you're saying "If this is a pytest config argh, it should be carried along; otherwise it should be filtered" .... or something similar. -- 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 <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
