You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/01/22 07:57:01 UTC
[GitHub] [cassandra-dtest] jacek-lewandowski opened a new pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
jacek-lewandowski opened a new pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r562474277
##########
File path: conftest.py
##########
@@ -602,20 +609,17 @@ def pytest_collection_modifyitems(items, config):
if deselect_test:
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 item.get_closest_marker("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)
+ elif not force_resource_intensive:
if not sufficient_system_resources_resource_intensive:
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 not item.get_closest_marker("resource_intensive"):
Review comment:
Not that I can remember, unfortunately :(
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564093530
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
> I can spend 1-2 hours on fixing that properly
i have a PR somewhere for that :-)
(it would need to be a separate ticket.)
let's just comment out the line then for now 👍
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] jacek-lewandowski commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
jacek-lewandowski commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r562473099
##########
File path: conftest.py
##########
@@ -602,20 +609,17 @@ def pytest_collection_modifyitems(items, config):
if deselect_test:
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 item.get_closest_marker("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)
+ elif not force_resource_intensive:
if not sufficient_system_resources_resource_intensive:
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 not item.get_closest_marker("resource_intensive"):
Review comment:
@tlasica It was already mentioned as a problem in https://issues.apache.org/jira/browse/CASSANDRA-16221
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r562460898
##########
File path: conftest.py
##########
@@ -602,20 +609,17 @@ def pytest_collection_modifyitems(items, config):
if deselect_test:
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 item.get_closest_marker("resource_intensive"):
Review comment:
this code is so complex that IMO it would be reasonable to add tests in `meta_tests` with acceptance criteria validation, so that one does not have to reverse engineer the rules.
decision making for single `item` should be extracted to either a class or a function so that it can be easily tested without whole configuration burden.
not blocking the fix.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564031720
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
instead of commenting it out, could we add more ignores (for those currently failing?)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r562474277
##########
File path: conftest.py
##########
@@ -602,20 +609,17 @@ def pytest_collection_modifyitems(items, config):
if deselect_test:
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 item.get_closest_marker("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)
+ elif not force_resource_intensive:
if not sufficient_system_resources_resource_intensive:
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 not item.get_closest_marker("resource_intensive"):
Review comment:
Not that I can remember, unfortunately :(
(though @jacek-lewandowski is correct, the fault has already been illustrated)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#issuecomment-765904473
Up to https://github.com/apache/cassandra-dtest/pull/115/commits/b4fc4f07c2cc4c2779250f672bc09d9403647bb1 LGTM!
Given the size of the patch, even if it's simple stuff in practice, it would be nice to see some tests for the different combinations added to `meta_tests/`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever closed pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever closed pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#issuecomment-768975190
Merged with https://github.com/apache/cassandra-dtest/commit/ec84618b7450ef9357a3a88fc93e39d74a34b02e
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564031176
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
- git remote add apache git://github.com/apache/cassandra-dtest.git
- git fetch apache # fetch master for the next diff
# feed changed lines with no context around them to pycodestyle
# 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/master...HEAD -U0 | pycodestyle --ignore=E501 --diff
+ - git diff apache/trunk...HEAD -U0 | pycodestyle --ignore=E501 --diff
# lint all files except json_test.py for line length errors
- - git diff apache/master...HEAD -U0 | pycodestyle --diff --exclude='json_test.py' --exclude='meta_tests/assertion_test.py' --max-line-length=200
+ - git diff apache/trunk...HEAD -U0 | pycodestyle --diff --exclude='json_test.py' --exclude='meta_tests/assertion_test.py' --max-line-length=200
+ - pytest --metatests
Review comment:
💪
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
instead of commenting it out, could we add more ignores (for those currently failing?)
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
instead of commenting it out, could we add more ignores (for those currently failing?)
(duplicate and comment out the line so we know what it is supposed to be)
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
> I can spend 1-2 hours on fixing that properly
i have a PR somewhere for that :-)
(it would need to be a separate ticket.)
let's just comment out the line then for now 👍
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r563763263
##########
File path: meta_tests/conftest_test.py
##########
@@ -0,0 +1,87 @@
+import os
+from re import search
+from unittest import TestCase
+
+from dtest_config import DTestConfig
+from mock import Mock
+from pytest import UsageError
+
+from conftest import *
+
+
+def _response(input, responses, default_response):
Review comment:
why not `responses.get(input, default_response)` ?
because of logging?
I think we do not need this function at all.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] jacek-lewandowski commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
jacek-lewandowski commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564053574
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
I don't know if you tried, but there are so many of them that is probably make no sense at all - instead, I can spend 1-2 hours on fixing that properly
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r563763263
##########
File path: meta_tests/conftest_test.py
##########
@@ -0,0 +1,87 @@
+import os
+from re import search
+from unittest import TestCase
+
+from dtest_config import DTestConfig
+from mock import Mock
+from pytest import UsageError
+
+from conftest import *
+
+
+def _response(input, responses, default_response):
Review comment:
why not `responses.get(input, default_response)` ?
because of logging?
##########
File path: meta_tests/conftest_test.py
##########
@@ -0,0 +1,87 @@
+import os
+from re import search
+from unittest import TestCase
+
+from dtest_config import DTestConfig
+from mock import Mock
+from pytest import UsageError
+
+from conftest import *
+
+
+def _response(input, responses, default_response):
Review comment:
why not `responses.get(input, default_response)` ?
because of logging?
I think we do not need this function at all.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564301868
##########
File path: meta_tests/dtest_config_test.py
##########
@@ -0,0 +1,121 @@
+import os
+from re import search
+from unittest import TestCase
+
+from dtest_config import DTestConfig
+from mock import Mock, patch
+from pytest import UsageError
+import ccmlib.repository
+import ccmlib.common
+
+
+def _mock_responses(responses, default_response=None):
+ return lambda input: responses[input] if input in responses else \
+ "%s/meta_tests/cassandra-dir-4.0-beta" % os.getcwd() if input == "--cassandra-dir" else default_response
+
+
+def _check_with_params(params):
+ config = Mock()
+ config.getoption.side_effect = _mock_responses(params)
+ config.getini.side_effect = _mock_responses({})
+
+ dTestConfig = DTestConfig()
+ dTestConfig.setup(config)
+ return dTestConfig
+
+
+def _check_with_params_expect(params, pattern):
Review comment:
reading the name I would say we always expect exception.
but the code as it is does not check if exception is actually thrown,
it only checks if "when exception is thrown it has proper message".
maybe we should instead use:
```with pytest.raises(ValueError, match=r".* 123 .*"):```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564031720
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
instead of commenting it out, could we add more ignores (for those currently failing?)
(duplicate and comment out the line so we know what it is supposed to be)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] michaelsembwever commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564031176
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
- git remote add apache git://github.com/apache/cassandra-dtest.git
- git fetch apache # fetch master for the next diff
# feed changed lines with no context around them to pycodestyle
# 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/master...HEAD -U0 | pycodestyle --ignore=E501 --diff
+ - git diff apache/trunk...HEAD -U0 | pycodestyle --ignore=E501 --diff
# lint all files except json_test.py for line length errors
- - git diff apache/master...HEAD -U0 | pycodestyle --diff --exclude='json_test.py' --exclude='meta_tests/assertion_test.py' --max-line-length=200
+ - git diff apache/trunk...HEAD -U0 | pycodestyle --diff --exclude='json_test.py' --exclude='meta_tests/assertion_test.py' --max-line-length=200
+ - pytest --metatests
Review comment:
💪
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564301868
##########
File path: meta_tests/dtest_config_test.py
##########
@@ -0,0 +1,121 @@
+import os
+from re import search
+from unittest import TestCase
+
+from dtest_config import DTestConfig
+from mock import Mock, patch
+from pytest import UsageError
+import ccmlib.repository
+import ccmlib.common
+
+
+def _mock_responses(responses, default_response=None):
+ return lambda input: responses[input] if input in responses else \
+ "%s/meta_tests/cassandra-dir-4.0-beta" % os.getcwd() if input == "--cassandra-dir" else default_response
+
+
+def _check_with_params(params):
+ config = Mock()
+ config.getoption.side_effect = _mock_responses(params)
+ config.getini.side_effect = _mock_responses({})
+
+ dTestConfig = DTestConfig()
+ dTestConfig.setup(config)
+ return dTestConfig
+
+
+def _check_with_params_expect(params, pattern):
Review comment:
this will pass if exception is not thrown, why actually it should be thrown, correct
we should instead use:
```with pytest.raises(ValueError, match=r".* 123 .*"):```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] jacek-lewandowski commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
jacek-lewandowski commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r564053574
##########
File path: .travis.yml
##########
@@ -1,21 +1,23 @@
language: python
python:
- - "2.7"
+ - "3.8"
install:
- - pip install pycodestyle==2.3.1 flake8
+ - pip install pycodestyle==2.6.0 flake8
- pip check
+ - CASS_DRIVER_NO_CYTHON=1 pip install -r requirements.txt
script:
# we want pyflakes to check all files for unused imports only
# we use flake8 because it allows us to ignore other warnings
# exclude the thrift directories - they contain auto-generated code
- - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
+ # - flake8 --ignore=E501,F811,F812,F822,F823,F831,F841,N8,C9 --exclude=thrift_bindings,cassandra-thrift .
Review comment:
I don't know if you tried, but there are so many of them that is probably make no sense at all - instead, I can spend 1-2 hours on fixing that properly
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r563763263
##########
File path: meta_tests/conftest_test.py
##########
@@ -0,0 +1,87 @@
+import os
+from re import search
+from unittest import TestCase
+
+from dtest_config import DTestConfig
+from mock import Mock
+from pytest import UsageError
+
+from conftest import *
+
+
+def _response(input, responses, default_response):
Review comment:
why not `responses.get(input, default_response)` ?
because of logging?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-dtest] tlasica commented on a change in pull request #115: CASSANDRA-16399: Fix collecting resource intensive tests
Posted by GitBox <gi...@apache.org>.
tlasica commented on a change in pull request #115:
URL: https://github.com/apache/cassandra-dtest/pull/115#discussion_r562460898
##########
File path: conftest.py
##########
@@ -602,20 +609,17 @@ def pytest_collection_modifyitems(items, config):
if deselect_test:
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 item.get_closest_marker("resource_intensive"):
Review comment:
this code is so complex that IMO it would be reasonable to add tests in `meta_tests` with acceptance criteria validation, so that one does not have to reverse engineer the rules.
decision making for single `item` should be extracted to either a class or a function so that it can be easily tested without whole configuration burden.
##########
File path: conftest.py
##########
@@ -594,6 +594,13 @@ def pytest_collection_modifyitems(items, config):
sufficient_system_resources_resource_intensive = sufficient_system_resources_for_resource_intensive_tests()
logger.debug("has sufficient resources? %s" % sufficient_system_resources_resource_intensive)
+ force_resource_intensive = config.getoption("--force-resource-intensive-tests")
+ skip_resource_intensive = config.getoption("--skip-resource-intensive-tests")
+ only_resource_intensive = config.getoption("--only-resource-intensive-tests")
+ if skip_resource_intensive and only_resource_intensive:
Review comment:
also `skip` and `force` should not be combined?
##########
File path: conftest.py
##########
@@ -602,20 +609,17 @@ def pytest_collection_modifyitems(items, config):
if deselect_test:
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 item.get_closest_marker("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)
+ elif not force_resource_intensive:
if not sufficient_system_resources_resource_intensive:
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 not item.get_closest_marker("resource_intensive"):
Review comment:
I wonder what was the rationale for running this logic only if `not collect_only`?
maybe there was a valid reason not to do it always?
@michaelsembwever ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org