Repository navigation
bug(test): 262 tests return a value instead of asserting, so they cannot fail #14920
Description
Activity
Partially addressed by #14929. 49 of the population fixed, not 262 — this issue stays open with the exact remainder below.
My measured numbers differ from the ones filed
AST sweep on
1208b77ba, attributing areturn <value>only to the test function that directly encloses it:filed here measured total 262 tests 327 value-returns in 174 functions, 53 files naive walk (for scale) 739 1870 autobot-frontend22 0 autobot-infrastructure3 134 returns The
autobot-frontendfigure is the interesting one. That tree has exactly one Python test file, and itstest_*functions are methods ofclass AutoBotComprehensiveFrontendTester— which does not matchpython_classes = Test*, so pytest never collects them. They are not tests that cannot fail; they are tests that never run. Same explanation for most of the infrastructure delta.Exclusions, and why each is earned
excluded returns basis test_*methods of non-Test*classes152 pytest never collects them. Converting these would edit code nothing executes — filed as #14927 (120 methods, 23 classes, 19 files) and it must be fixed first @pytest.fixture-decoratedtest_*functions11 a fixture is supposed to return; only its name is test-shaped generator tests ( yield)0 returnsets StopIteration's value, not a verdict. Branch is covered by a self-test rather than left unprovenbare return/return Nonen/a pytest does not warn on either What #14929 delivered
- 49 functions / 50 returns converted. 48 were
return True→ barereturn, applied only where no caller consumes the value; plusbackend_startup_e2e_test.py::test_imports, the exemplar named here, rewritten so the verdict is an assertion and the script half moved intomain(). repo_tests/tests_that_return_instead_of_asserting_test.py— the prevention this issue's AC4 asks for. Eleven clean trees pinned at zero by derivation (no list to go stale); three named trees carry ceilings keyed on the tree, never a filename, so a rename strands nothing. The ratchet refuses a collapsed scan before trusting any count, and offers no route to raise a number — stated in both docstring and failure message, since a returning test is never the right thing to write.
Why the sweep was not blanket-converted
29 of the 30 offending files in
autobot-backendhave anif __name__ == "__main__":block; 22 contain zero assertions. They are scripts named*_test.pywhose ownmain()calls eachtest_*function and folds the returned bools into an exit code. Soreturn Falseis usually load-bearing for the script's exit status, not a stray verdict — converting it toassert Falseis the honest fix and reddens CI for every one needing a live backend. That is per-file judgement, which is what this issue asked for.Worth recording: 54 functions looked safe to convert mechanically, and checking call sites showed 45 were called by their driver. Checking whether the call's value is actually consumed — a different question from whether a call exists — cut that to 6. Stopping at the first question would have broken 39 scripts for nothing.
Remainder, exact
277 value-returns in 125 test functions across 45 files —
autobot-backend136,autobot-infrastructure134,autobot-npu-worker7.filterwarnings = error::pytest.PytestReturnNotNoneWarningis verified working (pytest 9.0.2: passes without it, fails with it) but deliberately not added yet. It would immediately redden the 136 inautobot-backend, and it only ever sees tests that run — 141 of the 277 are in trees no pytest invocation collects, so it would report a clean tree with more than half the population outside its reach. The AST guard has no such limit; the config line lands whenautobot-backendreaches zero.Related
- fix(test): tests, suites and hooks that cannot report what they were written to report (#14923, #14920, #14917) #14929 — the PR
- bug(test): 120 test_* methods sit in non-Test* classes, so pytest collects none of them #14927 — the 152 excluded returns, and why they must be collected before being converted
- ci: pytest is unpinned while its plugins are pinned, so 277 return-value tests fail on a release nobody chose #14928 —
pytestis unpinned, so the day the warning becomes an error is chosen by a release, not by us
- 49 functions / 50 returns converted. 48 were
- added 10 commits that reference this issue
on Aug 24, 2026 Delivered and merged as
10d7be234(PR #14929).- CI: 51 success / 9 skipped / 0 failures, and — checked separately — all 10 required contexts present, not merely non-failing. A required check that never runs is absent from the rollup entirely, so a tally of present checks reads it as green; this PR was gated against that.
- Review: two independent
code-reviewerpasses, plus a focused re-review of the delta I authored myself.
The population was measured, not taken from the issue. #14920 filed 262; the real count is 327 returns across 174 functions in 53 files. The naive walk figure was 1870, not 739.
autobot-frontend's 22 turned out to be methods of a class that does not matchpython_classes— pytest never collects them, a different defect, filed separately as #14927.49 functions fixed, and that number is stated rather than rounded up to the headline. Exclusions with basis: 152 returns in non-
Test*classes (dead code — converting would edit what nothing executes), 11 fixtures, and 6 whosereturn Truea caller genuinely consumes.Review caught 9 conversions that broke their own drivers. Those drivers dispatch indirectly —
for name, fn in tests: result = await fn()— and sum truthiness, so a barereturnmaderesultNoneand every passing test reported failure. The consumed-check had matched literalfuncname(syntax and could not see a function referenced only as a bare name inside a tuple, so all 9 landed in the "uncalled, therefore safe" bucket. They now carry both a real assertion and a truthy return: the assertion so the test can fail, the return so the driver's verdict stays honest. Either alone is insufficient.That fix then exposed a flaw in the ratchet itself.
offending_returnscounted every value-return with no exemption for functions that also assert — so a correct fix was rejected by the guard the PR added. Refined to skip functions with their ownassertorraise, since the defect is returning instead of asserting. Driven on synthetic cases: return-only is an offence, assert-and-return and raise-and-return are not.The budget drop is documented honestly, because the obvious reading is wrong.
autobot-backendfell 136 → 78 while none of the nine fixes are in that tree. The entire movement is a tree-wide side effect of the exemption, which un-flags 39 pre-existing functions there and 4 inautobot-infrastructure. All 43 were checked individually — every one carries real assertions (1-24 apiece), so nothing vacuous is hidden. The comment in base now says exactly that.The ratchet refuses a collapsed sweep, failing with "Fix the sweep; do NOT lower these numbers" rather than inviting someone to lower the floor, and offers no route to raise a budget — verified in review to match the docstring's claim, unlike #14919 where a message promised a route the code never implemented.
filterwarnings=errorwas deliberately deferred, and review judged that correct: it only sees tests that run, and 141 of the 277 remaining returns are in trees pytest never collects. Enabling it would have reported clean with over half the population out of reach. The AST ratchet is the broader prevention.- added 5 commits that reference this issue
on Aug 24, 2026
Found while reading a CI log for PR #14916 (#14884's wiring fix). Pre-existing, unrelated to that branch — none of the affected files is touched by it.
The warning
Why this is not cosmetic
A test that ends in
return Falsestill passes. pytest ignores the value and emits a warning, so the function reads like a check and behaves like a no-op.backend_startup_e2e_test.py::test_importsis the clearest case: it is a script that prints progress and returns a bool from atry/except, and it passes whichever branch it takes — including the failure branch.This is the same class the #14884 / #14891 / #14909 cluster is about: a check that is absent reads exactly like a check that passed. These ones are worse than absent, because they are collected, counted and reported green.
pytest has signalled an intent to turn
PytestReturnNotNoneWarninginto an error, so this is also a forward-compatibility item — the day it flips, every one of these becomes a failure at once.Measured
Counted by AST on the current base, attributing a
return <value>only to the test function that directly encloses it (a naive walk descends into nested helpers and inflates the number to 739):autobot-backendautobot-frontendautobot-npu-workerautobot-infrastructureNone is in
autobot-infrastructure/shared/scripts/hooks/, the directory #14916 wires into CI, so that PR neither introduces nor inherits any of them. The 3 underautobot-infrastructure/sit outside the wired path and are covered by the collection work in #14917.What needs doing
Each one needs reading, not a blanket rewrite — the correct fix depends on what the return value meant:
asserttest_importsis one) gets renamed out of thetest_*namespace or moved behind amain()guardAcceptance criteria
filterwarnings = error::pytest.PytestReturnNotNoneWarninginpytest.ini, so the class cannot come backreturnof the wrong value and confirm the suite goes redRelated