Michael Brown has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9494 )

Change subject: IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle 
exit_code 5
......................................................................


Patch Set 4:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/9494/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9494/4//COMMIT_MSG@7
PS4, Line 7: IMPALA-5886 & IMPALA-4812 Update run-tests.py script to handle
> The commit convention is:
Agreed. And also, please explain testing done.

For IMPALA-5886, the problem is pytest is run in several phases, and a specific 
match on a test might run a test in one of the phases only (say, just serial), 
but no tests collected during the parallel or custom cluster phases. On master, 
that results in a failure, but that's wrong. Ideally you'd test various 
scenarios like this, testing running tests in only one of the phases, to make 
sure the return code is correct. For IMPALA-4812, run-tests.py --collect-only 
should mimic impala-py.test --collect-only as much as possible.

Here's a good matrix to play with:

-m: nothing; execute_serially; "not execute_serially"

-k: nothing or some pattern match

module args: nothing or a module that has all serial, a mix of serial/parallel, 
just parallel


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py
File tests/run-tests.py:

http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@30
PS4, Line 30: from _pytest.runner import runtestprotocol
Is this used?


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@65
PS4, Line 65: class TestStatisticsPlugin:
Please use new-style classes, and inherit from object.

  class TestStatisticsPlugin(object)

Should this just be call TestCounterPlugin ?


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@67
PS4, Line 67:   def __init__(self):
            :       self.tests_collected = []
            :       self.tests_executed = []
2 spaces, please.


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@71
PS4, Line 71: # items represents the list of collected test items
> Use Python's docstring comment instead.
Agreed.


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@72
PS4, Line 72:   def pytest_collection_modifyitems(self, items):
For these special hooks I think it's important to note that the name is 
significant and point to documentation like 
https://docs.pytest.org/en/2.9.2/writing_plugins.html#_pytest.hookspec.pytest_collection_modifyitems


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@96
PS4, Line 96:     if "--collect-only" in args:
> In general, we use single quotes for strings in Python.
Unfortunately there's a mix of styles in play here. I think it's fine to use 
whatever quote system dominates the file.


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@97
PS4, Line 97:       for test in plugin.tests_collected:
            :         print(test)
Not sure this is necessary. It looks like you're printing tests twice: once is 
pytest doing it, the other is this print statement. I also found a bug 
somewhere:

  ./run-tests.py --collect-only -m "execute_serially" 
query_test/test_tpch_queries.py

should not select any tests because none of the test cases are serial, but your 
patch prints them.


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@100
PS4, Line 100:     if exit_code == 5:
> Some documentation on what exit_code 5 means?
I wonder if this would work?
  import pytest
  # In my environment, the line above needed to run before the line below.
  from _pytest.main import EXIT_NOTESTSCOLLECTED


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@244
PS4, Line 244:     print_metrics('connections')
Please make sure to maintain consistent print_metrics() usage. I think you may 
have omitted some calls that were there before.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 4
Gerrit-Owner: Nithya Janarthanan <njanartha...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Nithya Janarthanan <njanartha...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 22:58:59 +0000
Gerrit-HasComments: Yes

Reply via email to