Skip to content

Replace fixed-duration sleeps after bus events with gather#6803

Open
agners wants to merge 1 commit intomainfrom
tests-bus-fire-event-gather
Open

Replace fixed-duration sleeps after bus events with gather#6803
agners wants to merge 1 commit intomainfrom
tests-bus-fire-event-gather

Conversation

@agners
Copy link
Copy Markdown
Member

@agners agners commented May 5, 2026

Proposed change

Several tests use await asyncio.sleep(...) to "wait for the listener to run" after firing a bus event. The fixed duration is real wall-clock time and the wait can be indeterministic — if the handler chain happens to need slightly more time on a busy CI runner, the assertion races the handler.

Bus.fire_event returns the listener tasks since #6252; capture and await asyncio.gather(*tasks) instead of sleeping. Touches test_bus.py (the bus tests were poking scheduling instead of verifying their assertions), test_home_assistant_watchdog.py, test_plugin_base.py, addons/test_manager.py, docker/test_addon.py, and test_store_execute_reload.py.

Other cleanups in the same spirit:

  • _fire_test_event in addons/test_addon.py becomes async def and gathers the listener tasks itself, so its 17 call sites collapse to a single await _fire_test_event(...).
  • The two test_store_execute_reload.py sites that used the private _update_connectivity() helper are reworked to set the cached connectivity flag directly and fire the event themselves so they can gather the listener tasks the same way.
  • The two sleep(1) post-pull drains in docker/test_interface.py collapse to sleep(0) (handler tasks are already gathered inside pull_image), saving ~2s.
  • The sleep(0.01) waits inside container_events() task bodies (api/test_addons.py, api/test_store.py, backups/test_manager.py) are just one-yield-to-the-parent and become sleep(0).

Switching to gather exposes a few latent test mocks that were silently swallowing TypeErrors as background-task failures before: CGroup.add_devices_allowed is async def but was patched as a plain MagicMock in docker/test_addon.py (now new_callable=AsyncMock), and the watchdog does await (await self.start()) / await (await self.restart()) because App.start / App.restart return asyncio.Task — the mocks in addons/test_addon.py (test_app_watchdog, test_watchdog_on_stop, test_watchdog_during_attach) needed AsyncMock(return_value=<settled future>) to mirror that shape rather than a plain MagicMock.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:
  • Link to cli pull request:
  • Link to client library pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Ruff (ruff format supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints or add-on configuration are added/changed:

@agners agners requested a review from mdegat01 May 5, 2026 10:13
@agners agners added the test Adding missing tests or correcting existing tests label May 5, 2026
Several tests use ``await asyncio.sleep(...)`` to "wait for the
listener to run" after firing a bus event. The fixed duration is
real wall-clock time and the wait can be indeterministic — if the
handler chain happens to need slightly more time on a busy CI
runner, the assertion races the handler.

``Bus.fire_event`` returns the listener tasks since #6252; capture
and ``await asyncio.gather(*tasks)`` instead of sleeping. Touches
test_bus.py (the bus tests were poking scheduling instead of
verifying their assertions), test_home_assistant_watchdog.py,
test_plugin_base.py, addons/test_manager.py, docker/test_addon.py,
and test_store_execute_reload.py.

Other cleanups in the same spirit:

- ``_fire_test_event`` in addons/test_addon.py becomes ``async def``
  and gathers the listener tasks itself, so its 17 call sites
  collapse to a single ``await _fire_test_event(...)``.
- The two test_store_execute_reload.py sites that used the private
  ``_update_connectivity()`` helper are reworked to set the cached
  connectivity flag directly and fire the event themselves so they
  can gather the listener tasks the same way.
- The two ``sleep(1)`` post-pull drains in docker/test_interface.py
  collapse to ``sleep(0)`` (handler tasks are already gathered
  inside pull_image), saving ~2s.
- The ``sleep(0.01)`` waits inside ``container_events()`` task
  bodies (api/test_addons.py, api/test_store.py,
  backups/test_manager.py) are just one-yield-to-the-parent and
  become ``sleep(0)``.

Switching to ``gather`` exposes a few latent test mocks that were
silently swallowing TypeErrors as background-task failures before:

- ``CGroup.add_devices_allowed`` is ``async def`` but was patched
  as a plain MagicMock in docker/test_addon.py — now patched via
  ``new_callable=AsyncMock``.
- The watchdog does ``await (await self.start())`` /
  ``await (await self.restart())`` because ``App.start`` /
  ``App.restart`` return ``asyncio.Task``. The mocks in
  addons/test_addon.py (test_app_watchdog, test_watchdog_on_stop,
  test_watchdog_during_attach) needed
  ``AsyncMock(return_value=<settled future>)`` to mirror that
  shape rather than a plain MagicMock.
Comment on lines +54 to +65
async def _fire_test_event(coresys: CoreSys, name: str, state: ContainerState) -> None:
"""Fire a test event and await the listener tasks the bus spawned."""
await asyncio.gather(
*coresys.bus.fire_event(
BusEvent.DOCKER_CONTAINER_STATE_CHANGE,
DockerContainerStateEvent(
name=name,
state=state,
id="abc123",
time=1,
),
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could consider moving this to tests.common and making it a generic helper that accepts event, data and awaits the task. So we can use it in all the places shown.

Copy link
Copy Markdown
Contributor

@mdegat01 mdegat01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I really hated those sleeps, especially the ones that had to be > 0 to make the test work, much better 👍

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

Labels

cla-signed test Adding missing tests or correcting existing tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants