tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r563147576
##########
File path: conftest.py
##########
@@ -597,58 +588,59 @@ def pytest_collection_modifyitems(items, config):
for item in items:
deselect_test = False
- if config.getoption("--execute-upgrade-tests-only"):
- deselect_test = not item.get_closest_marker("upgrade_test")
- if deselect_test:
+ is_upgrade_test = _is_upgrade_test(item)
+ is_resource_intensive_test =
item.get_closest_marker("resource_intensive") is not None
+ is_vnodes_test = item.get_closest_marker("vnodes") is not None
+ is_no_vnodes_test = item.get_closest_marker("no_vnodes") is not None
+ is_no_offheap_memtable_test =
item.get_closest_marker("no_offheap_memtables") is not None
+ is_depends_driver_test = item.get_closest_marker("depends_driver") is
not None
+
+ if is_upgrade_test:
+ if not dtest_config.execute_upgrade_tests and not
dtest_config.execute_upgrade_tests_only:
+ deselect_test = True
+ logger.info("SKIP: Deselecting upgrade test %s because neither
--execute-upgrade-tests or "
+ "--execute-upgrade-tests-only was specified" %
item.name)
+ else:
+ if dtest_config.execute_upgrade_tests_only:
+ deselect_test = True
logger.info("SKIP: Deselecting non-upgrade test %s because of
--execute-upgrade-tests-only" % item.name)
- if item.get_closest_marker("resource_intensive") and not collect_only:
- force_resource_intensive =
config.getoption("--force-resource-intensive-tests")
- skip_resource_intensive =
config.getoption("--skip-resource-intensive-tests")
- if not force_resource_intensive:
- if skip_resource_intensive:
- deselect_test = True
- logger.info("SKIP: Deselecting test %s as test marked
resource_intensive. To force execution of "
- "this test re-run with the
--force-resource-intensive-tests command line argument" % item.name)
- if not sufficient_system_resources_resource_intensive:
+ if is_resource_intensive_test:
+ if dtest_config.skip_resource_intensive_tests:
+ deselect_test = True
+ logger.info("SKIP: Deselecting test %s as test marked
resource_intensive and "
+ "--skip-resource-intensive-tests was specified." %
item.name)
+ elif not sufficient_system_resources_resource_intensive:
+ if not
dtest_config.force_execution_of_resource_intensive_tests:
deselect_test = True
- logger.info("SKIP: Deselecting resource_intensive test %s
due to insufficient system resources" % item.name)
-
- if not item.get_closest_marker("resource_intensive") and not
collect_only:
- only_resource_intensive =
config.getoption("--only-resource-intensive-tests")
- if only_resource_intensive:
+ logger.info("SKIP: Deselecting resource_intensive test %s
due to insufficient system resources. "
+ "Specify --force-resource-intensive-tests if
you really want to run it" % item.name)
+ else:
Review comment:
nit: I am not a big fun of nested ifs and elseses...
maybe instead it should be a sequence of rules.
if rules i matched -> skip.
even if repeating conditions.
```
if dtest_config.only_resource_intensive and not is_resource_intentive:
skip
if dtest_config.use_vnodes and is_no_vnodes_test:
skip with log
if not dtest_config.use_vnodes and is_vnodes_test:
skip with log
(...)
```
##########
File path: conftest.py
##########
@@ -597,58 +588,59 @@ def pytest_collection_modifyitems(items, config):
for item in items:
deselect_test = False
- if config.getoption("--execute-upgrade-tests-only"):
- deselect_test = not item.get_closest_marker("upgrade_test")
- if deselect_test:
+ is_upgrade_test = _is_upgrade_test(item)
+ is_resource_intensive_test =
item.get_closest_marker("resource_intensive") is not None
+ is_vnodes_test = item.get_closest_marker("vnodes") is not None
+ is_no_vnodes_test = item.get_closest_marker("no_vnodes") is not None
+ is_no_offheap_memtable_test =
item.get_closest_marker("no_offheap_memtables") is not None
+ is_depends_driver_test = item.get_closest_marker("depends_driver") is
not None
+
+ if is_upgrade_test:
+ if not dtest_config.execute_upgrade_tests and not
dtest_config.execute_upgrade_tests_only:
+ deselect_test = True
Review comment:
nit: I do not see why we keep `deselect_test` and `deseleted` instead of
`skip_test` and `skipped"... of course also in messaging.
##########
File path: conftest.py
##########
@@ -597,58 +588,59 @@ def pytest_collection_modifyitems(items, config):
for item in items:
deselect_test = False
- if config.getoption("--execute-upgrade-tests-only"):
- deselect_test = not item.get_closest_marker("upgrade_test")
- if deselect_test:
+ is_upgrade_test = _is_upgrade_test(item)
Review comment:
nit: I think it would not be bad to actually extract:
```
def should_skip_test(test, dtest_config):
```
because one could put `return` statements there which would not cause
reader to reflect if the `deselect` decision can be reverted.. making reasoning
easier.
and then the loop will become:
```
for item in items:
skip_test = self.should_skip_test(item, dtest_config)
if skip_test:
...
else:
....
```
##########
File path: conftest.py
##########
@@ -660,5 +652,17 @@ def pytest_collection_modifyitems(items, config):
items[:] = selected_items
-def _upgrade_testing_enabled(config):
- return config.getoption("--execute-upgrade-tests") or
config.getoption("--execute-upgrade-tests-only")
+def _is_upgrade_test(item):
+ if item.get_closest_marker("upgrade_test") is not None:
+ return True
+ else:
+ is_upgrade_test = False
+
+ for test_item_class in inspect.getmembers(item.module,
inspect.isclass):
+ if not hasattr(test_item_class[1], "pytestmark"):
+ continue
+ for module_pytest_mark in test_item_class[1].pytestmark:
+ if module_pytest_mark.name == "upgrade_test":
+ is_upgrade_test = True
Review comment:
if we want to keep this `for/if` then we can `break` once found.
but we can also go with more like
```
pytest_mark_names = [m.name for m in test_item_class[1].pytestmark
if "upgrade_test" in pytest_mark_names:
return True
```
##########
File path: conftest.py
##########
@@ -597,58 +588,59 @@ def pytest_collection_modifyitems(items, config):
for item in items:
deselect_test = False
- if config.getoption("--execute-upgrade-tests-only"):
- deselect_test = not item.get_closest_marker("upgrade_test")
- if deselect_test:
+ is_upgrade_test = _is_upgrade_test(item)
Review comment:
or even better:
```
skipped = [x for x in items if self.should_skip_test(x, dtest_config)
selected = list((set(items) - set(skipped))
```
no ifs, no loops, voila.
##########
File path: conftest.py
##########
@@ -660,5 +652,17 @@ def pytest_collection_modifyitems(items, config):
items[:] = selected_items
-def _upgrade_testing_enabled(config):
- return config.getoption("--execute-upgrade-tests") or
config.getoption("--execute-upgrade-tests-only")
+def _is_upgrade_test(item):
+ if item.get_closest_marker("upgrade_test") is not None:
+ return True
+ else:
+ is_upgrade_test = False
+
+ for test_item_class in inspect.getmembers(item.module,
inspect.isclass):
Review comment:
what will it be? a tuple? then it can be unboxed.
`test_item_class[1]` is rather cryptic ;-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
