- Notifications
You must be signed in to change notification settings - Fork446
Lint tests#1127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Lint tests#1127
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Where it's clear that function is called for side effects (e.g., in aq`with pytest.raises` block), don't assign function output. Whereit's not clear, e.g., binary ops on LTI objects, call result `_sys` orsimilar. There are plenty of in-between cases: for those I chosebased on understandability.
Where imports were test fixtures, replaced with`@pytest.mark.usefixtures('nameoffixture')`.Provide relevant symbols via `locals` argument to eval.
Symbol reference in lambda which is never called.
The test had no assertions, but have been intended to test MIMOfeedback; for this see testMIMOfb in same file.
The function doesn't exist, but the test is never called.
slycontonly is not a fixture.
It looks like a block of code was copied-and-pasted between two testfunctions; removed the unused variable in each case.
Only works from Python 3.13.
coveralls commentedFeb 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
murrayrm commentedFeb 16, 2025
Correct that there just needs to be some test that verifies that unknown keywords generate an error. Whichever of the tests that has that check should be kept and if both do then keeping either is fine.
Correct that the intent of this test was just to make sure an exception wasn't raised. We could either get rid of the assignment to
I'm pretty sure I wrote this example and I can't see any reason to keep |
roryyorke commentedFeb 16, 2025
Oops - had a commit there I'd intended for another PR. Reverted. If this PR isn't merged now, it'll be worth merging main into the PR before merging the PR to main, to make sure the lint checks don't causes failures on main after the merge of the PR. (I hope that sentence is less confusing than I think it is!) |
f6799ab intopython-control:mainUh oh!
There was an error while loading.Please reload this page.
This extends ruff testing to control/tests/*.py
I've tried to order the commits from obvious to subtle; on the whole:
evalcallsThe remaining errors I'm less sure of. It would be simple to silence them with
# noqa: F<relevant code>, and defer fixing these to later; then at least no new ruff errors will sneak in in the future.More detail on remaining failures:
control/tests/kwargs_test.pyHere the dict literal has repeated keys for
'StateSpace.__init__'and'StateSpace.__sample':Last-value-wins in dict literals, so the first two entries have no effect. It looks like this is just a check thatsomething exists, so I guess we can just erase the first two entries?
control/tests/modelsimp_test.pyThe variable
mrkis assigned to:mrk = markov(outp, inp, 1, transpose=False). I didn't investigate issue#395 referenced above -- is it enough that this call doesn't raise?control/tests/optimal_test.py
curved_seg_lengthis calculated, but not used. It's not obviously copy-and-pasted from another test, so I'm not sure if the test should be changed here. It's obviously easy enough to just remove this variable, andstraight_seg_lengthjust above it.