Skip to content

fix(ci): triage every failure and error in the marker-excluded suite, and make it report an executed count (#14930) - #14933

Merged
mrveiss merged 10 commits into
Dev_new_guifrom
issue-14930-marker-suite
Aug 24, 2026
Merged

mrveiss merged 10 commits into
Dev_new_guifrom
issue-14930-marker-suite

Conversation

@mrveiss

@mrveiss mrveiss commented Aug 24, 2026 •

Copy link
Copy Markdown
Owner

Closes #14930

Thinking Path

The issue's corrected framing was right: the suite partially works. 35 marker-carrying
tests execute and pass. The question was what the other 38 results actually are, and
the answer is that 37 of 38 are one defect class in two disguises — a live service
the runner does not have, reported as a hard result instead of a stated non-result.

The 19 errors are not 19 problems. They are one file:
autobot-backend/services/redis_service_api_integration_test.py. It has no
backend_url fixture anywhere on its conftest chain — the only definition lives in
autobot-infrastructure/shared/tests/conftest.py, which this tree never reaches — so
all 19 error at setup. Nothing else in the run raised that error.

The 19 failures are three causes, not one:

Count Where Cause
6 api/infrastructure_integration_test.py ConnectionRefused dialling the backend on :8001
10 api/voice_integration_test.py ConnectionRefused dialling :8001 and the TTS worker on :8083
2 knowledge/gpu_kb_integration_test.py, knowledge/kb_optimization_test.py RuntimeError: Failed to initialize knowledge base
1 performance_benchmarks_test.py AssertionError: Memory usage too high — 1644.55 < 500

The 16 connection failures are the same shape as the 19 errors: real HTTP to services
that do not exist on a GitHub-hosted runner.

The two knowledge-base failures looked like the same thing and are not. The workflow
provides a healthy Redis specifically for these, and its header says so. They fail
anyway because autobot-backend/conftest.py replaces autobot_shared.redis_client
with a socket-free stand-in whose get_redis_client() returns None for every
item under autobot-backend/, gated on directory membership and not on markers. So
the marker suite provisions a Redis that no backend test can reach — a service
configured, health-checked, and structurally unconsumable. That is filed as #14932
with the root cause and a proposed fix, and deliberately not fixed here: the change
flips Redis behaviour for every backend integration test at once, and 35 of them
currently pass. Landing it alongside the guard work would make the resulting count
unattributable.

The one genuine assertion defect is the benchmark. It asserted absolute process
RSS
under 500 MB, but a pytest-xdist worker sits at ~1.6 GB from importing the
orchestrator, knowledge base, memory manager and LLM service before the first
benchmark runs. The budget was spent before the test started: it could not pass, and
it could not have caught a regression in the code it names.

On the decision the issue asks for — provide the fixture, or deselect. Both, and
they are not alternatives. The tests are well-formed and should run, so
backend_url is defined rather than the module being marked out; but every request
goes over the wire through a real httpx.AsyncClient, so a live backend is a genuine
precondition. knowledge_api_integration_test.py is the contrast that settles it:
it patches aiohttp.ClientSession.get, needs only a syntactically valid URL string,
and correctly just defines the fixture.

Nothing here narrows what the suite selects. Two tests assert that, on both the
scheduled expression and the dispatch default.

What Changed

autobot_shared/live_service_probe.py (new) — the canonical precondition probe.
endpoint_is_listening() opens a TCP socket and catches OSError and nothing wider,
so "the service is absent" skips while a connected-but-wrong service, a malformed
target, or a programming error still fails. redis_client_is_stubbed() detects the
backend stand-in structurally, by the absence of __file__ on an in-memory
ModuleType, so the knowledge-base skips clear themselves the moment #14932 lands
rather than being permanent by construction.

Four test modules now report absent services as skips naming the missing service,
via autouse fixtures — so a deselected test on the PR gate pays nothing. The
knowledge-base guards are scoped to integration-marked items only: both files pair
the marked test with a hermetic one that runs on the PR gate, and guarding those too
would have silently disabled a passing gate test.

performance_benchmarks_test.py measures the RSS delta across the measured
region. 500 MB of growth from one mocked chat() call is a real leak; 500 MB of
absolute RSS is AutoBot being imported.

pipeline-scripts/marker_suite_report.py (new) + the workflow's new Report
marker coverage
step. Both pytest invocations now write JUnit XML, and the script
aggregates collected / executed / passed / failed / errors / skipped into the run
summary and fails the job when the selection collected nothing. A missing report
file is a hard failure inside the script, never a zero — a pytest that died before
writing its XML must not read as "collected nothing", let alone as clean. The step
runs on !cancelled() so a red pytest cannot hide the coverage count.

The || [ $? -eq 5 ] lines stay, but no longer decide anything: they exist so the
second invocation still runs and the report step still receives both files. Exit 5 is
now caught by the floor instead of being accepted.

Header. The file's triage record is written into it, and the stale "one observed
run is RED (11 failed, 7 passed, 40 errors)" paragraph is corrected.

Verification

Before, run 32702629146 on PR #14929's head 4d050db81:

19 failed, 35 passed, 12 skipped, 19 errors in 695.40s

After, run 32713266629 on head b3096ed86 — the workflow is green, the first
green run in its history (every prior run, on any branch, was red):

35 passed, 49 skipped in 697.15s

and its own new report step, on the run summary page:

| Invocation | Collected | Executed | Passed | Failed | Errors | Skipped |
| backend    |        84 |       35 |     35 |      0 |      0 |      49 |
| slm        |         0 |        0 |      0 |      0 |      0 |       0 |
| **total**  |    **84** |   **35** | **35** |  **0** |  **0** |  **49** |
before after
failed 19 0
errors 19 0
passed 35 35
skipped 12 49
collected 85 84

Every one of the 38 failures and errors became a skip with a stated reason, counted by
the new report step. From the run's own summary table and the uploaded JUnit artifact:

Skip reason Count
the AutoBot backend API is not listening on 127.0.0.1:8001 36
Chromium is not downloaded for Playwright (pre-existing) 10
the knowledge base needs a real Redis client … tracked as #14932 2
collection skipped (pre-existing) 1
AUTOBOT_SUMMARIZATION_PROVIDER is not set (pre-existing) 1

(Counts above are from the intermediate run 32709235066; the final run's 49 skips are
the same set less the over-reach fixed below.) 19 + 6 + 10 + 2 = 37 former
failures/errors became skips. The 12 pre-existing skips are unchanged.
TestLLMServicePerformance::test_llm_response_time_benchmark now passes — the RSS
delta fix — confirmed test-by-test in the artifact.

The two count deltas, both explained rather than waved at:

85 → 84 collected. Not a lost test. The "before" run was on #14929's head, which adds
tools scripts pipeline-scripts autobot-infrastructure/shared/scripts/hooks to the
pytest roots; this branch is on base, which has four roots. #14929's own comment names
the extra test: "scripts/check_script_exec_bits_test.py's @pytest.mark.integration
case was selected by NO workflow at all."
Nothing here removes a root, and that test
returns to the selection when #14929 lands.

35 → 34 passed — a real over-reach in this PR, found and fixed. The infrastructure
guard was module-wide, but only 6 of that module's 7 tests dial the backend;
test_celery_worker_status reads logs/celery-worker.log off disk and was passing. The
7-skips-vs-6-failures mismatch is what exposed it. It is now exempted by an explicit
constant that self-checks: the fixture asserts every exempted name still resolves in
the module, so a rename fails loudly instead of stranding the exemption silently. An AST
check confirms the exemption set equals exactly the tests containing no requests. call.
With that fix the measured figures are 35 passed, 49 skipped, 0 failed, 0 errors
(run 32713266629) — a net +1 pass over base, from the benchmark repair.

Full PR CI, deduped to the latest entry per check name: 50 SUCCESS, 1 SUCCESS
(StatusContext), 9 SKIPPED, 0 failing, 0 cancelled, 0 pending. mergeStateStatus
is BLOCKED on REVIEW_REQUIRED — the ruleset review gate, not a check.

One further defect found and filed, not fixed: #14935. Run 32710575293 went red on
test_system_startup_performance at 537.86ms against a 500.0ms budget, while the
run before and after it passed — a boundary flake on runner contention. Underneath it is
a real bug: every MultiModalProcessor() logs
Failed to load vision models: CLIPModel.__init__() got an unexpected keyword argument 'resume_download'
and continues, so no vision model loads at all and the benchmark is largely timing a
failing transformers call. Pre-existing (it passed by luck in the "before" run) and
unrelated to this PR's subject, but it will redden this suite about half the time, so it
must be fixed before the cron in #13286 is enabled. Relaxing the budget to clear the
board is explicitly rejected there.

Mutation pairs. Seven mutations were applied to the workflow, each verified
present in the source before mutating so a missing target could not report a false
pass. All seven were caught:

CAUGHT  delete the report step entirely        -> invokes=False
CAUGHT  workflow calls a renamed script        -> script_exists=False
CAUGHT  script moved on disk, workflow stale   -> script_exists=False
CAUGHT  junit report nothing checks            -> all_reports_checked=False
CAUGHT  narrow MARKER_EXPRESSION               -> expr_not_narrowed=False
CAUGHT  narrow the dispatch default            -> dispatch_not_narrowed=False
CAUGHT  skip the report on failure             -> runs_on_failure=False

Two of those exist because the exercise found real holes in this PR's own tests:

  • The rename check was keyed on the filename marker_suite_report.py, so renaming the
    script in the workflow left no token matching and the loop passed vacuously. It
    now asserts that every .py path the workflow references exists, which catches the
    rename from either end.
  • Narrowing the dispatch default slipped past entirely: MARKER_EXPRESSION is
    inputs.markers || <default>, so two strings decide what runs and only one was
    pinned.

Two regressions this PR introduced and fixed before merge, both caught by CI rather
than assumed away:

  • marker_suite_report_test.py failed collection on all 12 python-suite shards.
    On Python 3.14 @dataclass resolves string annotations through
    sys.modules[cls.__module__], which is None for a module executed without a
    sys.modules entry — the loader pattern this repo uses to avoid the sys.modules leak
    guard. Fixed at the source by dropping from __future__ import annotations from the
    script, plus a register/finally-pop loader so re-adding it cannot resurrect the
    error. Verified against a scratchpad copy, including that the loader leaves no entry.
  • AUTOBOT_LIVE_PROBE_TIMEOUT_SECONDS was unregistered. Added to
    autobot_shared/env_registry.py with its range, and the autogenerated table in
    docs/developer/CLAUDE_RULES.md regenerated — verified by rebuilding the whole
    147-row table from the registry via AST and diffing it against the committed section.

Presence assertions. pipeline-scripts/marker_suite_report_test.py feeds the
checker the empty and absent cases and asserts it fails: zero collected, an all-skipped
run, a missing report file, malformed XML, a <testsuite> missing an attribute, and
--min-collected 0 (rejected at the argument parser — the presence assertion is not
disableable from the call site). autobot_shared/live_service_probe_test.py pins the
probe's boundary: a non-OSError from create_connection propagates rather than
reading as "absent", and an unparseable target raises instead of skipping a live suite.

No code from the codebase was executed locally; the mutation and wiring checks above
read YAML and re-implement the assertions rather than importing the repo.

Residuals, filed not dropped: #14932 — the conftest Redis stand-in, with the root
cause, the proposed one-line window-4 fix, and a note that the marker run also lacks
config/redis-databases.yaml so named databases never resolve; and #14935 above.
Closes names only #14930, which is fully delivered.

Note for #14929: it also edits marker-tests.yml (appending pytest roots) and will
conflict with the restructured run steps here. Its roots re-apply cleanly onto the new
step bodies; nothing in this PR removes them.

Model Used

Claude Opus 5 (1M context)

@github-actions

Copy link
Copy Markdown
Contributor

✅ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant