test-guard: the returning-test sweep cannot see an assert neutralised by a bare except

Problem

repo_tests/tests_that_return_instead_of_asserting_test.py (#14920) skips any test
function that contains an assert or raise:

# The defect is returning *instead of* asserting — a test that cannot fail.
# A test that asserts AND returns can fail, and its return value is a
# separate contract: ...
if _own_nodes(function, (ast.Assert, ast.Raise)):
    continue

The reasoning is sound as stated, and the exclusion is right for the case it
describes. But it assumes an assert that is present is an assert that can
fire. That assumption fails for this shape:

def test_simple_cache_basic_operations():
    try:
        assert cache.get("k") == "v", "cache round-trip failed"
        return True
    except Exception as e:
        print(f"✗ failed: {e}")
        return False

A bare except Exception catches AssertionError. The assertion is inert: the
test cannot fail whichever way it goes. The sweep sees the assert node, concludes
the test can fail, and skips it — so the guard designed to find tests that cannot
fail is blind to a whole family of tests that cannot fail.

It is the worse variant, too. A plain return True at least trips
PytestReturnNotNoneWarning at runtime. This shape trips that warning and looks
defended in an AST sweep, so both detectors agree it is fine.

Measured

Found while fixing autobot-backend/cache/cache_consolidation_p4_test.py for #14979.
All ten of its test_* functions had this shape. The guard attributed only 2
offending returns in the whole file — both in test_migrated_files_import, the one
function with no assert at all (its body was pass plus six print() calls
claiming six modules import, with the import statements stripped at some point).

BASE offending in that file: 2  [('test_migrated_files_import', 288),
                                 ('test_migrated_files_import', 281)]
NOW  offending in that file: 0

So nine tests that could not fail sat inside the guarded population, counted as
compliant, for as long as the file has existed.

Suggested detection

An assert should only excuse a test if it can actually propagate. A cheap and
precise rule: do not count an assert as protective when it is lexically inside a
try whose handler catches Exception, BaseException, or AssertionError and
does not re-raise. That is a local AST check needing no execution, and it is
narrow enough not to disturb the legitimate assert-and-return drivers the current
exclusion exists to protect.

Worth pairing with a broader question the same shape raises: a bare
except Exception around assertions in any test_* function is almost always a bug,
whether or not the function returns.

Acceptance criteria

The sweep counts a test whose only assertions sit under a swallowing handler
The legitimate assert-and-return drivers the current exclusion protects are
still excluded — proved by a synthetic case in
test_the_detector_finds_a_planted_return_and_spares_the_legitimate_ones
The population is re-measured and the _KNOWN_OFFENDERS budgets adjusted in
the same commit (the number will rise as the blind spot closes — that is a
detector improvement, not a ratchet violation, and needs saying explicitly in
that file so the down-only rule is not read as forbidding it)
A guard, or an explicit note, for except Exception wrapping assertions in
test functions generally

bug(test): 262 tests return a value instead of asserting, so they cannot fail #14920 — the guard this extends
test(quality): ten tests that could not fail — bare 'except Exception' swallowing AssertionError, and one asserting imports it never performed #15189 — making the return-not-None detector fatal; this is the case that detector
also misses, since the warning fires but nothing acts on it
bug(test): 90 test_* methods sit in live-service validation drivers pytest cannot collect #14979 — where it was found; the ten tests are fixed there, the blind spot is not

refactor(repo_tests): lift the collected-test model out so the guard …

test-guard(repo_tests): an assert under a swallowing handler is not p…

PR #15241 (merged to Dev_new_gui) references this issue with a close keyword.

test-guard(repo_tests): an assert under a swallowing handler is not protection (#15195)

If this issue is fully resolved, close it manually. If work remains, no action is needed.

Verified in merged `Dev_new_gui` — closing

Merged as c5306fe85f (PR #15241).

Criterion	Verdict	Evidence
The sweep counts a test whose only assertions sit under a swallowing handler	MET	`swallowed_assertions_in` at `repo_tests/collected_test_model.py:307`; the assert/raise exemption is narrowed so the swallowed shape is counted too
Legitimate assert-and-return drivers are still spared, proved synthetically	MET	`tests_that_return_instead_of_asserting_test.py:497` — spares `assert True; return True` and a `try/except ValueError` wrapper (a non-assertion handler leaves the assert live), while flagging `except BaseException`, `except AssertionError`, bare `except:`, `except (ValueError, Exception)` and `except Exception as exc`
Population re-measured and the offender set adjusted in the same commit, with the rise stated as a detector improvement	MET	`:193-228` records "71 → 86 and 121 → 127 … A DELIBERATE RE-MEASUREMENT, NOT A RATCHET VIOLATION", enumerates all 21 newly-counted returns across 8 functions in 3 named files, and states the base set is a verified strict subset
A guard for `except Exception` wrapping assertions in tests generally	MET	`:427` `test_no_test_smothers_its_own_assertions_under_a_swallowing_handler` — catches the shape whether or not the function returns, with a down-only budget, a population floor, and drained-entry assertions

The third criterion is the one worth dwelling on. A count that rises because the detector got sharper looks identical, in a diff, to a ratchet being loosened — so stating it explicitly, enumerating every newly-counted site, and proving the old set is a strict subset is what separates the two. That record is why this closes rather than reading as a quietly raised budget.

The budgets have already walked down once since (autobot-backend 6 → 5, autobot-infrastructure deleted) in d4e4c428e5 — the ratchet working as designed.

Closing — all four acceptance criteria verified in `origin/Dev_new_gui`

Audit of open issues whose work landed via a merged commit (Closes never auto-fires on Dev_new_gui). Verified against the merged tree, not the PR diff. Delivered by c5306fe85f (PR #15241).

AC	Verdict	Evidence in `origin/Dev_new_gui`
1. The sweep counts a test whose only assertions sit under a swallowing handler	Met	`repo_tests/tests_that_return_instead_of_asserting_test.py:301` — the assert/raise exemption is now `if propagating_guards(function): continue`, so the exemption is spent only on assertions that can fire (rationale at `:295-300`). The predicate lives in `repo_tests/collected_test_model.py:276`, keyed on `ASSERTION_CATCHING = frozenset({"Exception", "BaseException", "AssertionError"})` at `:208` and the handler test at `:224`.
2. Legitimate assert-and-return drivers are still excluded, proved by a synthetic case	Met	`tests_that_return_instead_of_asserting_test.py:497` — `test_the_detector_finds_a_planted_return_and_spares_the_legitimate_ones` now carries both halves: a handler naming a non-assertion exception (`except ValueError`) leaves the assert able to fire and is spared (`:534-547`), and an assert under `except Exception` is counted (`:549-560`).
3. Population re-measured and `_KNOWN_OFFENDERS` budgets adjusted in the same commit, with the rise stated explicitly as a detector improvement	Met	Budgets at `:234-236` (`autobot-backend` 71 -> 86, `autobot-infrastructure` 121 -> 127, since revised to 121 by #15189). The note at `:193-232` names it "A DELIBERATE RE-MEASUREMENT, NOT A RATCHET VIOLATION, AND THE ONLY ONE THIS FILE SANCTIONS", enumerates all 21 newly-counted returns across the 3 files, and records that the base set is a strict subset of the new one. The terms on which a future raise is permissible are stated at `:230-232`.
4. A guard, or an explicit note, for `except Exception` wrapping assertions in test functions generally	Met — a guard, not a note	`tests_that_return_instead_of_asserting_test.py:427` — `test_no_test_smothers_its_own_assertions_under_a_swallowing_handler`, backed by `_swallowed_by_tree()` at `:311` and `collected_test_model.swallowed_assertions_in` at `:307`. It fires on any swallowed assertion in a test function, whether or not the function returns — the broader question the issue raised.

AC4 is met in its stronger form: the issue offered "a guard, or an explicit note" and the merged change is a guard with its own ceiling, so the general shape is enforced rather than described. The failure message at :464-467 names the remedy (catch the specific exception, or re-raise / pytest.fail).

Note for anyone reading the ratchet history: the autobot-infrastructure ceiling moved 121 -> 127 here and back to 121 under #15189, which unwrapped three swallowing drivers in autobot-infrastructure/shared/scripts/test_configuration.py. Both moves are recorded in the comment block above the dict.

Closing.

Navigation Menu

Repository navigation

test-guard: the returning-test sweep cannot see an assert neutralised by a bare except #15195

Problem

Measured

Suggested detection

Acceptance criteria

Related

Verified in merged `Dev_new_gui` — closing

Closing — all four acceptance criteria verified in `origin/Dev_new_gui`

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

test-guard: the returning-test sweep cannot see an assert neutralised by a bare except #15195

Description

Problem

Measured

Suggested detection

Acceptance criteria

Related

Activity

github-actions commented on Aug 28, 2026

mrveiss commented on Aug 29, 2026

Verified in merged Dev_new_gui — closing

mrveiss commented on Aug 29, 2026

Closing — all four acceptance criteria verified in origin/Dev_new_gui

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Verified in merged `Dev_new_gui` — closing

Closing — all four acceptance criteria verified in `origin/Dev_new_gui`