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: <test dir>.<module>.<class>.<function>.[<list of params>] 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 be less than zero. :-) That said, I think we might need to revisit the logic of this block, and also the placement of it. I think there's a fundamental issue here, in that we can't really eval the exit code of run_tests.py inside one the "loop" (it's not really a loop -- we just call it multiple times.) In pseudocode, I think the logic needs to work something like: t = TestExecutor() for test_group in (serial, parallel, stress, custom_cluster): t.testcounterplugin.tests_collected.append(collected nodeids) t.run_tests(test_group) if pytest_exit_code == 0 or pytest_exit_code = EXIT_NOTESTSCOLLECTED: continue else: t.tests_failed = True if t._exit_on_error: break # Finally, after all tests runs have been completed, at the very end, # if there were failed tests, or if not no tests at all were collected, then # exit with a non-zero status if t.tests_failed == True or len(testcounterplugin.tests_collected) == 0 sys.exit(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: 11 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: Sat, 10 Mar 2018 05:18:15 +0000 Gerrit-HasComments: Yes
