Replies: 15 comments
|
We already have BAT testing / UAT, this sounds like something that should be a part of BAT testing and not necessarily part of e2e. BAT/UAT are run by users/devs individually, I believe we currently have 13 "stories" set up. |
|
Thanks for the thorough write-up. A few thoughts. ha-mcp serves a single household. The LLM client is the only caller β there's no adversarial traffic, no untrusted input, no multi-tenant contention. The threat model doesn't justify a concurrency torture suite or injection fuzzing harness. Most of this proposal is input validation. The archetype taxonomy and torture dimensions are a useful way to think about it, but the actual test mechanics are straightforward: parametrize bad inputs, verify structured error responses. That fits naturally as an extension of our existing E2E tests rather than a separate framework. Error format consistency is already enforced. We use ast-grep rules in a commit hook to verify error paths use What I'd find valuable:
What I'd skip:
@kingpanther13 good point about BAT, though I see BAT as testing how well LLMs work with our tools (descriptions, context, workflows), while input validation tests the tools themselves. E2E is the better home for this. Could you point to specific bugs or regressions this would have caught? That would help ground the scope in real problems rather than theoretical coverage. |
|
Fair points across the board β and the ast-grep enforcement on What ast-grep doesn't cover is the input side: whether a bad input # Does this return a structured error, or an empty results list with success=True?
ha_get_state(entity_id="sensor.") # malformed
ha_search_entities(query="", limit=0) # boundary input
ha_config_set_helper(name="") # empty required fieldThat's the narrower gap I'd find useful β a handful of parametrized cases
If even that feels like over-engineering here, totally understood β |
|
I think that's fair to look into. We recently had a lot of issues with false success, and I just went thru a PR that sought to eliminate the remaining false successes, but obviously it's impossible to catch everything. I've also noticed Claude will "typo" things pretty frequently ... My MCP in Claude has "cloudflared" in the name and occasionally Claude will call it "cloudflUred" or "cloudflaredd" for no particular reason (I have never used those names before so no idea where it even got them) which causes the tool to fail. I ask it why this happened and it just says "oops, I made a typo" which makes no sense. It catches itself in those instances and tries again with proper name, but this tells me it's a certainly possible for it to hallucinate any type of input. So yeah, tests to catch intentionally malformed inputs to see if errors are returned properly sounds like a decent idea. |
|
@Patch76 Sounds good β a focused PR adding parametrized negative-input cases to the existing E2E suite is welcome. A few guidelines: Start small. Pick one archetype (A3 or A4) and cover 5-6 tools in a single PR. A PR touching all 92+ tools will be hard to review, and any repeated mistakes get multiplied across the whole suite. Better to get the pattern right on a small scope first. Keep it tight. More tests isn't always better β each one adds CI time. Before adding a test, ask: does this hit a different branch in the code, or just a different spelling of the same input? 2-3 cases per tool (malformed, empty, nonexistent) rather than exhaustive permutations. Check existing coverage. Some negative paths may already be tested in the E2E suite. No point duplicating. |
|
Following up β first PR is up: β‘οΈ #924 β Covers |
|
Following up on the A3 thread β #924 just landed its final revision and I've been mapping what's left in the archetype before opening another PR. Where #924 ended upAfter sergeykad's review I went back and verified the existing coverage properly β both unit and E2E layers β instead of relying on my initial E2E-only scan. The corrected picture was: 3 of 4 original tests were duplicates with hard assertions already in place ( That process also gave me a cleaner methodology for the next round: check the source code to identify distinct failure paths first, then verify coverage for each path across both layers before writing anything. What I found for the remaining A3 toolsApplying the same methodology to
The valid_entity_id("") # False β WS rejects β error response
valid_entity_id("sensor.") # False β WS rejects β error response
valid_entity_id("sensor.nonexistent") # True β WS succeeds, empty resultThese two paths ( One note relevant to #911: that PR introduces
if automation_id.startswith("automation."):
domain = "automation"
elif automation_id.startswith("script."):
domain = "script"
else:
raise_tool_error(...) # rejected before any WebSocket call
Proposed scopeThree tests across two tools, all in the existing E2E suite:
Happy to hold off until #924 and/or #911 land first if that's preferable. |
|
Following up on the A3 thread β a correction to the #924 methodology, and a proposal for A4. Correction from #924sergeykad's review of #924 surfaced a methodology error worth documenting before the next PR. The analysis behind #924 claimed that The corrected coverage picture for
So the only case in #924 with genuinely new coverage is The methodology fix going forward: verify Proposed scope: A4 Β·
|
| # | Input | Path | What it proves |
|---|---|---|---|
| 1 | ha_search_entities(query="light", limit=0) |
slice [0:0] |
count=0, success=True; documents boundary |
| 2 | ha_search_entities(query="light", limit=-1) |
slice [0:-1] |
Error or documented degraded behavior |
| 3 | ha_search_entities(query="", limit=5) |
_exact_match_search empty string |
All entities match; hard-asserts current behavior |
Test 3 is the weakest β happy to drop it if soft coverage is considered sufficient.
Happy to wait until #924 and/or #911 land before opening the next PR.
|
The The For the A4 scope: |
|
Closing the loop on this discussion β here's what came out of it.
#924 (closed, not merged) β first A3 attempt. Revealed that 3 of 4 proposed tests duplicated existing hard assertions elsewhere in the suite. Closed in favor of a cleaner scope β but the failure was the most useful outcome: verifying coverage across both unit and E2E layers before writing tests changed the methodology for everything that followed. #946 (merged 2026-04-11) β silent data-corruption fix in #954 (merged 2026-04-11) β same root cause in #945 (merged 2026-04-12) β negative-input tests for #957 (merged 2026-04-12) β one hard-asserting test for #958 (merged 2026-04-12) β same for Thanks again for the direction that shaped this β the archetype framing made it straightforward to identify where real gaps existed versus where coverage only looked missing. I learned a lot out of it ... and my Claude too π©οΈ To be continued ... |
|
Following up with A7 β first PR is up: β‘οΈ #987 β Three structurally distinct failure paths, source-verified before writing:
The existing CRUD lifecycle tests verify deletion via Survey of remaining A7 tools:
Happy to follow up on the remaining gaps in a subsequent PR if the pattern in #987 looks right. |
|
Correction to the A7 survey in my previous comment β I checked more carefully against the actual test files. Revised picture:
So the remaining work is smaller than I thought: 3 hard gaps + 2 soft-only tests worth hardening. Plan is to address these in 1β2 follow-up PRs after #987 lands. |
|
Closing the loop on A7 β #987 landed today
So the actual remaining A7 work is larger than my previous comment suggested: 4 hard gaps ( Methodology note for myself: I had relied on test names ( For context, the archetype-PR series that grew out of this thread:
Flagging the corrected gap list here as a record. No action from my side until there's appetite for a follow-up β happy to leave it open. |
|
A7 follow-up landed β #1047 ( Source-verification before authoring shifted the picture from my 17.04 survey:
So A7 is functionally closed: 5 of 7 hard, 2 left in flight with documented blockers (not silent gaps). Updated archetype state:
Same methodology lesson held up: reading the assert body (not just the test name) caught the four false-gap entries before they became another #924-style retraction. |
|
Wrap-up β tracing each archetype to its current disposition. State at 2026-05-23:
A7 blockers β filed as successor issues:
A1: behavior under degraded HA states (down, restarting, partial integration failures) belongs in chaos-engineering scope, not input-archetype scope. Defensibly out. Closing as resolved. 8 of 9 archetypes closed or substantively covered; 1 defensibly skipped (A1). The two A7 follow-ups now have focused homes in #1412 and #1413. Wrap-up β tracing each archetype to its current disposition. State at 2026-05-23:
A7 blockers β filed as successor issues:
A1: behavior under degraded HA states (down, restarting, partial integration failures) belongs in chaos-engineering scope, not input-archetype scope. Defensibly out. Closing as resolved. 8 of 9 archetypes closed or substantively covered; 1 defensibly skipped (A1). The two A7 follow-ups now have focused homes in #1412 and #1413. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
I've been thinking about test coverage for ha-mcp tools and wanted to open a discussion
before writing a single line of code. @kingpanther13 @sergeykad β would love your take.
The problem
We have 92+ tools. Writing an individual test scenario for each one doesn't scale β
and the existing E2E suite covers happy paths well, but adversarial inputs (bad entity
IDs, wrong types, injection attempts, double-deletes, concurrent calls) are largely untested.
The idea: group by input archetype, not by domain
Looking at the tool surface, I count 9 input archetypes that recur across all domains.
One reusable test suite per archetype covers the majority of tools without per-tool work:
ha_get_overview,ha_check_configha_get_zone,ha_config_get_label,ha_get_blueprintentity_idha_get_state,ha_get_entityha_search_entities,ha_deep_search,ha_hacs_searchha_config_set_automation,ha_config_set_helperha_config_set_automation(identifier=β¦),ha_set_entityha_config_remove_automation,ha_delete_config_entryha_restart,ha_backup_restore,ha_bulk_controlha_get_operation_status(operation_id=β¦)Torture dimensions (orthogonal to archetypes)
Each archetype gets hit from these angles:
limit=0,offset > total,"",null, 10k-char stringsstrwherelistexpected,intwhereboolexpected{{ 1/0 }}), null bytes, Unicode edge casesha_bulk_controlrace vs. individual state write. This matters: a misbehaving LLM will fire concurrent calls.ha_deep_search,ha_backup_create; does the server degrade gracefully or crash?create_error_response? No raw exceptions?Starting point I'd suggest
A3 + A4 first β purely read-only, no side effects, high tool count. Quick wins to validate
the harness pattern before touching anything with state or side effects.
A rough sketch (pseudocode β adapting to actual test client API):
Open questions β in order of difficulty
tests/src/e2e/ortests/torture/, or a standalone harness users run against their own instance? Both have merit.asyncio.gather()suffices for simple races; is there appetite for a more structured concurrency test layer?ha_manage_custom_tool) β if it lands, it'll need its own class: sandbox-escape attempts, code injection, resource exhaustion. Worth designing upfront.Happy to draft a first PR for A3+A4 if the direction makes sense.
All reactions