Fix unexpectedly ignored and repeated tests

patch by Ruslan Fomkin; reviewed by Andres de la Peña and Ekaterina Dimitrova for CASSANDRA-16841

.travis.yml

@@ -16,7 +16,8 @@ script:
   # I know we don't enforce line length but if you introduce
   # 200-char lines you are doing something terribly wrong.
   # lint all files for everything but line length errors
-  - git diff apache/trunk...HEAD -U0 | pycodestyle --ignore=E501 --diff
+  # and W503, since it is inconflict with W504
+  - git diff apache/trunk...HEAD -U0 | pycodestyle --ignore=E501,W503 --diff
   # lint all files except json_test.py for line length errors
   - git diff apache/trunk...HEAD -U0 | pycodestyle --diff --exclude='json_test.py' --exclude='meta_tests/assertion_test.py' --max-line-length=200
   - pytest --metatests

CONTRIBUTING.md

@@ -14,7 +14,7 @@ Another way to make sure that your code will pass compliance checks is to run **
 ```
 flake8 --install-hook
 git config flake8.strict true
-git config flake8.ignore E501,F811,F812,F821,F822,F823,F831,F841,N8,C9
+git config flake8.ignore E501,F811,F812,F821,F822,F823,F831,F841,N8,C9,W503
 ```
 
 We do not enforce **import** sorting, but if you choose to organize imports by some convention, use the `isort` tool (`pip install isort`).

bootstrap_test.py

@@ -28,7 +28,8 @@
 ported_to_in_jvm = pytest.mark.ported_to_in_jvm
 logger = logging.getLogger(__name__)
 
-class TestBootstrap(Tester):
+
+class BootstrapTester(Tester):
     byteman_submit_path_pre_4_0 = './byteman/pre4.0/stream_failure.btm'
     byteman_submit_path_4_0 = './byteman/4.0/stream_failure.btm'
 
@@ -1097,3 +1098,13 @@ def test_host_id_override(self):
         # 3. check host_id in other node's table
         session1 = self.patient_exclusive_cql_connection(node1)
         assert_one(session1, "SELECT host_id FROM system.peers_v2 WHERE peer = {}".format(address2), [uuid.UUID(host_id)])
+
+
+class TestBootstrap(BootstrapTester):
+    """
+    This child class is a helper for PyTest to pick up the test methods.
+    Since the same test methods are executed as part of upgrade, this helper child class is
+    necessary to avoid double execution of the same tests.
+    It is necessary to put some dummy expression in this child class.
+    """
+    pass

conftest.py

@@ -1,6 +1,5 @@
 import copy
 import collections
-import inspect
 import logging
 import os
 import platform
@@ -545,53 +544,55 @@ def cassandra_dir_and_version(config):
     return cassandra_dir, cassandra_version
 
 
-def has_mark(item, mark):
-    if item.get_closest_marker(mark) is not None:
-        return True
-    else:
-        for item_module in inspect.getmembers(item.module, inspect.isclass):
-            if hasattr(item_module[1], "pytestmark"):
-                mark_names = [m.name for m in item_module[1].pytestmark]
-                if mark in mark_names:
-                    return True
-
-        return False
-
-
-def _is_skippable(item, mark, include_marked, include_other):
-    if has_mark(item, mark):
-        if include_marked:
-            return False
+class SkipConditions:
+    def __init__(self, dtest_config, sufficient_resources):
+        self.skip_upgrade_tests = not dtest_config.execute_upgrade_tests and not dtest_config.execute_upgrade_tests_only
+        self.skip_non_upgrade_tests = dtest_config.execute_upgrade_tests_only
+        self.skip_resource_intensive_due_to_resources = (
+            not dtest_config.force_execution_of_resource_intensive_tests
+            and not sufficient_resources)
+        self.skip_resource_intensive_tests = (
+            self.skip_resource_intensive_due_to_resources
+            or dtest_config.skip_resource_intensive_tests)
+        self.skip_non_resource_intensive_tests = dtest_config.only_resource_intensive_tests
+        self.skip_vnodes_tests = not dtest_config.use_vnodes
+        self.skip_no_vnodes_tests = dtest_config.use_vnodes
+        self.skip_no_offheap_memtables_tests = dtest_config.use_off_heap_memtables
+
+    @staticmethod
+    def _is_skippable(item, mark, skip_marked, skip_non_marked):
+        if item.get_closest_marker(mark) is not None:
+            if skip_marked:
+                logger.info("SKIP: Skipping %s because it is marked with %s" % (item, mark))
+                return True
+            else:
+                return False
         else:
-            logger.info("SKIP: Skipping %s because it is marked with %s" % (item, mark))
-            return True
-    else:
-        if include_other:
-            return False
-        else:
-            logger.info("SKIP: Skipping %s because it is not marked with %s" % (item, mark))
-            return True
-
-
-def is_skippable(item,
-                 include_upgrade_tests,
-                 include_non_upgrade_tests,
-                 include_resource_intensive_tests,
-                 include_non_resource_intensive_tests,
-                 include_vnodes_tests,
-                 include_no_vnodes_tests,
-                 include_no_offheap_memtables_tests):
-
-    skippable = False
-
-    skippable = skippable or _is_skippable(item, "upgrade_test", include_upgrade_tests, include_non_upgrade_tests)
-    skippable = skippable or _is_skippable(item, "resource_intensive", include_resource_intensive_tests, include_non_resource_intensive_tests)
-    skippable = skippable or _is_skippable(item, "vnodes", include_vnodes_tests, True)
-    skippable = skippable or _is_skippable(item, "no_vnodes", include_no_vnodes_tests, True)
-    skippable = skippable or _is_skippable(item, "no_offheap_memtables", include_no_offheap_memtables_tests, True)
-    skippable = skippable or _is_skippable(item, "depends_driver", False, True)
-
-    return skippable
+            if skip_non_marked:
+                logger.info("SKIP: Skipping %s because it is not marked with %s" % (item, mark))
+                return True
+            else:
+                return False
+
+    def is_skippable(self, item):
+        return (self._is_skippable(item, "upgrade_test",
+                                   skip_marked=self.skip_upgrade_tests,
+                                   skip_non_marked=self.skip_non_upgrade_tests)
+                or self._is_skippable(item, "resource_intensive",
+                                      skip_marked=self.skip_resource_intensive_tests,
+                                      skip_non_marked=self.skip_non_resource_intensive_tests)
+                or self._is_skippable(item, "vnodes",
+                                      skip_marked=self.skip_vnodes_tests,
+                                      skip_non_marked=False)
+                or self._is_skippable(item, "no_vnodes",
+                                      skip_marked=self.skip_no_vnodes_tests,
+                                      skip_non_marked=False)
+                or self._is_skippable(item, "no_offheap_memtables",
+                                      skip_marked=self.skip_no_offheap_memtables_tests,
+                                      skip_non_marked=False)
+                or self._is_skippable(item, "depends_driver",
+                                      skip_marked=True,
+                                      skip_non_marked=False))
 
 
 def pytest_collection_modifyitems(items, config):
@@ -605,28 +606,16 @@ def pytest_collection_modifyitems(items, config):
     selected_items = []
     deselected_items = []
 
-    can_run_resource_intensive_tests = dtest_config.force_execution_of_resource_intensive_tests or sufficient_system_resources_for_resource_intensive_tests()
-    if not can_run_resource_intensive_tests:
-        logger.info("Resource intensive tests will be skipped because there is not enough system resource "
-                    "and --force-resource-intensive-tests was not specified")
+    sufficient_resources = sufficient_system_resources_for_resource_intensive_tests()
+    skip_conditions = SkipConditions(dtest_config, sufficient_resources)
 
-    include_upgrade_tests = dtest_config.execute_upgrade_tests or dtest_config.execute_upgrade_tests_only
-    include_non_upgrade_tests = not dtest_config.execute_upgrade_tests_only
-    include_resource_intensive_tests = can_run_resource_intensive_tests and not dtest_config.skip_resource_intensive_tests
-    include_non_resource_intensive_tests = not dtest_config.only_resource_intensive_tests
-    include_vnodes_tests = dtest_config.use_vnodes
-    include_no_vnodes_tests = not dtest_config.use_vnodes
-    include_no_offheap_memtables_tests = not dtest_config.use_off_heap_memtables
+    if skip_conditions.skip_resource_intensive_due_to_resources:
+        logger.info("Resource intensive tests will be skipped because "
+                    "there is not enough system resources "
+                    "and --force-resource-intensive-tests was not specified")
 
     for item in items:
-        deselect_test = is_skippable(item,
-                                     include_upgrade_tests,
-                                     include_non_upgrade_tests,
-                                     include_resource_intensive_tests,
-                                     include_non_resource_intensive_tests,
-                                     include_vnodes_tests,
-                                     include_no_vnodes_tests,
-                                     include_no_offheap_memtables_tests)
+        deselect_test = SkipConditions.is_skippable(skip_conditions, item)
 
         if deselect_test:
             deselected_items.append(item)

meta_tests/conftest_test.py

@@ -1,32 +1,29 @@
-from unittest import TestCase
+import pytest
 
-from conftest import is_skippable
+from conftest import SkipConditions
 from mock import Mock
 
 
+class DTestConfigMock():
+    def __init__(self):
+        self.execute_upgrade_tests = False
+        self.execute_upgrade_tests_only = False
+        self.force_execution_of_resource_intensive_tests = False
+        self.only_resource_intensive_tests = False
+        self.skip_resource_intensive_tests = False
+        self.use_vnodes = False
+        self.use_off_heap_memtables = False
+
+    def set(self, config):
+        if config != "":
+            setattr(self, config, True)
+
+
 def _mock_responses(responses, default_response=None):
     return lambda arg: responses[arg] if arg in responses else default_response
 
 
-def _is_skippable(item,
-                  include_upgrade_tests=True,
-                  include_non_upgrade_tests=True,
-                  include_resource_intensive_tests=True,
-                  include_non_resource_intensive_tests=True,
-                  include_vnodes_tests=True,
-                  include_no_vnodes_tests=True,
-                  include_no_offheap_memtables_tests=True):
-    return is_skippable(item,
-                        include_upgrade_tests,
-                        include_non_upgrade_tests,
-                        include_resource_intensive_tests,
-                        include_non_resource_intensive_tests,
-                        include_vnodes_tests,
-                        include_no_vnodes_tests,
-                        include_no_offheap_memtables_tests)
-
-
-class ConfTestTest(TestCase):
+class TestConfTest(object):
     regular_test = Mock(name="regular_test_mock")
     upgrade_test = Mock(name="upgrade_test_mock")
     resource_intensive_test = Mock(name="resource_intensive_test_mock")
@@ -35,39 +32,106 @@ class ConfTestTest(TestCase):
     no_offheap_memtables_test = Mock(name="no_offheap_memtables_test_mock")
     depends_driver_test = Mock(name="depends_driver_test_mock")
 
-    def setup_method(self, method):
+    def setup_method(self):
         self.regular_test.get_closest_marker.side_effect = _mock_responses({})
-        self.upgrade_test.get_closest_marker.side_effect = _mock_responses({"upgrade_test": True})
-        self.resource_intensive_test.get_closest_marker.side_effect = _mock_responses({"resource_intensive": True})
-        self.vnodes_test.get_closest_marker.side_effect = _mock_responses({"vnodes": True})
-        self.no_vnodes_test.get_closest_marker.side_effect = _mock_responses({"no_vnodes": True})
-        self.no_offheap_memtables_test.get_closest_marker.side_effect = _mock_responses({"no_offheap_memtables": True})
-        self.depends_driver_test.get_closest_marker.side_effect = _mock_responses({"depends_driver": True})
-
-    def test_regular_test(self):
-        assert not _is_skippable(item=self.regular_test)
-        assert _is_skippable(item=self.regular_test, include_non_upgrade_tests=False)
-        assert _is_skippable(item=self.regular_test, include_non_resource_intensive_tests=False)
-
-    def test_upgrade_test(self):
-        assert not _is_skippable(item=self.upgrade_test)
-        assert _is_skippable(item=self.upgrade_test, include_upgrade_tests=False)
-
-    def test_resource_intensive_test(self):
-        assert not _is_skippable(item=self.resource_intensive_test)
-        assert _is_skippable(item=self.resource_intensive_test, include_resource_intensive_tests=False)
-
-    def test_vnodes_test(self):
-        assert not _is_skippable(item=self.vnodes_test)
-        assert _is_skippable(item=self.vnodes_test, include_vnodes_tests=False)
-
-    def test_no_vnodes_test(self):
-        assert not _is_skippable(item=self.no_vnodes_test)
-        assert _is_skippable(item=self.no_vnodes_test, include_no_vnodes_tests=False)
-
-    def test_no_offheap_memtables_test(self):
-        assert not _is_skippable(item=self.no_offheap_memtables_test)
-        assert _is_skippable(item=self.no_offheap_memtables_test, include_no_offheap_memtables_tests=False)
-
-    def test_depends_driver_test(self):
-        assert _is_skippable(item=self.depends_driver_test)
+        self.upgrade_test.get_closest_marker.side_effect = _mock_responses(
+            {"upgrade_test": True})
+        self.resource_intensive_test.get_closest_marker.side_effect = _mock_responses(
+            {"resource_intensive": True})
+        self.vnodes_test.get_closest_marker.side_effect = _mock_responses(
+            {"vnodes": True})
+        self.no_vnodes_test.get_closest_marker.side_effect = _mock_responses(
+            {"no_vnodes": True})
+        self.no_offheap_memtables_test.get_closest_marker.side_effect = _mock_responses(
+            {"no_offheap_memtables": True})
+        self.depends_driver_test.get_closest_marker.side_effect = _mock_responses(
+            {"depends_driver": True})
+
+    @pytest.mark.parametrize("item", [upgrade_test, resource_intensive_test, vnodes_test,
+                                      depends_driver_test])
+    def test_skip_if_no_config(self, item):
+        dtest_config = DTestConfigMock()
+        assert SkipConditions(dtest_config, False).is_skippable(item)
+
+    @pytest.mark.parametrize("item", [regular_test, resource_intensive_test, no_vnodes_test,
+                                      no_offheap_memtables_test])
+    def test_include_if_no_config(self, item):
+        dtest_config = DTestConfigMock()
+        assert not SkipConditions(dtest_config, True).is_skippable(item)
+
+    @pytest.mark.parametrize("item,config",
+                             [(upgrade_test, "execute_upgrade_tests_only"),
+                              (resource_intensive_test, "only_resource_intensive_tests")])
+    def test_include_if_config_only(self, item, config):
+        dtest_config = DTestConfigMock()
+        dtest_config.set(config)
+        assert not SkipConditions(dtest_config, True).is_skippable(item)
+
+    @pytest.mark.parametrize("item",
+                             [regular_test, upgrade_test, resource_intensive_test, vnodes_test,
+                              no_vnodes_test, no_offheap_memtables_test])
+    @pytest.mark.parametrize("only_item,config",
+                             [(upgrade_test, "execute_upgrade_tests_only"),
+                              (resource_intensive_test, "only_resource_intensive_tests")])
+    def test_config_only(self, item, only_item, config):
+        dtest_config = DTestConfigMock()
+        dtest_config.set(config)
+        skip_conditions = SkipConditions(dtest_config, True)
+        if item != only_item:
+            assert skip_conditions.is_skippable(item)
+        else:
+            assert not skip_conditions.is_skippable(item)
+
+    @pytest.mark.parametrize("item",
+                             [regular_test, upgrade_test, resource_intensive_test,
+                              no_vnodes_test, no_offheap_memtables_test])
+    def test_include_if_execute_upgrade(self, item):
+        dtest_config = DTestConfigMock()
+        dtest_config.set("execute_upgrade_tests")
+        assert not SkipConditions(dtest_config, True).is_skippable(item)
+
+    @pytest.mark.parametrize("config, sufficient_resources",
+                             [("", False),
+                              ("skip_resource_intensive_tests", True),
+                              ("skip_resource_intensive_tests", False)])
+    def test_skip_resource_intensive(self, config, sufficient_resources):
+        dtest_config = DTestConfigMock()
+        dtest_config.set(config)
+        assert SkipConditions(dtest_config, sufficient_resources).is_skippable(self.resource_intensive_test)
+
+    @pytest.mark.parametrize("sufficient_resources", [True, False])
+    def test_include_resource_intensive_if_any_resources(self, sufficient_resources):
+        dtest_config = DTestConfigMock()
+        dtest_config.set("force_execution_of_resource_intensive_tests")
+        assert not SkipConditions(dtest_config, sufficient_resources).is_skippable(self.resource_intensive_test)
+
+    def test_skip_resource_intensive_wins(self):
+        dtest_config = DTestConfigMock()
+        dtest_config.set("force_execution_of_resource_intensive_tests")
+        dtest_config.set("only_resource_intensive_tests")
+        dtest_config.set("skip_resource_intensive_tests")
+        assert SkipConditions(dtest_config, True).is_skippable(self.resource_intensive_test)
+
+    @pytest.mark.parametrize("item",
+                             [regular_test, resource_intensive_test, vnodes_test,
+                              no_offheap_memtables_test])
+    def test_if_config_vnodes(self, item):
+        dtest_config = DTestConfigMock()
+        dtest_config.set("use_vnodes")
+        assert not SkipConditions(dtest_config, True).is_skippable(item)
+
+    def test_skip_no_offheap_memtables(self):
+        dtest_config = DTestConfigMock()
+        dtest_config.set("use_off_heap_memtables")
+        assert SkipConditions(dtest_config, True).is_skippable(self.no_offheap_memtables_test)
+
+    @pytest.mark.parametrize("config", ["", "execute_upgrade_tests", "execute_upgrade_tests_only",
+                                        "force_execution_of_resource_intensive_tests",
+                                        "only_resource_intensive_tests",
+                                        "skip_resource_intensive_tests", "use_vnodes",
+                                        "use_off_heap_memtables"])
+    @pytest.mark.parametrize("sufficient_resources", [True, False])
+    def test_skip_depends_driver_always(self, config, sufficient_resources):
+        dtest_config = DTestConfigMock()
+        dtest_config.set(config)
+        assert SkipConditions(dtest_config, sufficient_resources).is_skippable(self.depends_driver_test)

sstable_generation_loading_test.py

@@ -22,7 +22,7 @@
 
 # Also used by upgrade_tests/storage_engine_upgrade_test
 # to test loading legacy sstables
-class TestBaseSStableLoader(Tester):
+class BaseSStableLoaderTester(Tester):
 
     @pytest.fixture(autouse=True)
     def fixture_add_additional_log_patterns(self, fixture_dtest_setup):
@@ -250,7 +250,7 @@ def read_and_validate_data(session):
                     assert 0 == len(temp_files), "Temporary files were not cleaned up."
 
 
-class TestSSTableGenerationAndLoading(TestBaseSStableLoader):
+class TestSSTableGenerationAndLoading(BaseSStableLoaderTester):
 
     def test_sstableloader_uppercase_keyspace_name(self):
         """