Cleanup warnings in tests #6911
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6911 +/- ##
==========================================
- Coverage 79.20% 78.99% -0.20%
==========================================
Files 566 566
Lines 43461 43461
==========================================
- Hits 34417 34327 -90
- Misses 9044 9134 +90 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danielhollas
force-pushed
the
cleanup-click-tests
branch
from
bb462bf to
9103cc8
Compare
| @@ -2095,7 +2095,7 @@ def test_1(self): | |||
| ) | |||
| tmpf.flush() | |||
| pymatgen_parser = CifParser(tmpf.name) | |||
| pymatgen_struct = pymatgen_parser.get_structures()[0] | |||
| pymatgen_struct = pymatgen_parser.parse_structures(primitive=True)[0] | |||
There was a problem hiding this comment.
tests/test_dataclasses.py::TestStructureDataFromPymatgen::test_1
tests/test_dataclasses.py:2098: FutureWarning: get_structures is deprecated;
use parse_structures in pymatgen.io.cif instead.
The only difference is that primitive defaults to False in the new parse_structures method.
So parse_structures(primitive=True) is equivalent to the old behavior of get_structures().
pymatgen_struct = pymatgen_parser.get_structures()[0]
There was a problem hiding this comment.
Thanks for the explanation, since they are equivalent is there a specific reason for change?
There was a problem hiding this comment.
The only reason is to get rid of the warning.
danielhollas
force-pushed
the
cleanup-click-tests
branch
from
9103cc8 to
2b9dd8b
Compare
tests/test_dataclasses.py::TestStructureDataFromPymatgen::test_1 tests/test_dataclasses.py:2098: FutureWarning: get_structures is deprecated; use parse_structures in pymatgen.io.cif instead. The only difference is that primitive defaults to False in the new parse_structures method. So parse_structures(primitive=True) is equivalent to the old behavior of get_structures(). pymatgen_struct = pymatgen_parser.get_structures()[0]
danielhollas
force-pushed
the
cleanup-click-tests
branch
from
2b9dd8b to
e494510
Compare
| @@ -95,14 +95,11 @@ async def exponential_backoff_retry_fail_kill(fct: t.Callable[..., t.Any], *args | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope='function') | |||
| @pytest.mark.usefixtures('started_daemon_client') | |||
There was a problem hiding this comment.
This was throwing a warning since usefixtures decorator should be used on tests, not on fixtures.
| @@ -378,7 +378,7 @@ ignore_errors = true | |||
| module = 'plumpy' | |||
|
|
|||
| [tool.pytest.ini_options] | |||
| addopts = '--benchmark-skip --durations=50 --durations-min=1 --strict-config --strict-markers -ra --cov-report xml --cov-append ' | |||
There was a problem hiding this comment.
I removed the --durations parameter as a default (which reports on slow tests), it just creates noise most of the time imho. If you need it you can specify on the command line.
There was a problem hiding this comment.
MAybe at least keep 10, or 5
So that future developers can be reminded of performance, etc.
I'm also fine to remove it, no strong opinions..
There was a problem hiding this comment.
Okay, I've kept 5, so that when developing a new test one can easily see how long it takes. Thanks!
There was a problem hiding this comment.
All good @danielhollas
Thanks a lot!
I've put two minor comments.
| @@ -2095,7 +2095,7 @@ def test_1(self): | |||
| ) | |||
| tmpf.flush() | |||
| pymatgen_parser = CifParser(tmpf.name) | |||
| pymatgen_struct = pymatgen_parser.get_structures()[0] | |||
| pymatgen_struct = pymatgen_parser.parse_structures(primitive=True)[0] | |||
There was a problem hiding this comment.
Thanks for the explanation, since they are equivalent is there a specific reason for change?
| @@ -378,7 +378,7 @@ ignore_errors = true | |||
| module = 'plumpy' | |||
|
|
|||
| [tool.pytest.ini_options] | |||
| addopts = '--benchmark-skip --durations=50 --durations-min=1 --strict-config --strict-markers -ra --cov-report xml --cov-append ' | |||
There was a problem hiding this comment.
MAybe at least keep 10, or 5
So that future developers can be reminded of performance, etc.
I'm also fine to remove it, no strong opinions..
|
One failing test was a case of this flake: #6853 (comment) |
Split from #6883.
Changes here bring down the number of warnings when running presto tests from over 100 to 30.
See inline comments for more details, most of the changes are straightforward, just removing usage of deprecated methods.