chore: enable pytest ruff rule and apply fixes #4928
Conversation
Aswinr24
changed the title
There was a problem hiding this comment.
Thanks, @Aswinr24! Where did you get the match="string" strings from, as most of them are wrong? Is it from an older version of PyBaMM?
Please see my comments below to fix the tests.
tests/unit/test_expression_tree/test_operations/test_convert_to_casadi.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks, @Aswinr24! This looks good now. Could you please resolve the conflicts?
|
Yes thanks!, would resolve the conflicts and push it |
|
Was thinking to open the PR to enable isort rule along with its suggested fixes as well |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4928 +/- ##
========================================
Coverage 98.58% 98.58%
========================================
Files 304 304
Lines 23689 23689
========================================
Hits 23353 23353
Misses 336 336 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -689,7 +689,7 @@ def test_failures(self): | |||
| solver = pybamm.IDAKLUSolver() | |||
|
|
|||
| t_eval = [0, 3] | |||
| with pytest.raises(ValueError): | |||
| with pytest.raises(ValueError, match="std::exception"): | |||
There was a problem hiding this comment.
The error raised is different on Windows and UNIX. Can we not have match= here (ignore the rule) and add a comment for future reference -
# raises `std::exception` on UNIX and `IDA failed with flag -22` on Windows
with pytest.raises(ValueError) # noqa: ...
...There was a problem hiding this comment.
Sure, Will add this comment and ignore the rule here
| @@ -894,7 +894,7 @@ def test_solver_options(self): | |||
| options = {option: options_fail[option]} | |||
| solver = pybamm.IDAKLUSolver(options=options) | |||
|
|
|||
| with pytest.raises(ValueError): | |||
| with pytest.raises(ValueError, match="std::exception"): | |||
Yes, sounds good to me. |
|
pre-commit.ci run |
|
Thanks! |
| interp = pybamm.Interpolant(x4_, data4, y4, interpolator="linear") | ||
| interp_casadi = interp.to_casadi(y=casadi_y) | ||
| interp_casadi = interp.to_casadi(y=casadi_y) | ||
|
|
There was a problem hiding this comment.
This part looks off to me. If the raise value error happens in the pybamm.Interpolant() line then the to_casadi() line does nothing
There was a problem hiding this comment.
There are a few other places with similar changes
| with pytest.raises(pybamm.OptionError, match="natural numbers"): | ||
| options = {"number of rc elements": -1} | ||
| options = {"number of rc elements": -1} | ||
| with pytest.raises(pybamm.OptionError, match="natural numbers"): # noqa: PT012 |
There was a problem hiding this comment.
What is being suppressed here?
Description
This PR enables additional Ruff linting rules and applies fixes to the codebase as suggested by Ruff. The changes include:
Enabling PT (pytest-style) rules in pyproject.toml.
fixes issues raised by ruff linter which included:
pytest.raises()is called with amatchparameter to avoid catching overly broad exceptions.pytest.raises()context managers contains only a single statement.Related to #4925
Type of change
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python -m pytest(or$ nox -s tests)$ python -m pytest --doctest-plus src(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick.Further checks: