[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 15: Verified+1


--
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: 15
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Tue, 27 Mar 2018 22:02:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9494 )

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

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

exit_code for EE tests when no tests are collected.After this
change return_code will be either 0 if no tests are expected
to be collected (dry-run) and 1 if tests are expected to be
collected but are not collected due to some error

Testing:
 - Ran end-to-end shell, hs2 tests for IMPALA-5886 with debug
   statements to verify the exit_codes
 - Ran end-to-end shell tests with collect-only for IMPALA-4812

Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Reviewed-on: http://gerrit.cloudera.org:8080/9494
Reviewed-by: David Knupp 
Tested-by: Impala Public Jenkins
---
M tests/run-tests.py
1 file changed, 69 insertions(+), 22 deletions(-)

Approvals:
  David Knupp: Looks good to me, approved
  Impala Public Jenkins: Verified

--
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: merged
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 16
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 15:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2185/


--
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: 15
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Tue, 27 Mar 2018 18:14:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[David Knupp (Code Review)]

David Knupp 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 15: Code-Review+2

Carrying the +2 after the rebase


--
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: 15
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Tue, 27 Mar 2018 18:13:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[David Knupp (Code Review)]

David Knupp 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 14: Code-Review+2


--
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: 14
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Tue, 27 Mar 2018 18:00:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#14). ( 
http://gerrit.cloudera.org:8080/9494 )

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

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

exit_code for EE tests when no tests are collected.After this
change return_code will be either 0 if no tests are expected
to be collected (dry-run) and 1 if tests are expected to be
collected but are not collected due to some error

Testing:
 - Ran end-to-end shell, hs2 tests for IMPALA-5886 with debug
   statements to verify the exit_codes
 - Ran end-to-end shell tests with collect-only for IMPALA-4812

Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
---
M tests/run-tests.py
1 file changed, 69 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/9494/14
--
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: newpatchset
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 14
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[David Knupp (Code Review)]

David Knupp 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 12:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@112
PS11, Line 112: d on the outcome of test run when no tests
> Patch Set 12:
>
> (7 comments)

Since you didn't include your command line, it's not clear what tests you were 
running, so I'm not sure what to make of your debug output. But here's an 
illustration of what I was trying to get at.

I've added a dummy test that simply asserts True (called "test_always_passes") 
to one of our test modules. Because I didn't mark it as a serial test, 
run-tests.py should find this test when it tries to run the parallel tests.

BEFORE your patch, when I try to run just this test in isolation, run-tests.py 
finds nothing in the serial tests, stress, or custom cluster tests (as 
expected), but it does successfully find and execute the test in the parallel 
test run.

  $ impala-python run-tests.py shell/test_shell_commandline.py -k 
test_always_passes
   test session starts =
  [...]
  scheduling tests via LoadScheduling

  shell/test_shell_commandline.py::test_always_passes
  [gw1] PASSED shell/test_shell_commandline.py::test_always_passes
  generated xml file: /Impala/logs/ee_tests/results/TEST-impala-parallel.xml
  == 1 passed in 1.22 seconds ==


When I query the run-tests.py exit code, it should be zero, however:

  systest@impala-tc-u1404-client:~/Impala/tests$ echo $?
  1  # < This is a bug. Everything ran as expected, and passed.


For a second use case, I'll try to run a test that I know is NOT a valid test.

  $ impala-python run-tests.py shell/test_shell_commandline.py -k 
test_does_not_exist
  [...no tests are found in any group, obviously...]


Now when I query the exit code, it should show that there was an error, and it 
does.

  systest@impala-tc-u1404-client:~/Impala/tests$ echo $?
  1  # < This is correct. We tried a non-existent test, so we should 
exit with an error.


AFTER your patch, when I try the same experiment, I  can see that the first 
issue has indeed been corrected.

  $ impala-python run-tests.py shell/test_shell_commandline.py -k 
test_always_passes
   test session starts =
  [...]
  scheduling tests via LoadScheduling

  shell/test_shell_commandline.py::test_always_passes
  [gw1] PASSED shell/test_shell_commandline.py::test_always_passes
generated xml file: /Impala/logs/ee_tests/results/TEST-impala-parallel.xml
  == 1 passed in 1.22 seconds ==
  systest@impala-tc-u1404-client:~/Impala/tests$ echo $?
  0  # < This is correct. We found the test, ran it, and it passed.


However, now there's a problem in the second use case. If I try to run an 
invalid test, run-tests.py tells me that everything is OK.

  $ impala-python run-tests.py shell/test_shell_commandline.py -k 
test_does_not_exist
  [...no tests are found in any group, obviously...]
   no tests ran in 1.22 seconds 
  systest@impala-tc-u1404-client:~/Impala/tests$ echo $?
  0  # < This is a new bug. Exit code should not be zero in this case.


This is also what I was trying to get at with the earlier suggestion that the 
pytest exit code and the overall run-tests.py exit code are different things. 
The pytest exit status is evaluated after each invocation of pytest. The 
overall run-tests.py exit code can only be evaluated at the very end, after 
we've gone through all of our pytest attempts.

I'm sorry that I didn't make this more clear 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: 12
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Sun, 11 Mar 2018 23:51:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 11:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9494/9/tests/run-tests.py@63
PS9, Line 63: class TestCounterPlugin(object):
> Extend from (object) just like TestExecutor definition. In general it's pre
Done


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

http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@107
PS11, Line 107: #though 5 is a normal return code, our jenkins handles 
non-zero RC as error in
> This is odd wording in this context. Don't forget that this code goes into
Done



--
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: 11
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Sat, 10 Mar 2018 15:38:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#12). ( 
http://gerrit.cloudera.org:8080/9494 )

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

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

exit_code for EE tests when no tests are collected.After this
change return_code will be either 0 if no tests are expected
to be collected (dry-run) and 1 if tests are expected to be
collected but are not collected due to some error

Testing:
 - Ran end-to-end shell, hs2 tests for IMPALA-5886 with debug
   statements to verify the exit_codes
 - Ran end-to-end shell tests with collect-only for IMPALA-4812

Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
---
M tests/run-tests.py
1 file changed, 70 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/9494/12
--
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: newpatchset
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 12
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@112
PS11, Line 112: len(testcounterplugin.tests_executed) <= 0
> I think this comment was missed from before as well. I don't thinks len(tes
Hi David,

I think the current placement of this logic is fine because we are handling the 
non-zero exit code of 5 inside the test_executor. Lets say for example 
pytest.main gets invoked for "serially" and there were no tests to run, it 
would default the exit_code to 0 and then after that when parallel gets called 
and there were tests to execute and they pass then the exit_code would be 0 and 
1 if they fail, While before for the same sequence of events RC would be a 5 
and 0 or 5 and 1 , but now it would be 0 or 1.

So I don't think we need to move this logic to be inside main

So going forward systest will exit(1) only when the tests have genuinely failed 
and not when no tests were collected

if exit_code == EXIT_NOTESTSCOLLECTED:
  if len(testcounterplugin.tests_executed) <= 0 :
exit_code = 0
  else:
exit_code = 1

This would take care of setting the exit_code accordingly thereby avoiding 
run-tests exiting because there was a pytest RC of 5 (no tests collected)



--
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: 11
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Sat, 10 Mar 2018 14:18:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[David Knupp (Code Review)]

David Knupp 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 11:

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/9494/9/tests/run-tests.py@66
PS9, Line 66:   tests_collected is a collection of items
tests_collected actually winds up being a list of node ids, not test instances.


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

http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@64
PS11, Line 64:   """ Custom pytest plugin to collect details of tests
This comment is no longer seems accurate. This plugin really just keeps a 
record of the tests that were collected, and the tests that were run. I don't 
think we gather any other details than those.


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@66
PS11, Line 66:   tests_collected is a collection of items
I don't think this is right.  The elements of either list are pytest "nodeids", 
which is just a unique string identifier for each test that takes the form of:

 []

E.g., from a typical test run for Impala, an example nodeid would be something 
like:

  "query_test.test_insert.TestInsertQueries.test_insert[exec_option: 
{'sync_ddl': 0, 'batch_size': 0, 'num_nodes': 0, 
'disable_codegen_rows_threshold': 0, 'disable_codegen': False, 
'abort_on_error': 1, 'exec_single_node_rows_threshold': 0} | table_format: 
text/none]"


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@73
PS11, Line 73:   
https://docs.pytest.org/en/2.9.2/writing_plugins.html#_pytest.hookspec.pytest_collection_modifyitems
This link is really specific to pytest_collection_modifyitems hook, not 
pytest_runtest_logreport. Can you move this to L81?


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@78
PS11, Line 78: self.tests_executed = []
In a standard test run, one would expect that tests_collected and 
tests_executed would be same at the end. However, pytest reporting happens in 
multiple stages (in other words, pytest_runtest_logreport gets called multiple 
times), and you'll find that tests_executed winds up being exactly three times 
as long as tests_collected in your patch, because report.passed gets eval'ed 
three times.

I did a quick test with some perf tests, and printed out tests_executed to show 
the multiple entries:

  [
'test_impala_perf.py::test_cluster_workload[tpch:_300]',
'test_impala_perf.py::test_cluster_workload[tpch:_300]',
'test_impala_perf.py::test_cluster_workload[tpch:_300]',
'test_impala_perf.py::test_cluster_workload[tpcds-unmodified_kudu:_1000]',
'test_impala_perf.py::test_cluster_workload[tpcds-unmodified_kudu:_1000]',
'test_impala_perf.py::test_cluster_workload[tpcds-unmodified_kudu:_1000]',
'test_impala_perf.py::test_cluster_workload[tpch_kudu:_300]',
'test_impala_perf.py::test_cluster_workload[tpch_kudu:_300]',
'test_impala_perf.py::test_cluster_workload[tpch_kudu:_300]
  ]

The quick way around this would be to make both of these sets(), rather than 
lists.


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@85
PS11, Line 85: if report.passed:
Add the link here that is specific to pytest_runtest_logreport:

https://docs.pytest.org/en/2.9.2/_modules/_pytest/hookspec.html#pytest_runtest_logreport


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@97
PS11, Line 97:   exit_code = pytest.main(args, plugins=[testcounterplugin])
I think I asked earlier to have some distinction drawn between exit_code 
variables. E.g., this should be pytest_exit_code, and then later, when we 
determine how to exit this script, we should use runtest_exit_code. There have 
been a lot of comments though, so maybe it just got overlooked. (You might need 
to click show all messages at the top of the messages section.)


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@107
PS11, Line 107: #though 5 is a normal return code, our jenkins handles 
non-zero RC as error in
This is odd wording in this context. Don't forget that this code goes into the 
upstream Apache repo, where "our Jenkins" has no real significance. I would 
reword this comment to be more general, like:

  # EXIT_NOTESTSCOLLECTED is a pytest return code meaning "no tests collected" 
and is equal to 5.
  # This would typically be an error, but not necessarily in the case of this 
script, because it executes
  # pytest multiple times, and we may selectively choose not to run some of the 
tests for one or two of
  # those executions.


http://gerrit.cloudera.org:8080/#/c/9494/11/tests/run-tests.py@112
PS11, Line 112: len(testcounterplugin.tests_executed) <= 0
I think this comment was missed from before as well. I don't thinks 
len(tests_executed) can ever 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 11:

https://jenkins.impala.io/job/gerrit-verify-dryrun-external/90/console - 
gerrit-dry-run was successful


--
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: 11
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Fri, 09 Mar 2018 22:13:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#11). ( 
http://gerrit.cloudera.org:8080/9494 )

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

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

exit_code for EE tests when no tests are collected.After this
change return_code will be either 0 if no tests are expected
to be collected (dry-run) and 1 if tests are expected to be
collected but are not collected due to some error

Testing:
 - Ran end-to-end shell, hs2 tests for IMPALA-5886 with debug
   statements to verify the exit_codes
 - Ran end-to-end shell tests with collect-only for IMPALA-4812

Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
---
M tests/run-tests.py
1 file changed, 66 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/9494/11
--
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: newpatchset
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 11
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

(1 comment)

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@100
PS4, Line 100: if exit_code == 5:
> I wonder if this would work?
Done



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Thu, 08 Mar 2018 00:50:39 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

(1 comment)

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@244
PS4, Line 244: print_metrics('connections')
> Please make sure to maintain consistent print_metrics() usage. I think you
Done



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Thu, 08 Mar 2018 00:41:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/9494 )

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

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

exit_code for EE tests when no tests are collected.After this
change return_code will be either 0 if no tests are expected
to be collected (dry-run) and 1 if tests are expected to be
collected but are not collected due to some error

Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
---
M tests/run-tests.py
1 file changed, 65 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/9494/8
--
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: newpatchset
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 8
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

(1 comment)

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
> Again...handled in the last patch
Done



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Wed, 07 Mar 2018 23:21:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

(8 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
> Agreed. And also, please explain testing done.
It has been handled in the latest patch


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?
Again...handled in the last patch


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.
Done.

I would prefer to keep the name as TestStatistics , so  any functions related 
to pytest data can be added here


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@65
PS4, Line 65: class TestStatisticsPlugin:
> I see what you are saying. But the other classes (Class Testexecutor) in ru
Done


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.
Done


http://gerrit.cloudera.org:8080/#/c/9494/4/tests/run-tests.py@71
PS4, Line 71: # items represents the list of collected test items
> Agreed.
Done


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 sig
Done


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
I have kept this to allow suppressing of pytest verbose messages and just see 
the list of collected tests by

run-tests.py shell --collect-only -p no:terminal



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Wed, 07 Mar 2018 23:20:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Michael Brown (Code Review)]

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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Wed, 07 Mar 2018 22:58:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#1). ( 
http://gerrit.cloudera.org:8080/9494 )

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

IMPALA-5886 Update run-tests.py script to handle exit_code for EE tests when no 
tests are collected
After this change return_code will be either 0 if no tests are expected to be 
collected (dry-run) and 1 if tests are expected to be collected but are not 
collected due
to come error

Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
---
M tests/run-tests.py
1 file changed, 19 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/9494/1
--
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: newpatchset
Gerrit-Change-Id: If82f974cc2d1e917464d4053563eaf4afc559150
Gerrit-Change-Number: 9494
Gerrit-PatchSet: 1
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5886 & IMPALA-4812 Update run-tests.py to handle exit code 5

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

(1 comment)

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:
Done



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Wed, 07 Mar 2018 22:50:03 +
Gerrit-HasComments: Yes