Repository: incubator-impala Updated Branches: refs/heads/master 37ec25396 -> 067af1957
IMPALA-3614: work around pytest bugs causing custom cluster test skips All versions of pytest contain various bugs regarding test marking (including skips) when tests are both: 1. class-level marked 2. inherited More info is available in IMPALA-3614 and IMPALA-2943, but the gist is that it's possible for some tests to be skipped when they shouldn't be. This is happening pretty badly with the custom cluster tests, because CustomClusterTestSuite has a class level skipif mark. The easiest workaround for now is to remove the pytest skipif mark in CustomClusterTestSuite and skip using explicit pytest.skip() in the setup_class() method. Some CustomClusterTestSuite children implemented their own setup_* methods, and I made some adjustments to them both to clean them up and implement proper parent method calling via super(). Testing: I ran the following combinations of all the custom cluster tests: DEBUG / HDFS / core RELEASE / HDFS / exhaustive DEBUG / LOCAL / core DEBUG / S3 / core Before, we'd get situations in which most of the tests were skipped. Consider the RELEASE/HDFS/exhaustive situation: custom_cluster/test_admission_controller.py ..... custom_cluster/test_alloc_fail.py ss custom_cluster/test_breakpad.py sssss custom_cluster/test_delegation.py sss custom_cluster/test_exchange_delays.py ss custom_cluster/test_hdfs_fd_caching.py s custom_cluster/test_hive_parquet_timestamp_conversion.py ss custom_cluster/test_insert_behaviour.py ss custom_cluster/test_legacy_joins_aggs.py s custom_cluster/test_parquet_max_page_header.py s custom_cluster/test_permanent_udfs.py sss custom_cluster/test_query_expiration.py sss custom_cluster/test_redaction.py ssss custom_cluster/test_s3a_access.py s custom_cluster/test_scratch_disk.py ssss custom_cluster/test_session_expiration.py s custom_cluster/test_spilling.py ssss authorization/test_authorization.py ss authorization/test_grant_revoke.py s Now, more tests run appropriately: custom_cluster/test_admission_controller.py ..... custom_cluster/test_alloc_fail.py ss custom_cluster/test_breakpad.py sssss custom_cluster/test_delegation.py ... custom_cluster/test_exchange_delays.py ss custom_cluster/test_hdfs_fd_caching.py . custom_cluster/test_hive_parquet_timestamp_conversion.py .. custom_cluster/test_insert_behaviour.py .. custom_cluster/test_kudu_not_available.py . custom_cluster/test_legacy_joins_aggs.py . custom_cluster/test_parquet_max_page_header.py . custom_cluster/test_permanent_udfs.py ... custom_cluster/test_query_expiration.py ... custom_cluster/test_redaction.py .... custom_cluster/test_s3a_access.py s custom_cluster/test_scratch_disk.py .... custom_cluster/test_session_expiration.py . custom_cluster/test_spilling.py .... authorization/test_authorization.py .. authorization/test_grant_revoke.py . Change-Id: Ie301b69718f8690322cc3b4130fb1c715344779c Reviewed-on: http://gerrit.cloudera.org:8080/3265 Reviewed-by: Michael Brown <[email protected]> Tested-by: Michael Brown <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/067af195 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/067af195 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/067af195 Branch: refs/heads/master Commit: 067af1957c3c58c54adacaa267d3de319a98a0c3 Parents: ee53ddb Author: Michael Brown <[email protected]> Authored: Tue May 24 16:55:37 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Mon Jun 6 17:34:07 2016 -0700 ---------------------------------------------------------------------- tests/common/custom_cluster_test_suite.py | 17 ++++++++++---- tests/custom_cluster/test_insert_behaviour.py | 1 + tests/custom_cluster/test_legacy_joins_aggs.py | 24 ++++--------------- tests/custom_cluster/test_s3a_access.py | 5 ++-- tests/custom_cluster/test_scratch_disk.py | 1 + tests/custom_cluster/test_spilling.py | 26 ++++++++++----------- 6 files changed, 34 insertions(+), 40 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/067af195/tests/common/custom_cluster_test_suite.py ---------------------------------------------------------------------- diff --git a/tests/common/custom_cluster_test_suite.py b/tests/common/custom_cluster_test_suite.py index 9faafa5..c5822e4 100644 --- a/tests/common/custom_cluster_test_suite.py +++ b/tests/common/custom_cluster_test_suite.py @@ -17,11 +17,13 @@ import os import os.path +import pytest import re from subprocess import check_call from tests.common.impala_test_suite import ImpalaTestSuite from tests.common.impala_cluster import ImpalaCluster from tests.common.skip import SkipIfLocal +from tests.util.filesystem_utils import IS_LOCAL from time import sleep IMPALA_HOME = os.environ['IMPALA_HOME'] @@ -33,7 +35,6 @@ IMPALAD_ARGS = 'impalad_args' STATESTORED_ARGS = 'state_store_args' CATALOGD_ARGS = 'catalogd_args' [email protected]_impalad class CustomClusterTestSuite(ImpalaTestSuite): """Every test in a test suite deriving from this class gets its own Impala cluster. Custom arguments may be passed to the cluster by using the @with_args decorator.""" @@ -52,12 +53,20 @@ class CustomClusterTestSuite(ImpalaTestSuite): v.get_value('exec_option')['disable_codegen'] == False and v.get_value('exec_option')['num_nodes'] == 0) + @classmethod def setup_class(cls): - # No-op, but needed to override base class setup which is not wanted in this - # case (it is done on a per-method basis). - pass + # Explicit override of ImpalaTestSuite.setup_class(). For custom cluster, the + # ImpalaTestSuite.setup_class() procedure needs to happen on a per-method basis. + # IMPALA-3614: @SkipIfLocal.multiple_impalad workaround + # IMPALA-2943 TODO: When pytest is upgraded, see if this explicit skip can be + # removed in favor of the class-level SkipifLocal.multiple_impalad decorator. + if IS_LOCAL: + pytest.skip("multiple impalads needed") + @classmethod def teardown_class(cls): + # Explicit override of ImpalaTestSuite.teardown_class(). For custom cluster, the + # ImpalaTestSuite.teardown_class() procedure needs to happen on a per-method basis. pass @staticmethod http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/067af195/tests/custom_cluster/test_insert_behaviour.py ---------------------------------------------------------------------- diff --git a/tests/custom_cluster/test_insert_behaviour.py b/tests/custom_cluster/test_insert_behaviour.py index 73acb6c..0f92cda 100644 --- a/tests/custom_cluster/test_insert_behaviour.py +++ b/tests/custom_cluster/test_insert_behaviour.py @@ -26,6 +26,7 @@ class TestInsertBehaviourCustomCluster(CustomClusterTestSuite): @classmethod def setup_class(cls): + super(TestInsertBehaviourCustomCluster, cls).setup_class() if pytest.config.option.namenode_http_address is None: hdfs_conf = HdfsConfig(pytest.config.option.minicluster_xml_conf) cls.hdfs_client = get_hdfs_client_from_conf(hdfs_conf) http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/067af195/tests/custom_cluster/test_legacy_joins_aggs.py ---------------------------------------------------------------------- diff --git a/tests/custom_cluster/test_legacy_joins_aggs.py b/tests/custom_cluster/test_legacy_joins_aggs.py index 2c5a949..19c4cae 100644 --- a/tests/custom_cluster/test_legacy_joins_aggs.py +++ b/tests/custom_cluster/test_legacy_joins_aggs.py @@ -24,26 +24,10 @@ class TestLegacyJoinsAggs(CustomClusterTestSuite): def get_workload(self): return 'functional-query' - @classmethod - def setup_class(cls): - #start impala with args - cls._start_impala_cluster(['--impalad_args="--enable_partitioned_hash_join=false ' - '--enable_partitioned_aggregation=false"', - 'catalogd_args="--load_catalog_in_background=false"']) - super(CustomClusterTestSuite, cls).setup_class() - - @classmethod - def teardown_class(cls): - pass - - @classmethod - def setup_method(self, method): - pass - - @classmethod - def teardown_method(self, method): - pass - + @CustomClusterTestSuite.with_args( + impalad_args=('--enable_partitioned_hash_join=false ' + '--enable_partitioned_aggregation=false'), + catalogd_args='--load_catalog_in_background=false') def test_nested_types(self, vector): self.run_test_case('QueryTest/legacy-joins-aggs', vector, use_db='functional_parquet') http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/067af195/tests/custom_cluster/test_s3a_access.py ---------------------------------------------------------------------- diff --git a/tests/custom_cluster/test_s3a_access.py b/tests/custom_cluster/test_s3a_access.py index 974aafa..be27443 100644 --- a/tests/custom_cluster/test_s3a_access.py +++ b/tests/custom_cluster/test_s3a_access.py @@ -30,6 +30,7 @@ class TestS3AAccess(CustomClusterTestSuite): cmd_filename = "" @classmethod def setup_class(cls): + super(TestS3AAccess, cls).setup_class() try: tmp.write('echo badkey') finally: @@ -42,8 +43,8 @@ class TestS3AAccess(CustomClusterTestSuite): def teardown_class(cls): os.remove(BAD_KEY_FILE) - def teardown_method(cls, method): - cls._drop_test_tbl() + def teardown_method(self, method): + self._drop_test_tbl() def _drop_test_tbl(self): client = self._get_impala_client() http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/067af195/tests/custom_cluster/test_scratch_disk.py ---------------------------------------------------------------------- diff --git a/tests/custom_cluster/test_scratch_disk.py b/tests/custom_cluster/test_scratch_disk.py index aeabf69..611bb89 100644 --- a/tests/custom_cluster/test_scratch_disk.py +++ b/tests/custom_cluster/test_scratch_disk.py @@ -54,6 +54,7 @@ class TestScratchDir(CustomClusterTestSuite): @classmethod def setup_class(cls): + super(TestScratchDir, cls).setup_class() cls.normal_dirs = cls.generate_dirs(5) cls.non_writable_dirs = cls.generate_dirs(5, writable=False) cls.non_existing_dirs = cls.generate_dirs(5, non_existing=True) http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/067af195/tests/custom_cluster/test_spilling.py ---------------------------------------------------------------------- diff --git a/tests/custom_cluster/test_spilling.py b/tests/custom_cluster/test_spilling.py index 541352f..2256f0b 100644 --- a/tests/custom_cluster/test_spilling.py +++ b/tests/custom_cluster/test_spilling.py @@ -27,20 +27,23 @@ class TestSpillStress(CustomClusterTestSuite): @classmethod def setup_class(cls): - # Start with 256KB buffers, to reduce data size required to force spilling. + super(TestSpillStress, cls).setup_class() + if cls.exploration_strategy() != 'exhaustive': + pytest.skip('runs only in exhaustive') + # Since test_spill_stress below runs TEST_IDS * NUM_ITERATIONS times, but we only + # need Impala to start once, it's inefficient to use + # @CustomClusterTestSuite.with_args() to restart Impala every time. Instead, start + # Impala here, once. + # + # Start with 256KB buffers to reduce data size required to force spilling. cls._start_impala_cluster(['--impalad_args=--"read_size=262144"', 'catalogd_args="--load_catalog_in_background=false"']) - super(CustomClusterTestSuite, cls).setup_class() - @classmethod - def teardown_class(cls): - pass - - @classmethod def setup_method(self, method): + # We don't need CustomClusterTestSuite.setup_method() or teardown_method() here + # since we're not doing per-test method restart of Impala. pass - @classmethod def teardown_method(self, method): pass @@ -49,16 +52,11 @@ class TestSpillStress(CustomClusterTestSuite): super(TestSpillStress, cls).add_test_dimensions() cls.TestMatrix.add_constraint(lambda v:\ v.get_value('table_format').file_format == 'text') - # Each client will get a different test id. # TODO: this test takes extremely long so only run on exhaustive. It would # be good to configure it so we can run some version on core. TEST_IDS = xrange(0, 3) - NUM_ITERATIONS = [0] - if cls.exploration_strategy() == 'exhaustive': - TEST_IDS = xrange(0, 3) - NUM_ITERATIONS = [1] - + NUM_ITERATIONS = [1] cls.TestMatrix.add_dimension(TestDimension('test_id', *TEST_IDS)) cls.TestMatrix.add_dimension(TestDimension('iterations', *NUM_ITERATIONS))
