Repository navigation
test(quality): ten tests that could not fail — bare 'except Exception' swallowing AssertionError, and one asserting imports it never performed #15189
Description
Activity
Correcting this issue's premise — the remedy I proposed would have caught one of the ten
I wrote above that "the detector already existed and was ignored because it was advisory", and set an acceptance criterion to make pytest's return-not-None warning fatal. That framing is wrong, and the criterion as written is insufficient.
There is already a fatal guard for exactly this defect:
repo_tests/tests_that_return_instead_of_asserting_test.py(#14920), an AST sweep with a per-tree down-only ratchet. It ran, it gates, and it did not catch nine of the ten — for a structural reason:if _own_nodes(function, (ast.Assert, ast.Raise)): continue
The exclusion treats an
assertthat is present as anassertthat can fire. Under a bareexcept Exception, it cannot. Nine of the ten functions contained asserts — inert ones — so the sweep skipped them. Measured against the base file, it attributed only 2 offending returns in the whole module, both insidetest_migrated_files_import, the one function with noassertat all:BASE offending in that file: 2 [('test_migrated_files_import', 288), (…, 281)] NOW offending in that file: 0So the shape was worse than neglected. It tripped
PytestReturnNotNoneWarningat runtime and read as defended to the AST guard. Both detectors agreed the file was fine — one because it only warns, the other because it mistook a present assertion for an effective one.That is the real finding, and it is filed as #15195, with the concrete rule: an
assertis not protective when it sits under a handler catchingException,BaseExceptionorAssertionErrorwithout re-raising.What this changes here
- The criterion "make the return-not-None detector fatal" stays useful but is not sufficient — on its own it would have caught one of these ten. Read it alongside test-guard: the returning-test sweep cannot see an assert neutralised by a bare except #15195 rather than as the fix.
- The sweep criteria above remain right, and the second shape (a body of
passplus prints claiming an assertion) is still uncovered by either detector. - Closing test-guard: the returning-test sweep cannot see an assert neutralised by a bare except #15195's blind spot will make the measured population rise. That is a detector improvement, not a ratchet violation, and it needs saying in that file so the down-only rule is not read as forbidding it.
I filed this issue on the strength of a report I did not check against the guard's source. The agent that found it corrected me, and the corrected version is a better finding than the one I filed.
- added 4 commits that reference this issue
on Aug 28, 2026 Two of the three remaining sites cleared.
1c7688e9ff(PR #15393), verified against mergedDev_new_gui:_KNOWN_OFFENDERS["autobot-backend"]is now 75 (was 86) and_SWALLOWED_ASSERTIONS["autobot-backend"]is 4 (was 5). Zeroexcept Exceptionremain inconfig_consolidation_p2_test.py— the single textual match on merged base is the module docstring describing the old pattern.What came out from under the swallow is worse than the swallow itself. Ten checks lived in one function, each wrapped
try: assert ...; except Exception: return False. Removing that revealed:- Test 2 never tested anything, and took eight others with it. It called
unified_config_manager._get_default_config()— a method that does not exist on that object; the real one is module-levelconfig.defaults.get_default_config(). TheAttributeErrorwas caught and the function returned, so tests 3 through 10 never executed at all while the suite reported PASSED. One non-existent method call silently disabled nine tests. - Test 4 wrote a live
config/test.jsoninto the repository root on every run —ConfigManager.config_dirresolves fromPATH.PROJECT_ROOT, which is the repo root, notautobot-backend/. A test mutating the working tree, on a machine that also hosts a CI runner. Now redirected viamonkeypatch+tmp_path. - Test 7 was vacuous independently of the swallow — it printed whichever env var happened to be set and asserted nothing either way. Now a real round trip through
config.loader.apply_env_overrides. - Test 8 asserted a stale pre-fix value (
0.8), where bug(multimodal): VoiceProcessor reads multimodal.audio but defaults define multimodal.voice — config silently ignored #13207 deliberately set0.7.
The ten are now independent top-level tests, so one failure can no longer suppress the rest — which was the actual defect, more than the bare except.
One flake was caught in review and fixed before merge: the env-override test asserted strict dict equality against the entire
apply_env_overridesresult, which would fail whenever any of ~15 otherAUTOBOT_*vars happened to be set — sourcing.env.localhostis a documented workflow here. Now scoped to the key it controls.Not closing. The four
test_causal_framework_integration.pyaggregator-consumed*_full_pipelinemethods remain — 1145 lines, grandfathered inpython_file_size_known_large.py, needing #15166-style restructuring. That is its own scope, and the guard infrastructure to catch regressions is already in place from #15254/#15241/#15256/#15351.- Test 2 never tested anything, and took eight others with it. It called
Closing — all six criteria met on merged base, verified against
git show origin/Dev_new_gui:.AC evidence 1 — sweep for the try: assert…; return True / except Exception: return Falseshape and report the countrepo_tests/tests_that_return_instead_of_asserting_test.py:266_SWALLOWED_ASSERTIONS, enforced bytest_no_test_smothers_its_own_assertions_under_a_swallowing_handlerat:428. Ten in one file was indeed not the whole population.2 — sweep separately for the weaker form, any test function returning a value test_no_tree_outside_the_known_set_has_a_test_that_returns_a_valueat:349.3 — make the detector fatal, or record the population as a down-only ratchet _KNOWN_OFFENDERSat:139, enforced bytest_the_known_offender_budgets_only_ever_shrinkat:369.4 — sweep for the second shape: a body that is passor onlyprint()repo_tests/tests_that_execute_nothing_test.py, drivingempty_bodies_infromrepo_tests/collected_test_model.py:406-423.5 — contrast mutation: reintroduce a swallowing test, show CI fails test_the_detector_finds_a_planted_return_and_spares_the_legitimate_onesat:498— the detector is run against a planted offender and against legitimate tests, so it proves both directions rather than only that something fails.6 — confirm cache_consolidation_p4_test.pywas in no exemption or skip list, and record the answer either wayAnswer: it was not. No filterwarningsentry exists inpytest.ini,setup.cfg,pyproject.tomlorautobot-backend/pytest.ini— the count is 0 in all four. Its only references anywhere are.test_durations(a timing record),scripts/migrate_tests.py(a path map), and the two guards' own changelog comments. So the warning was emitted normally and was simply non-fatal; nothing suppressed it.Two things beyond the checklist that make the guard non-vacuous, and are worth noting because this issue is about checks that cannot fail: a population floor at
:330(_MIN_MODULES = 1800,_MIN_TEST_FUNCTIONS = 25000) so a collapsed sweep fails by name instead of reading clean, andtest_an_unparseable_swept_file_fails_loudly_instead_of_reading_as_cleanat:578so a file the sweep cannot parse is not silently counted as compliant.- added a commit that references this issue
on Sep 3, 2026
Found while converting live-service drivers for #14979, in a file that conversion never touched.
What
autobot-backend/cache/cache_consolidation_p4_test.py— all ten test functions were structurally incapable of failing. Every one was shaped:A bare
except ExceptioncatchesAssertionError. So a failing assertion returnedFalseinstead of propagating, and pytest treats a returning test as passing (with aDid you mean to use assert instead of return?warning, which is how it was noticed). Ten green tiles, zero enforceable assertions.One was worse than vacuous — it was a false claim.
test_migrated_files_importhad a body ofpassplus sixprint()statements announcing that six modules import successfully. Theimportstatements had been stripped at some point. It asserted six imports while performing none, and reported success for all six.Status
Fixed in PR #15166: the file goes 499 → 236 lines, 10 passed with 0 return-warnings, every original assertion preserved and now able to fail, roughly 30 forbidden
print()calls removed, andtest_migrated_files_importrewritten to driveimportlib.import_moduleand assert on the result. Unwrapping the swallow surfaced no hidden failure — the vacuousness was structural, not concealing a real break.This issue exists because the fix belongs to a PR about something else, and this deserves its own record.
Why it is worth an issue rather than a line in a changelog
This is the failure mode the whole wave-0 cluster is about, in its purest form: a suite reporting success while asserting nothing. It is the same class as #15182 (
assert Truein a test the marker run actually selects) and the same class as #15018 (a coverage guard whose enumeration matched half of nothing). The distinguishing feature here is that the tests looked thorough — they had real assertions inside them, and the swallow was three lines away.The open question is how many more there are.
pytest's return-not-None warning is emitted, not fatal, so it scrolls past in a green run.Acceptance criteria
try: … assert …; return True / except Exception: return Falseshape and report the count. Ten in one file is not evidence there is one file.returns a value — since pytest reports that as a warning and it is the cheapest possible detector for this class.passor onlyprint()while its name and output claim an assertion. That one cannot be caught by the return-warning and needs its own check.cache_consolidation_p4_test.pywas not in any exemption or skip list that would have suppressed the warning, and record the answer either way.Refs #14979, #15166, #15182, #15018.