test(suite): convert the thirteen uncollected validation drivers and settle all three ceilings (#14979)

[mrveiss]

Refs #14979 — eleven of the thirteen files converted. Two were deliberately left unconverted; see "What is still unmet".

Thinking Path

#14927 landed the guard and the first tranche. #14979 is the remainder: 90 test_* methods in 14 classes across 14 files, every one of which collects zero items because pytest refuses a class defining __init__. A prior pass converted one file and moved one of three ceilings; this settles the rest.

The issue is emphatic that a bare rename is worse than nothing — SessionTakeoverTestSuite → TestSessionTakeoverSuite still collects zero while looking fixed — so each file was converted rather than renamed: __init__ replaced by setup_method or a fixture, every return True/return False replaced by an assertion naming what failed, the live dependency declared through require_live_endpoint so an absent service skips with a reason instead of failing on a refused socket, and a marker applied so the unit gate excludes anything needing a running stack. main(), the run_all_* drivers and every print() went with them.

The correction that shaped this PR. An earlier revision converted all thirteen and described three of them as "knowingly cosmetic", leaning on AC3's own escape clause. CI rejected that, and it was right to: repo_tests/marker_suite_root_coverage_test.py (#13286) fails any module carrying a marker ci.yml deselects that lives under no marker-tests.yml root, because such a test runs in no workflow at all. Marking an unrun file does not leave it where it was — it converts "a file that ran nowhere" into "a test that runs nowhere", which is strictly worse, because it now reads as coverage. Three files tripped it. Each got a decision on its merits rather than an exemption.

Two further corrections, both against my own earlier claims. I had reported that autobot-infrastructure/shared/tests was a dead CI root whose conftest could not import. That was wrong. Measured with the invocation the workflow actually uses:

$ pytest autobot-infrastructure/shared/tests libs -m "integration or slow or distributed or performance" --collect-only
24/41 tests collected (17 deselected)

The ImportError only appears when that directory is named alone: autobot-infrastructure/shared/config/ is a directory of YAML with no __init__.py, so as the sole root it resolves as a namespace package named config and shadows autobot-backend/config, which is what exports unified_config_manager. Adding a second root fixes resolution. So the two files under that tree are genuinely wired and running, not blocked, and #15161 has been corrected and retitled to the narrower defect that is actually there.

What Changed

Eleven files converted. Each collects exactly the number of methods the issue attributed to it, with no PytestCollectionWarning.

File	New class	Methods	Marker	Live probe	CI-reachable
autobot-backend/utils/hardware_metrics_test.py	TestHardwareMonitoring	10	integration, slow	none — in-process against the hardware_monitor / gpu_optimizer singletons	yes
autobot-backend/comprehensive_system_validation_test.py	TestAutoBotSystemValidation + TestSystemValidationLocalEnvironment	8	integration on the live class only	backend API	yes
autobot-backend/knowledge/chat_knowledge_system_e2e_test.py	TestChatKnowledgeSystem	7	integration	backend API	yes
autobot-backend/monitoring/monitoring_and_alerts_test.py	TestMonitoringAndAlerting	7	integration	backend API, exempting the one filesystem-only test	yes
autobot-backend/agents/multi_agent_workflow_validation_test.py	TestMultiAgentWorkflow	6	integration	backend API	yes
autobot-backend/npu_integration_e2e_test.py	TestNPUWorker	5	integration	NPU worker	yes
autobot-backend/async_baseline_performance_test.py	TestAsyncBaseline	4	performance	backend API	yes
autobot-backend/knowledge/knowledge_performance_test.py	TestKnowledgePerformance	4	performance	backend API	yes
autobot-infrastructure/shared/tests/integration/test_distributed_system_integration.py	TestDistributedSystem	6 (3 methods + 3 pre-existing module functions)	distributed	backend API, per-scenario	yes — 6 selected
autobot-infrastructure/shared/tests/performance/test_performance_optimization.py	TestPerformanceOptimization	5	performance	backend API, NPU worker	yes — 5 selected
autobot-frontend/tests/frontend_comprehensive_corrected_test.py	TestFrontendSurface	10	integration	frontend + backend	yes — newly wired, 10 selected

One tree wired in. autobot-frontend/tests is now a root on marker-tests.yml's third invocation. That directory holds exactly one tracked Python module — no conftest.py, no __init__.py — so it adds no collection surface of its own. It cannot ride the frontend workflow: that suite is npm/vitest and its path filter never fires on a .py file, so a Python invocation had to name it. It joined the existing third invocation rather than opening a fourth because a fourth would have needed --min-passed frontend=0, and marker_suite_root_coverage_test.py reads any name=0 floor as the claim "this tree carries no marker-selected test" — which is false here. Sharing the existing floors states the truth: the bucket collects more than it passes, and infra=10 still holds because skips do not reduce passes.

Selection is proven by a real CI run, not by inspection. Because this PR edits marker-tests.yml itself, that workflow's pull_request paths filter matches and it ran on this branch — run 33151328667, conclusion success:

Invocation	Collected	Executed	Passed	Failed	Errors	Skipped	Collected floor	Passed floor
backend	134	43	43	0	0	91	1	34
slm	0	0	0	0	0	0	0 (declared)	0 (declared)
infra	34	10	10	0	0	24	1	10 (declared)
total	168	53	53	0	0	115

The infra invocation went from 12 collected to 34 — the 22 added are this PR's frontend_comprehensive_corrected_test.py (10), test_distributed_system_integration.py (6) and test_performance_optimization.py (5), plus one from population drift. Matching per-file counts from the same root list locally: frontend 10, distributed 6, performance 5.

Two things that table settles, which no amount of local checking could:

• The backend invocation absorbed this PR's 51 newly-marked tests with 0 failed and 0 errors (134 collected, 43 passed, 91 skipped). The conversions skip cleanly where their services are absent rather than erroring on a refused socket, which is the behaviour require_live_endpoint exists to give.
• --min-passed infra=10 still holds — but with exactly zero margin. All 21 tests this PR added to that bucket skipped, so passed stayed at 10 against a floor of 10. That is the correct outcome (no live stack in CI), and it is stated plainly rather than left for someone to discover: the floor is satisfied, not comfortably.

Both aiohttp and websockets, which the frontend module imports, were already installed by that job (requirements-ci/networking.txt:3, requirements-ci/framework.txt:8) — confirmed before pushing, so the wiring could not introduce a collection error.

One marker registration fixed. autobot-infrastructure/shared/tests/pytest.ini registered integration, slow and distributed but not performance, so under --strict-markers the converted performance module was a collection error for any invocation rooted in that directory. Root cause fixed, not worked around.

Ten tests that were structurally incapable of failing

The python-suite shard log carried ten PytestReturnNotNoneWarnings from autobot-backend/cache/cache_consolidation_p4_test.py, a file this PR did not otherwise touch. The warning was the least of it. Every one of its ten test_* functions had this shape:

def test_imports():
    print("=" * 70)                      # noqa: print
    try:
        from utils.advanced_cache_manager import SimpleCacheManager, advanced_cache, cache_manager
        assert advanced_cache is not None, "advanced_cache instance missing"
        assert isinstance(cache_manager, SimpleCacheManager), "cache_manager should be SimpleCacheManager instance"
        return True
    except Exception as e:
        print(f"✗ Import test failed: {e}")   # noqa: print
        return False

The bare except Exception catches AssertionError. Every assertion in the file was therefore decorative: a failing assert was caught, printed, and converted into return False — which pytest reports as a pass with a warning. Ten tests, none of which could fail, in the same suite as a PR about making tests real.

Worse, one of them asserted a fact it did not check. test_migrated_files_import claimed six modules "import successfully"; its body was pass plus six print() calls saying so. The import statements had been stripped at some earlier point, leaving a function that performed no import and made no assertion — a coverage claim for six modules, backed by nothing.

Fixed rather than deferred:

	before	after
lines	499	236
tests collected	10	10
tests that can fail	0	10
PytestReturnNotNoneWarning	10	0
print() calls	~30	0
except Exception swallows	10	0

Every original assert and its message was preserved; the try/except wrapper and the return True/return False were removed so failures propagate. test_migrated_files_import now drives importlib.import_module over a named MIGRATED_MODULES tuple and asserts on each — the only place a check was added rather than unwrapped. Removing the swallows exposed no hidden failure: all ten pass against the current advanced_cache_manager, so the vacuousness was structural rather than concealing a live defect.

What let it survive — and it was not simple neglect. I first reported that nothing hid this file. That was half right, and the other half is the more useful finding.

No exemption list contains it: checked against python_file_size_known_large.py, python_file_size_ratchet_baseline.py, sys_modules_leak_baseline.txt, _KNOWN_OFFENDERS, .flake8, .bandit and the workflows. The only other mention repo-wide is a stale mapping at scripts/migrate_tests.py:161 pointing at autobot-user-backend/cache — a directory in no commit, the same phantom tree the shared/tests conftest puts on sys.path. Reported for filing, not fixed here; it is one of 39 tracked references to that removed tree.

But the repository does have 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 did not catch these ten, and the reason is structural:

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

The exclusion assumes an assert that is present is an assert that can fire. Under except Exception it cannot. Nine of the ten functions had asserts — inert ones — so the sweep passed over them. Measured against the base file, it attributed only 2 offending returns in the whole module, both in test_migrated_files_import, the single function with no assert at all:

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

So the shape was worse than "advisory warning nobody acted on": it tripped PytestReturnNotNoneWarning at runtime and read as defended to the AST guard. Filed as #15195, with a suggested rule — do not treat an assert as protective when it sits under a handler catching Exception/BaseException/AssertionError without re-raising. Not fixed here.

Consequently this PR also lowers that guard's budget, autobot-backend 73 → 71, in the same commit that removed the returns — the ratchet fails below its budget as well as above. Only 2 of the 10 move the number, for the reason above, and that is recorded at the constant so the arithmetic is not mistaken for a miscount.

Six size-ceiling entries deleted, not lowered. The conversions replaced driver scaffolding with tests, pushing six previously-grandfathered files back under 600 lines. An entry naming a compliant file exempts nothing while looking authoritative, so each was deleted from both scripts/python_file_size_known_large.py and repo_tests/python_file_size_ratchet_baseline.py:

File	Old ceiling	Now
autobot-backend/monitoring/monitoring_and_alerts_test.py	1192	308
autobot-frontend/tests/frontend_comprehensive_corrected_test.py	1009	344
autobot-backend/knowledge/knowledge_performance_test.py	852	328
autobot-backend/comprehensive_system_validation_test.py	777	238
autobot-backend/utils/hardware_metrics_test.py	714	268
autobot-backend/async_baseline_performance_test.py	623	305

That was caught by CI, not by me. I had checked that no changed file exceeded 600 and never checked the inverse, even though the same both-directions property is the whole point of the ceilings this PR moves.

Ceilings, before and after

Tree	Before (ceiling, floor)	After	Why
autobot-backend	(59, 18000)	(8, 18000)	51 methods across 8 files collected. The 8 remaining are not from #14979's table — other classes in the tree the issue never named, now pinned exactly.
autobot-infrastructure	(19, 250)	(11, 250)	The two shared/tests files converted (11 methods). The 11 left are the two shared/scripts files, deliberately unconverted.
autobot-frontend	(10, 10)	entry deleted	Drained to zero; the ratchet requires a drained entry be deleted so the tree is pinned at zero by derivation.

Measured, not assumed: _offenders_by_tree() on the final tree returns {'autobot-backend': 8, 'autobot-infrastructure': 11}.

Verification

Criterion	Kind	Evidence
AC1 — each file converted individually, probe-declared, asserting	static / unit	Per-file table. All 11 collect their exact expected count, warn=0 err=0, from a per-file --collect-only sweep.
AC2 — each conversion lowers the matching ceiling in the same commit	unit	repo_tests/test_methods_in_uncollected_classes_test.py passes with the three values above; the guard fails if any ceiling does not match.
AC3 — trees wired in, or knowingly cosmetic	partially unmet	Frontend wired; shared/tests already covered; two shared/scripts files left unconverted rather than made cosmetic. See below.
AC4 — each conversion mutation-proved	unit	Two contrast mutations below, both against the final tree.
#13286 root coverage	unit	repo_tests/marker_suite_root_coverage_test.py + ratchet + hook_suites_run_in_ci_test.py → 25 passed. Every marker-carrying module this PR touches is under a marker-tests.yml root.
Unit gate not polluted	unit	Unit-gate marker selection over the converted backend files: 51 collected, 49 deselected, 2 selected, 2 passed.
Size ratchet	static	check_python_file_size.py --audit-ceilings → 4910 scanned, 502 grandfathered, all live and at size.
env_registry.py untouched	static	git diff origin/Dev_new_gui -- autobot_shared/env_registry.py empty; still exactly 1743 lines. All 36 AUTOBOT_* names the conversions introduced were reverted to plain module constants.
Formatting parity	static	black --line-length=120 and isort --settings-path=. clean over every changed file under autobot-backend/ and autobot_shared/.
No print() introduced	static	Zero in every changed file; 30-odd removed from cache_consolidation_p4_test.py.
Return-instead-of-assert warnings cleared	unit	cache_consolidation_p4_test.py: 10 passed, 0 PytestReturnNotNoneWarning (was 10 warnings, 10 tests that could not fail).

Contrast mutation 1 — reintroduce the defect

Added an __init__ back to TestChatKnowledgeSystem; its 7 methods stop being collected and become offenders again:

AssertionError: these trees gained a test_* method in a class pytest cannot collect
(actual, budget): {'autobot-backend': (15, 8)}. The budgets are ceilings and there is
no route to raise one (#14927).
FAILED ...::test_the_known_offender_budgets_only_ever_shrink
1 failed, 5 passed

Contrast mutation 2 — empty the enumeration

_test_modules() forced to return [], proving a sweep that finds nothing goes red rather than reporting every tree clean:

AssertionError: only 0 modules match pytest's python_files ['test_*.py', '*_test.py']
— expected at least 1800. The sweep has stopped matching and would call every tree clean.
AssertionError: the sweep no longer finds the tests it is supposed to be scanning
(found, floor): {'autobot-backend': (0, 18000)}. ... Fix the sweep; do NOT lower these
numbers to match it.
4 failed

Both reverted and the tree re-verified before commit.

Read the skip counts correctly

Every converted file is marker-gated and probe-guarded, so in CI — where no stack runs — they skip. "7 skipped" is exactly the shape that reads as "7 passing". What landed is that 72 methods which previously collected zero items and asserted nothing are now collected, marker-selected, and assert the moment a stack is present. It is not 72 passing tests in CI.

Against the stack on the build host, chat_knowledge_system_e2e_test.py is 1 passed / 6 failed. Those six are not conversion damage — all six are the one backend defect filed as #15160. The tests are right and the product is broken.

What is still unmet

Two files were deliberately NOT converted, and #14979 stays open for them:

• autobot-infrastructure/shared/scripts/utilities/test_autobot_functionality.py (8 methods)
• autobot-infrastructure/shared/scripts/analysis/test_npu_worker.py (3 methods)

They live under shared/scripts, which no pytest invocation names. Converting them would have produced marked tests that run in no workflow — the #13286 failure above, and the outcome that reads as coverage while being none. Wiring that tree in means taking on ~35 unvetted ad-hoc scripts under analysis/ and double-running the 18 hooks/ tests the unit gate already covers: a separate decision with its own blast radius, which marker-tests.yml's own comment says to file rather than smuggle in. Tracked by #15178. They are pinned at 11 in the infrastructure ceiling, so they cannot grow.

Nothing here is host evidence. Nothing on this branch is deployed. Tests executed against the build host ran against a different revision, so those results are reported as findings only, never as verification of this branch.

Defects found and filed rather than fixed here:

• perf(api): /api/knowledge_base/search stops answering under concurrent load — hidden for the life of the file by a swallowed timeout #15165 — POST /api/knowledge_base/search stops answering under concurrent load (~0.3s idle; no response at 25s, 30s, >70s). test_agent_task_completion has driven it since it was written, inside a bare try that logged a warning in a class collecting zero items. Removing the swallow caught it on the first pre-push run. Its budget is now 180s with the reason and the issue recorded at the constant, and the ReadTimeout is re-raised as a named AssertionError.
• bug(api): chat_knowledge module global is never assigned — six routes 500 on every call #15160 — chat_knowledge.py:500 declares chat_knowledge_manager = None and nothing assigns it; six routes 500 on every call while the health probe reads the same dead global and reports idle.
• ci: autobot-infrastructure/shared/tests cannot be collected on its own — a namespace-package config shadows the backend's #15161 — corrected and retitled; the real defect is that shared/tests cannot be collected alone because a namespace-package config shadows the backend's.
• tech-debt(ci): file-size ceilings recorded at current size guarantee the next commit violates them — 9 files now over #14762 — three zero-headroom ratchet instances, including the env_registry.py case that forced the env-var revert.

Model Used

Claude Opus 5 (1M context).

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[mrveiss]

Blocked on #15055 — root cause, and why this PR is not the cause

Labelling blocked rather than merging or working around it, per the standing rule that a red check is root-caused and never merged past.

The failing check is marker-tests step 7, and the failure is a wall-clock assertion in a file this PR does not touch:

AssertionError: Multimodal processor startup too slow: 538.9466285705566ms
FAILED autobot-backend/system_benchmarks_performance_test.py::TestSystemPerformanceBenchmarks::test_system_startup_performance
1 failed, 42 passed, 91 skipped

system_benchmarks_performance_test.py:311 asserts processor_startup_time < 500.0.

Evidence that it is load, not this branch:

run	head	step 7
33151328667	591c0a080	43 passed, 0 failed
33154304714	90d88bb60	42 passed, 1 failed

The entire diff between those two commits is repo_tests/tests_that_return_instead_of_asserting_test.py — a file that carries no marker, so this very run deselects it. system_benchmarks_performance_test.py is untouched on this branch. One test flipped pass→fail with no reachable code change.

Third data point against the same constant: passed, then 547 ms, then 538.9 ms. Both failures within ~10% of budget — a threshold set near the runner's median rather than above its worst case. This machine is also the CI runner and routinely has several suites running at once, so a loaded runner is the normal condition, not an anomaly.

Why this PR is the first to hit it on a pull request. marker-tests.yml's header comment states it is not a pull_request workflow; the trigger block twelve lines below says otherwise, paths-filtered to that file (tracked as #15183). This PR edits marker-tests.yml to wire the frontend tree in, so the filter matches and the workflow runs — inheriting the flake as a red check. Until #15055 lands, any PR touching that workflow file will look broken for a reason that has nothing to do with it.

Not fixed here, deliberately. Raising or removing the 500 ms constant is #15055's call, in a file outside this PR's scope, and doing it under a #14979 commit would be the "raise a ceiling to reach a fix" move this work has avoided throughout. An agent is on #15055 now with a bar that the reworked assertion must still fail on a genuine regression.

Everything this PR owns is green. Step 8 — its own invocation — passed again at 10 passed / 24 skipped with every floor holding, and the coverage report step succeeded. The 12 python-suite shards are queued with none failed.

Unblocks when #15055 merges. This PR then needs a base update — already verified as a clean merge, with the two co-modified ratchet files touching disjoint keys.