Tighten test hygiene: scoped env mutation, narrower exception assertions - #309
Merged
Conversation
Clears 43 of the 44 open SonarCloud test-hygiene findings: 24 python:S5778,
12 python:S9073, 5 python:S8997, 2 of 3 python:S5958.
The S8997 group turned up a real bug rather than a style nit.
tests/test_config.py's tearDown deleted QUADLET_CONFIG_PATH,
QUADLET_MASTER_KEY and DEV_AUTO_LOGIN instead of restoring them.
.github/workflows/tests.yml sets QUADLET_MASTER_KEY for the whole job, so
running this TestCase unset a variable it never owned; only --dist=loadfile
sharding kept it from biting. setUp now snapshots those three and tearDown
restores the exact prior state. Demonstrated by running the TestCase
in-process with the variable set: before, it was None afterwards; after,
it survives intact.
The file is a unittest.TestCase, so the rule's suggested monkeypatch fix is
impossible there (monkeypatch is a pytest fixture and cannot be injected
into TestCase methods). Those three use patch.dict instead, which restores
prior values rather than clearing them. The two in test_crypto.py are plain
pytest functions and do use monkeypatch; both previously leaked, one via a
finally that restored DATABASE_PATH but not the env var, the other via a
pop on the last line that never ran if an earlier assert failed.
S5778: the second throwing invocation was always mock construction inside
the pytest.raises block, so the block could pass because MagicMock() threw.
Hoisted to a local; each block now contains exactly the call under test.
S5958: narrowed pytest.raises(Exception) to HostKeyMismatchError and
HTTPException, confirmed against the raising code. The third is left as-is
and commented: services/ssh_manager.py:118 genuinely raises bare Exception.
Narrowing it needs the app-side fix already tracked as python:S112 on that
exact line, so it is blocked rather than skipped.
S9073: composite assertions split so a failure identifies which half broke.
One split exposed a decorative assertion, `"(" in teardown_block`, which is
vacuously true alone; replaced with `"window._quadletLintDetach(" in
teardown_block`, which implies both halves and actually tests invocation.
Verified no coverage was lost: 159 test functions before and after, assert
count 251 to 262, no test removed, no exception type broadened.
|
Contributor
Up to standards ✅🟢 Issues
|
karoltheguy
added a commit
that referenced
this pull request
Aug 3, 2026
Fixes the quality gate on this PR. Adding DualProtocolCM to both test_log_level.py and test_session_duration.py pushed new-code duplication to 14% against a 3% threshold. The duplication was mostly pre-existing: the two files carried byte-identical copies of the login mock, the login helper and SettingsMockDB, 75 duplicated lines each. Touching those lines made them count as new code, which is the same inherit-the-debt effect seen on PR #304. Extracting the shared scaffolding fixes the cause rather than the symptom. The two files drop from 147 and 127 lines to 79 and 56, and shared blocks fall from 75 lines each to 22, which is now just imports and fixtures. Follows the existing tests/sudo_permissions.py precedent for a non-test helper module under tests/. test_sync_engine.py has its own DualProtocolCM copy, deliberately left alone here since it is modified on PR #309 and touching it would conflict. Unit 942 and unmarked 47, both unchanged.
karoltheguy
added a commit
that referenced
this pull request
Aug 3, 2026
…art 1) (#310) Fifth PR in the SonarCloud cleanup series (group 8: Python maintainability), after #303, #304, #308 and #309. Clears 13 of 19 findings. The 6 `python:S3776` cognitive-complexity refactors are held back for a second PR, since a control-flow change deserves isolated review and a revert that does not drag constant extraction with it. | Rule | Count | Status | |---|---|---| | `python:S1192` duplicated literals | 7 | Fixed | | `python:S108` empty block | 2 | Fixed, exposed two bad test mocks | | `python:S1066` nested if | 2 | Fixed | | `python:S112` generic exception | 2 | Fixed, unblocks a finding left open in #309 | | `python:S3776` cognitive complexity | 6 | Deferred to part 2 | ## S1192, and a count discrepancy worth knowing Sonar reports 3 occurrences each of `"/login"` and `"/change-password"`. The file actually contains **5 of each**, since Sonar's duplicate-literal count excludes route decorators. All occurrences were replaced, not just the flagged subset. Verified by AST rather than by eye: each of the seven literals now appears exactly once, as its constant definition, and **no other string literal in the file changed count**. That check is what makes a bulk find-and-replace across a 1700-line routing module trustworthy. ## S108 found a test that was propping up the outlier The two `async with db.execute(...): pass` blocks become `await db.execute(...)`, which is what **every other write site in the codebase** does. These two were the only exceptions. That broke two tests, and the tests were wrong. Their hand-rolled `SettingsMockDB.execute` returned a cursor supporting only `async with`, while a real `aiosqlite` `execute()` result supports both protocols. The repo already knows this: `tests/test_sync_engine.py:90` defines `DualProtocolCM` whose docstring says *"Objects returned by aiosqlite execute() support both `async with obj` and `await obj`."* Both mocks now use that same pattern, so the fake stops dictating which idiom production code may use. Worth being explicit, since "the change broke tests so I changed the tests" deserves scrutiny: the mock was asserting an implementation detail, not behavior. The tests still prove the same thing, that settings persist and apply live. ## S112, and why these do not subclass SSHCommandError The two bare `raise Exception` in `ssh_manager.py` become `ServerConfigurationError` and `KeyDecryptionError`, both subclassing `Exception` directly. Not `SSHCommandError`, because that type carries `exit_status` and `stderr` for a command that actually ran, and `tests/test_ssh_manager.py:137-138` asserts on exactly those fields. Both new errors fire while assembling the connection, before anything reaches the host, so typing them as command failures would hand callers a null exit status and imply a command failed when none was issued. Caller audit: all ~41 handlers in the codebase use `except Exception`, and there are no exact-type comparisons such as `type(e) is Exception`, so nothing downstream changes. Three new tests cover both types plus `__cause__` chaining, and they fail against the previous code at import time. **Follow-up this enables:** #309 left one `python:S5958` open because `ssh_manager.py:118` raised bare `Exception`, with a comment saying so. Once both merge, that comment is stale and the test can narrow to `KeyDecryptionError`. It could not be done in either PR alone without a conflict, since `tests/test_crypto.py` is modified in #309. ## S1066 Two nested ifs merged. `and` short-circuits, so `_drop_if_stale()`'s side effect still fires only when the connection is cached, exactly as before, and the conditions are not reordered. **One deliberate extra, not flagged by Sonar:** the structurally identical re-check inside `get_connection`'s lock was merged too. Fixing only the flagged one left two adjacent, semantically identical checks written differently, which is worse than either form. Easy to drop if you would rather keep the diff strictly to the findings. ## Verification * `-m unit`: 939 to 942, the three new exception tests, nothing lost * unmarked suite: 47 passed, unchanged * re-run in random order to catch cross-test contamination: 942 passed * AST verification of literal extraction as described above ## Deliberately skipped, with reasons Three `S3776` findings will not be fixed even in part 2: * `sync_engine.py:166` `check_quadlets` (complexity 25) and `sync_engine.py:114` `_fetch_mtimes`. These are the subject of open bug #283: the function iterates the `quadlets` table, and the only `INSERT INTO quadlets` anywhere in the repo is in test files, so it is dead in production. Its uncovered branches are **uncoverable by construction** while the work set is always empty, meaning no test can protect a refactor of the most complex function in the group. Whoever fixes #283 rewrites it anyway. * `scripts/import_servers.py:12`, a one-off script not referenced by `Dockerfile` or `start.sh`, so not shipped, and not covered. Restructuring untested, unshipped code to satisfy a metric is risk without benefit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth PR in the SonarCloud cleanup series (group 3: Python test hygiene), after #303, #304 and #308. Clears 43 of the 44 findings; the 44th is blocked on an app-side fix and explained below.
python:S5778exception test has multiple invocationspython:S9073composite assertionpython:S8997global state mutationpython:S5958assertion too broadThe one that was a real bug, not hygiene
tests/test_config.py'stearDowndeleted three environment variables instead of restoring them:.github/workflows/tests.yml:22setsQUADLET_MASTER_KEYfor the entire job, so running this TestCase silently unset a variable it never owned. It has not bitten yet only because--dist=loadfileputs each file in its own worker, which masks the contamination.setUpnow snapshots the three variables andtearDownrestores the exact prior state, including restoring "was absent" as absent. Demonstrated by running the TestCase in-process with the variable set:Note the rule's own suggested fix is impossible in that file: it is a
unittest.TestCase, andmonkeypatchis a pytest fixture that cannot be injected intoTestCasemethods. Those three usepatch.dictinstead, which restores prior values rather than clearing them. The two findings intest_crypto.pyare plain pytest functions and do usemonkeypatch; both genuinely leaked, one via afinallythat restoredDATABASE_PATHbut not the env var, the other via apopon the last line that never runs if an earlier assert fails.The blocked one
tests/test_crypto.pykeepspytest.raises(Exception)with a comment saying why.services/ssh_manager.py:118genuinely raises bareExceptionon the decrypt-failure path, so there is no more specific type to narrow to. That line is already tracked aspython:S112("replace this generic exception class with a more specific one") and is scheduled in group 8. Once the app raises a dedicated exception, this test can narrow. Inventing a type here would have made the test pass for the wrong reason.What the mechanical fixes actually change
S5778: the offending second invocation was always mock construction inside the block, so it could pass becauseMagicMock()threw rather than the code under test. Hoisted to a local; each block now contains exactly the call under test.S9073: one split is worth a look. It exposed a decorative half:Standing alone, "this JavaScript contains an open paren" is vacuously true. Replaced with
assert "window._quadletLintDetach(" in teardown_block, which implies both original halves and actually tests invocation:Verification
Test hygiene changes can quietly delete coverage, so this was checked by AST rather than by eye:
pytest.raises(Exception)went 3 to 1, the remainder being the blocked case-m unit: 939 passed, 3 skipped. Unmarked suite: 47 passed-p randomlyenabled) to catch any new cross-test contamination: 939 passed