Repository navigation
test(quality): test_redis_connectivity asserts True — it cannot fail, and it is 1 of only 8 tests the marker run selects there #15182
Description
Found while fixing #15161. Reported by the agent that delivered PR #15175, which deliberately did not change it — repairing an assertion is a different decision from repairing collection.
What
autobot-infrastructure/shared/tests/integration/test_architecture_compliance.py:170 — test_redis_connectivity calls client.ping() and then asserts:
assert Truewith a pytest.skip on ConnectionError.
There is no outcome in which this test fails. With the Redis service the marker workflow provides, it passes while asserting nothing. Without it, it skips. Either way the tile is green.
Why it matters here specifically
This test is one of only 8 that the marker run actually selects from this tree. It is therefore a meaningful fraction of the signal that directory appears to provide — and one eighth of that signal is a constant.
The wider point is the one #15018 and #15161 keep landing on: a green result that cannot go red is worse than no test, because it occupies the place where coverage would otherwise be noticed as missing. ping() genuinely does assert something — it raises on a broken connection — but the assert True states nothing about what a correct connection looks like, and the skip converts the one real failure mode into a non-result.
Acceptance criteria
- Decide what
test_redis_connectivityis for: that a connection can be established, that it uses the SSOT-configured parameters, or that the pool behaves. Then assert that, specifically. -
assert Trueis gone. If the intent really is only "ping did not raise", the test asserts on the ping result rather than on a literal. - Reconsider the
pytest.skiponConnectionError. In a workflow that provisions Redis, an unreachable Redis is a failure, not a reason to skip — a skip there hides exactly the condition the test exists to detect. If the skip is kept, it is conditional on the service genuinely being optional, and that is recorded. - The contrast mutation: point the client at a port nothing listens on and show the test fails rather than skipping.
- Sweep this tree for other
assert True/ assertion-free tests and record the count.
Activity
fix(test): make four SSOT tests run and give Redis connectivity a rea…
mrveiss commentedon Aug 28, 2026
PR #15249 (targets
Dev_new_gui).assert Trueis gone and so is the skip. The test now asserts thatget_redis_client()returns a client at all, that the pool's host and port are the onesunified_config_managerdeclares, that itsdbis the numberredis-databases.yamlallocates tomain(the mapping #15181 guards), thatping()returnsTrue, and that a set/get/delete round-trip returns what was written.Skip removed rather than made conditional:
marker-tests.ymlis the only workflow selectingintegrationand it provisionsredis:7-alpineas a health-gated service container, so Redis is not optional where this test runs. Theexceptis kept only to turn a transport error into a namedpytest.failinstead of a bare traceback.Contrast mutation — client pointed at a port nothing listens on:
1 failed,AssertionError: get_redis_client(database='main') returned None — Redis is disabled, or the connection failed inside the canonical accessor. Fails, does not skip. Reverted from a pristine copy,diff -qclean, live-Redis run green.One thing the mutation showed that the issue did not predict:
get_redis_client()catchesConnectionErrorinternally and returnsNone, so the oldexcept redis.ConnectionErrorat this call site was unreachable. Against a dead port the old test did not skip — it raisedAttributeError: 'NoneType' object has no attribute 'ping'. The skip had been dead code the whole time, and the only non-pass outcome was an uninformative traceback. That is whyclient is not Noneis asserted first.Sweep: of the 36 tests pytest collects from this tree, 1 could not fail by assertion before (this one), 0 after. A further 13
test_*-named functions in the tree have no assertion but are collected by nothing — 10 incomprehensive_test_suite.py, 2 inresults/comprehensive_testing_20250909_150149/system_assessment_report.py, plusconftest.py:104 test_data_dir, which is a fixture named like a test. Those filenames match nopython_filespattern; that population is #15178's, not this PR's.Note on the framing: #15182 put this at 1 of 8 tests the marker run selects here. Re-measured on this branch the selection is 19 (16 passed, 3 skipped, 17 deselected of 36), #15161 having restored the collection the earlier figure was taken under.
test(repo): an unparseable test module is an unknown, not a clean swe…
fix(test): make four SSOT tests run and give Redis connectivity a rea…
mrveiss commentedon Aug 28, 2026
Verified in merged
Dev_new_gui— closingPR #15249 merged as
d8d45edd9.Closesdoes not fire on this branch, so closing by hand against code read from the merged base.#15181 — four tests that had never executed
autobot-infrastructure/shared/tests/test_redis_db_ssot.pynow anchors atparents[1], with the reason recorded in the file. Before: 4 collected, 0 executed —4 errors, eachAssertionError: SSOT file not found. After: 4 collected, 4 executed, 4 passed, against 15 real database entries.What they turn out to assert, now that they run:
_FALLBACK_MAPPINGmatches the YAML exactly; every YAML name appears inDATABASE_MAPPINGat the same number;DATABASE_MAPPINGinvents no name absent from the YAML and not a declared alias; eachRedisDatabaseenum member's value matches the YAML. No real failure was hiding behind the broken anchor — worth stating, because it was the open question.One latent vacuity fixed alongside:
_load_yaml_mappingreturned{}if theredis_databaseskey were ever renamed, and all four tests iterate that mapping — four passes waiting to become vacuous. It now asserts non-empty.#15182 — a test that could not fail
test_redis_connectivitywasclient.ping()followed byassert True, withpytest.skiponConnectionError. It now asserts client-not-None, pool host and port againstunified_config_manager, pooldbagainst the YAML allocation,ping() is True, and a set/get/delete round-trip on a UUID-namespaced key insidetry/finally.The skip turned out to be dead code, which is worse than the issue described:
get_redis_client()catchesConnectionErrorinternally and returnsNone, so the call site'sexcept redis.ConnectionErrorwas unreachable. Against a dead port the old test did not skip — it raisedAttributeError: 'NoneType' object has no attribute 'ping'. Its only non-pass outcome was an uninformative traceback, which is whyclient is not Noneis now asserted first.Removing the skip is safe, traced across four workflow files rather than asserted:
ci.ymlfiltersintegrationout of both invocations,coverage.ymlandtest-durations.ymlfilter it and do not name that tree, andmarker-tests.yml— the only selector — provisions a health-gatedredis:7-alpinewith--health-retries 10before any step runs.The guard, and a correction it earned
repo_tests/test_module_path_anchors_15181_test.pysweeps 111 anchored data-file expressions across the repository, reaching modules by parsing rather than importing so it sees files behind collection errors.MIN_SWEPT_EXPRESSIONS = 100;KNOWN_UNRESOLVEDholds one grandfathered entry, down-only in both directions.An independent review found the first draft reproducing the very defect it cites —
_file_anchorsswallowedSyntaxErrorand swept an unparseable module clean. Now it raises, withKNOWN_UNPARSEABLEdeliberately empty and a test that fails if it is not: 0 unreadable of 1,995 modules, so failing loudly costs nothing today.Adding
.worktreesto the skip list then exposed a latent bug: parts were matched against the absolute path, so any checkout under a directory namedbuild,venv,distor.worktreesswept zero. The population floor caught it on the first run — firing for real rather than hypothetically.Sweeps
111 anchored expressions, 1 unresolved after the fix (0 within this tree, 1 before). Of the 36 tests this tree collects, 1 could not fail by assertion before, 0 after. A further 13
test_*functions there have no assertion and are collected by nothing — recorded on #15178, not here.One figure corrected: this tree was described as having 8 marker-selected tests. Re-measured on the branch, it is 19 (16 passed, 3 skipped, 17 deselected of 36) — #15161's conftest fix restored collection the earlier count was taken under.