SonarCloud has started firing python:S5778 — "Refactor this exception test to have only one invocation possibly throwing an exception" — across the test suite. 56 open sites across 12 files. Split out of the PR #48 review round, where one newly introduced instance was fixed and the rest were left alone to keep that PR focused on its safety fix.
The finding is real — this isn't rule-appeasement
The flagged idiom is everywhere in this suite:
with pytest.raises(CliError) as excinfo:
arm_cmd.cmd_arm_limits(_args(joint=[ELBOW]))
Two calls can throw inside that block, not one. If _args(...) — the fixture builder — ever raised a CliError (a bad joint name, a malformed pose, a future validation added to it), the test would still go green, and it would be asserting a CliError the command under test never raised. The test would be pinning its own fixture.
That is exactly the failure mode this repo has been burned by on this branch: a verdict that replicates for the wrong reason is worse than no verdict. A test that cannot distinguish "the command raised" from "the setup raised" is the same bug in the test layer.
The fix
Hoist the builder above the block, so the only call that may throw inside it is the one under test:
args = _args(joint=[ELBOW])
with pytest.raises(CliError) as excinfo:
arm_cmd.cmd_arm_limits(args)
Mechanical, and behaviour-preserving. Reference fix already merged into #48: tests/test_arm_limits_commit.py @ 97f0f49.
Scope
| File |
Sites |
tests/test_arm_rezero_cli.py |
11 |
tests/test_rolling_frame.py |
8 |
tests/test_arm_explore_cli.py |
7 |
tests/test_classify.py |
6 |
tests/test_arm_profile_cli.py |
6 |
tests/test_limits.py |
4 |
tests/test_arm_limits_commit.py |
3 |
tests/test_soft_limit_store.py |
3 |
tests/test_arm_limits_cli.py |
3 |
tests/test_calibration_journal.py |
2 |
tests/test_rezero.py |
2 |
tests/test_soft_limit_enforcement.py |
1 |
Notes for whoever picks this up
- These are pre-existing, not a regression. Sonar backdates
creationDate to each line's blame date, so when the rule started firing the whole suite lit up at once and looked like a fresh regression. It isn't.
- The quality gate passes with all 56 open (they are MAJOR test-code smells, not gate conditions), so there is no CI pressure here — this is hygiene, and it should land as a reviewable test-only diff on its own branch.
- Check each site as you go rather than blind-substituting: a few may have the builder inside the block deliberately because the builder's raise is the thing under test. Those should keep one throwing call — the builder — and drop the command, not the other way round.
- Remember the version bump (
version-check blocks the PR otherwise), even for a test-only change.
Found during PR #48 review triage.
SonarCloud has started firing
python:S5778— "Refactor this exception test to have only one invocation possibly throwing an exception" — across the test suite. 56 open sites across 12 files. Split out of the PR #48 review round, where one newly introduced instance was fixed and the rest were left alone to keep that PR focused on its safety fix.The finding is real — this isn't rule-appeasement
The flagged idiom is everywhere in this suite:
Two calls can throw inside that block, not one. If
_args(...)— the fixture builder — ever raised aCliError(a bad joint name, a malformed pose, a future validation added to it), the test would still go green, and it would be asserting aCliErrorthe command under test never raised. The test would be pinning its own fixture.That is exactly the failure mode this repo has been burned by on this branch: a verdict that replicates for the wrong reason is worse than no verdict. A test that cannot distinguish "the command raised" from "the setup raised" is the same bug in the test layer.
The fix
Hoist the builder above the block, so the only call that may throw inside it is the one under test:
Mechanical, and behaviour-preserving. Reference fix already merged into #48:
tests/test_arm_limits_commit.py@97f0f49.Scope
tests/test_arm_rezero_cli.pytests/test_rolling_frame.pytests/test_arm_explore_cli.pytests/test_classify.pytests/test_arm_profile_cli.pytests/test_limits.pytests/test_arm_limits_commit.pytests/test_soft_limit_store.pytests/test_arm_limits_cli.pytests/test_calibration_journal.pytests/test_rezero.pytests/test_soft_limit_enforcement.pyNotes for whoever picks this up
creationDateto each line's blame date, so when the rule started firing the whole suite lit up at once and looked like a fresh regression. It isn't.version-checkblocks the PR otherwise), even for a test-only change.Found during PR #48 review triage.