test(e2e): harden test_delete_calendar_event scaffold + skip pending #1416 - #1415
test(e2e): harden test_delete_calendar_event scaffold + skip pending #1416#1415Patch76 wants to merge 5 commits into
Conversation
…etable event (closes homeassistant-ai#1413) Replace the hard-coded fake-UID negative-path assertion with a create-then- delete-twice flow: - New `deletable_event_uid` yield-fixture creates a fresh event with a UUID-based unique summary, retrieves the HA-assigned UID via ha_config_get_calendar_events (set_calendar_event does not return it), and best-effort deletes on teardown so the event never leaks regardless of test outcome. - test_delete_calendar_event now hard-asserts success on the first delete (positive path) and hard-asserts failure with suggestions on a second delete of the just-released UID (negative path). UID-collision risk is eliminated because the UID was just held and released by this test. Closes A7-archetype soft-coverage gap from discussion homeassistant-ai#914. No user-visible change — pure CI-side regression-test hardening. Assumes ha_config_remove_calendar_event raises on missing UID per the current _remove_* tool family convention. If homeassistant-ai#1412 commits to idempotent- success on missing, the negative-path assertion will need to flip.
…est_calendar.py CI's whole-file ruff format check fails on pre-existing parenthesisation style across 5 assertion blocks and 1 logger.info call in this file. Mechanical reformat with no semantic change; touched lines are all outside the test_delete_calendar_event hardening from the previous commit. Required to unblock the CI gate on this PR rather than discretionary boy-scout cleanup; the ruff format check runs on every changed file (`echo "$changed_py" | xargs uv run ruff format --check`), so any edit to this file pulls in the pre-existing debt as a hard prerequisite.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability and coverage of the calendar event deletion tests. By dynamically generating test data and validating both successful and failed deletion attempts, the test suite is now more robust and less prone to side effects or false positives in CI environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the calendar E2E tests by introducing a deletable_event_uid fixture to manage temporary test events and refactoring test_delete_calendar_event to cover both successful and failed deletion scenarios. The reviewer recommends using the safe_call_tool utility for tool calls within the fixture to improve error handling and leveraging the assert_mcp_success helper for more idiomatic assertions.
…ble_event_uid CI E2E failure on both ubuntu-latest and ubuntu-24.04-arm: first delete of the just-created event returned `400 Bad Request` despite the UID having been retrieved successfully via ha_config_get_calendar_events moments earlier. HA Local Calendar appears to expose the new event through the REST endpoint behind get_calendar_events before the calendar.delete_event service will accept the same UID. Switch the positive-path delete from a single safe_call_tool to wait_for_tool_result with a predicate of `success is True`, absorbing the window without committing to a specific delay. Negative path stays on safe_call_tool — its expected outcome is failure, polling doesn't apply. Also adopt three Gemini code-assist suggestions on the fixture: - create call uses safe_call_tool (consistent with existing test patterns) - get_calendar_events call uses safe_call_tool (same) - positive-path assertion is implicit in wait_for_tool_result; the helper raises TimeoutError on persistent failure, which is more informative than a manual assert
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the calendar E2E tests by introducing a deletable_event_uid fixture to manage temporary test events and improving the test_delete_calendar_event workflow with both positive and negative test paths. Feedback focuses on improving test reliability through polling for event creation, ensuring consistency by using UTC for timestamps, and addressing a contradiction between the test's failure expectations and the tool's idempotentHint: True safety annotation.
…400 investigation (refs homeassistant-ai#1416) The new positive-path assertion against ha_config_remove_calendar_event fails deterministically against the seeded `calendar.local_e2e_test` entity in CI: HA returns 400 Bad Request from calendar.delete_event for a UID retrieved seconds earlier via ha_config_get_calendar_events, and the 400 persists across a 15-second polling window. Two CI iterations (single-shot and polled) reproduce the same failure. Filed homeassistant-ai#1416 with full evidence + open question. Skip this test so the hardening scaffold (yield-fixture, helper adoption, create-then-delete- twice flow) still lands as the foundation for a clean un-skip once the root cause is known. The original soft-assertion test is replaced rather than left in place — the new pattern is the right one, just blocked at runtime today.
Patch76
changed the title
Implementation SummaryChoices MadeYield-fixture over inline create/delete. The bot's proposed diff used a regular UID retrieval via polled TZ-aware datetime in the fixture. The new fixture uses Adopted Gemini code-assist findings on the fixture — Bundled a pre-existing ruff-format fix for 5 assertion blocks + 1 logger.info call in untouched regions of the same file (
Problems EncounteredDeterministic 400 on Title/body identifier drift post-skip: PR originally said Gemini suggestion required adaptation for Python 3.13. The literal Suggested Improvements (post-merge)Triage #1416 — the failure pattern is reproducible, the existing successful path bypasses individual delete entirely, and source reads into HA Core's local_calendar component did not converge on a confident root cause. A direct check by someone with HA Local Calendar familiarity would unblock #1413's full closure. |
…vent_uid Address two Gemini code-assist findings on PR homeassistant-ai#1415 round 2: - Replace the single-shot ha_config_get_calendar_events call in deletable_event_uid with wait_for_tool_result polling on a predicate that checks the just-created summary appears in the events list. Failure-after-timeout now raises pytest.fail (loud) instead of pytest.skip (silent), matching the "create succeeded but list lags" failure mode the suggestion described. - Switch the fixture's datetime.now() to datetime.now(UTC) so the ISO 8601 strings are unambiguous when the test runner and the HA instance live in different timezones. Uses the repo's existing convention (from datetime import UTC, datetime, ...) per tests/src/e2e/workflows/core/test_history.py. A third finding about the second-deletion assertion contradicting the tool's idempotentHint annotation is intentionally not adopted — the tool's actual behaviour today is to raise on missing UID, consistent with the rest of the _remove_* family. The annotation/behaviour mismatch is tracked at homeassistant-ai#1412 and applies to all 12 _remove_* tools, not just this one. Tolerant assertion would defeat the hardening purpose.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the calendar E2E tests by introducing a deletable_event_uid fixture for temporary event management and refactoring the deletion test to include positive and negative validation paths with asynchronous polling. Feedback was provided to narrow the exception handling in the fixture's cleanup logic to better distinguish between different failure types and adjust logging levels accordingly.
| try: | ||
| await mcp_client.call_tool( | ||
| "ha_config_remove_calendar_event", | ||
| {"entity_id": calendar_entity, "uid": event_uid}, | ||
| ) | ||
| except Exception as cleanup_error: | ||
| logger.debug( | ||
| f"Cleanup of test event {event_uid} on {calendar_entity}: " | ||
| f"{cleanup_error}" | ||
| ) |
There was a problem hiding this comment.
Avoid using broad except Exception in resource cleanup handlers. Following repository rules, use narrow exception handling to differentiate between failure types: log transport-level failures (like OSError) at debug level, but log command-level failures at warning level to surface potential resource leaks. Other exceptions should propagate to avoid masking programming bugs.
References
- In resource cleanup operations, use narrow exception handling and differentiate logging levels: log transport-level failures at debug level and command-level failures at warning level.
- Avoid using broad
except Exception: passin resource cleanup or teardown logic to prevent masking bugs.
Carry-over from PR homeassistant-ai#1415 — use datetime.now(UTC) instead of naive datetime.now() in the fixture so ISO 8601 strings stay unambiguous when the test runner and HA instance are in different timezones. Matches the convention in tests/src/e2e/workflows/core/test_history.py. Pre-existing naive datetime.now() calls elsewhere in this file are unchanged — format-debt tracked broadly at homeassistant-ai#1318.
…loses #1413, #1416) (#1418) * fix(calendar): use WS calendar/event/delete instead of REST service HA's calendar component registers only calendar.create_event and calendar.get_events as REST services. calendar.delete_event and calendar.update_event are exposed exclusively via the WebSocket API (calendar/event/delete, calendar/event/update) — see homeassistant/components/calendar/__init__.py:325-340. Calling /api/services/calendar/delete_event returned 400 "service not found" for every UID regardless of whether the event existed. Switch ha_config_remove_calendar_event to self._client.send_websocket_message({"type": "calendar/event/delete", ...}), check result["success"], and raise ToolError(SERVICE_CALL_FAILED) on WS failure. Replace the soft-assert test_delete_calendar_event (hard-coded fake UID) with a deletable_event_uid fixture that creates a fresh event, retrieves its UID via ha_config_get_calendar_events, yields (entity_id, uid), and best-effort cleans up on teardown. The rewritten test hard-asserts both the positive path (first delete succeeds) and the negative path (re-delete of released UID fails with suggestions). Closes #1413 Closes #1416 * refactor(calendar): use ws_client.send_command for delete (gemini review) Switch from client.send_websocket_message (manual envelope + status dict) to get_connected_ws_client + ws_client.send_command — matches the pattern used by tools_traces, tools_history, tools_hacs, etc. send_command auto-handles message-ID generation and response tracking, and raises HomeAssistantCommandError on WS failure which the outer except Exception routes through exception_to_structured_error. Removes the manual result["success"] check (now redundant since send_command raises instead of returning a status dict). * fix(calendar): address review findings — guard disconnect, poll for indexing, tighten asserts Review feedback from code-reviewer + silent-failure-hunter + test-analyzer agents on PR #1418: src/ha_mcp/tools/tools_calendar.py: - Guard ws_client.disconnect() in finally so a transport-teardown error from the close path does not replace the original send_command exception (e.g. ConnectionResetError swallowing a HomeAssistantCommandError the caller should actually see). tests/src/e2e/workflows/calendar/test_calendar.py: - Poll for HA indexing between create and get-UID using wait_for_tool_result (15s budget). Local Calendar persists synchronously but CalDAV / Google backends may not — per AGENTS.md § "E2E tests: poll after creating entities". - On the TimeoutError/missing-UID path, attempt a best-effort scan-by- summary cleanup before pytest.fail so the orphaned event is not leaked across CI runs (HAOS-inaddon reuses the image). - Hard-require `success is True` on the first delete in addition to assert_mcp_success (which accepts several indicators) — this PR's regression test should fail loudly if a future refactor drops the flag. - Accept either `error.suggestions` (plural) or `error.suggestion` (singular) — create_error_response writes the singular form when the list has one entry, so the previous assertion would silently flip if the tool's exception path was ever narrowed to one suggestion. - Surface cleanup failures at logger.warning instead of debug so leaked events are visible in CI output. * test(calendar): TZ-aware datetime in deletable_event_uid fixture Carry-over from PR #1415 — use datetime.now(UTC) instead of naive datetime.now() in the fixture so ISO 8601 strings stay unambiguous when the test runner and HA instance are in different timezones. Matches the convention in tests/src/e2e/workflows/core/test_history.py. Pre-existing naive datetime.now() calls elsewhere in this file are unchanged — format-debt tracked broadly at #1318. --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
|
Closing as superseded by #1418 |
2 participants
What does this PR do?
Refs #1413 (the runtime hardening is blocked at HA Local Calendar — see
#1416 for the investigation issue, body of this PR for the skip-context).
Hardens
test_delete_calendar_eventby replacing the hard-coded fake-UIDnegative-path assertion with a create-then-delete-twice flow.
A new
deletable_event_uidyield-fixture creates a fresh event with aUUID-based unique summary, retrieves the HA-assigned UID via
ha_config_get_calendar_events(set_calendar_eventdoes not return itin the response), yields
(entity_id, uid), and best-effort deletes onteardown so the event never leaks regardless of test outcome.
The refactored test would hard-assert success on the first delete
(positive path) and hard-assert failure with suggestions on a second
delete of the just-released UID (negative path). UID-collision risk on
shared calendars is eliminated because the UID was just held and
released by this test.
Runtime skipped pending #1416: against the seeded
calendar.local_e2e_testentity,calendar.delete_eventreturnspersistent 400 Bad Request for a UID retrieved seconds earlier via
ha_config_get_calendar_events. Two CI iterations (single-shot, thenpolled 15s) reproduce. The hardening scaffold lands as the foundation
for un-skip once the root cause is identified.
The negative-path assertion assumes the current
_remove_*familyconvention (raises on missing UID). If #1412 lands the alternative
contract (idempotent-success on missing), this assertion will need to
flip when the test is un-skipped.
Type of change
Testing
uv run pytest)uv run ruff check)Checklist