adelapena commented on a change in pull request #152:
URL: https://github.com/apache/cassandra-dtest/pull/152#discussion_r702037038
##########
File path: conftest.py
##########
@@ -545,53 +544,46 @@ def cassandra_dir_and_version(config):
return cassandra_dir, cassandra_version
-def has_mark(item, mark):
+def _is_skippable(item, mark, skip_marked, skip_non_marked):
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
- else:
- logger.info("SKIP: Skipping %s because it is marked with %s" %
(item, mark))
+ if skip_marked:
+ 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 False
+ else:
+ 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(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
+class SkipConditions:
Review comment:
If we are going to pack these vars into a new class perhaps we could
also place `_is_skippable` and `is_skippable` inside the class, for example
[this
way](https://github.com/adelapena/cassandra-dtest/commit/bb3b4382a59c453ccfcd8b85c5cbc70afe8b5cee).
##########
File path: conftest.py
##########
@@ -545,53 +544,46 @@ def cassandra_dir_and_version(config):
return cassandra_dir, cassandra_version
-def has_mark(item, mark):
+def _is_skippable(item, mark, skip_marked, skip_non_marked):
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
- else:
- logger.info("SKIP: Skipping %s because it is marked with %s" %
(item, mark))
+ if skip_marked:
+ 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 False
+ else:
+ 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(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
+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_tests = (
+ not dtest_config.force_execution_of_resource_intensive_tests
+ and not dtest_config.only_resource_intensive_tests
+ and not sufficient_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
- 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
+def is_skippable(item, skip_cond):
+ return (_is_skippable(item, "upgrade_test",
+ skip_cond.skip_upgrade_tests,
skip_cond.skip_non_upgrade_tests)
+ or _is_skippable(item, "resource_intensive",
+ skip_cond.skip_resource_intensive_tests,
skip_cond.skip_non_resource_intensive_tests)
+ or _is_skippable(item, "vnodes", skip_cond.skip_vnodes_tests,
skip_non_marked=False)
+ or _is_skippable(item, "no_vnodes",
skip_cond.skip_no_vnodes_tests, skip_non_marked=False)
+ or _is_skippable(item, "no_offheap_memtables",
skip_cond.skip_no_offheap_memtables_tests, skip_non_marked=False)
+ or _is_skippable(item, "depends_driver", skip_marked=True,
skip_non_marked=False))
Review comment:
Nit: maybe this block would be a bit easier to read if we always name
the `skip_marked`/`skip_non_marked` parameters:
```suggestion
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))
```
##########
File path: upgrade_tests/paging_test.py
##########
@@ -188,6 +188,7 @@ def random_txt(text):
assert_lists_equal_ignoring_order(pf.all_data(), expected_data,
sort_key='value')
[email protected]_test
Review comment:
Can we simply add the annotation to `BasePagingTester`, which is the
parent of all the marked tests, [this
way](https://github.com/adelapena/cassandra-dtest/commit/c3668b62654b0b5f11eccb9e2cfd74d1e164d990)?
##########
File path: conftest.py
##########
@@ -545,53 +544,46 @@ def cassandra_dir_and_version(config):
return cassandra_dir, cassandra_version
-def has_mark(item, mark):
+def _is_skippable(item, mark, skip_marked, skip_non_marked):
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
- else:
- logger.info("SKIP: Skipping %s because it is marked with %s" %
(item, mark))
+ if skip_marked:
+ logger.info(
+ "SKIP: Skipping %s because it is marked with %s" % (item,
mark))
Review comment:
Nit: I think we don't need to break this line nor the other
`loggger.info` call right below
```suggestion
logger.info("SKIP: Skipping %s because it is marked with %s" %
(item, mark))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
