Repository navigation
Fail tests that leave tasks behind - #1907
Draft
zigpy-review-bot wants to merge 5 commits into
Draft
zigpy-review-bot wants to merge 5 commits into
zigpy-review-bot wants to merge 5 commits into
Conversation
`verify_cleanup` is a sync fixture that looked up the running event loop. Sync fixtures run outside the loop under pytest-asyncio, so it always took its early return and never checked anything: a test that leaked a thread still passed. Like Home Assistant's version, it now uses `pytest_asyncio.fixture` with `asyncio.get_event_loop()`. With the fixture active, fix what it and `ResourceWarning`s turned up: - the `app` and `app_mock` fixtures shut their application down, so its watchdog, backup and topology tasks no longer linger - `with sqlite3.connect(...) as conn` only commits, so wrap the connections in `contextlib.closing()` as well - `test_v4_to_v5_migration_bad_neighbors` read the migrated table through the first block's cursor, which only worked because that connection was never closed - `test_dynamic_bounded_semaphore_multiple_event_loops` closes its event loops
When the second loop's `gather()` raises, the first `test_semaphore` task is still sleeping. Closing a loop does not cancel its tasks, so cancel and await them first.
`auto_kill_aiosqlite` was torn down before `verify_cleanup` and quietly stopped leaked aiosqlite threads, so a leaked connection still passed in the modules that use it. `verify_cleanup` now requests the fixture, which tears it down afterwards: the leak fails the test in every module, and the thread is still stopped so pytest can exit. No test leaves a timer behind, so lingering timers now fail the test as in Home Assistant instead of only being logged.
They only worked as plain `pytest.fixture`s because of `asyncio_mode = auto`. Declare them like `verify_cleanup` so they don't depend on that setting.
`verify_cleanup` only logged lingering tasks, so leaks went unnoticed: the topology scan loop that `stop_periodic_scans()` could not stop showed up only as such a warning, and not at all while `verify_cleanup` did not run. Fail the test instead, like timers and threads. Tasks and timers are cleaned up first and reported together. Nearly all lingering tasks came from applications that tests create with `make_app()` and never shut down. `make_app()` now registers each application in a list like Home Assistant's `INSTANCES`, but instead of aborting the run, `verify_cleanup` shuts them down before its checks. It does not close a database the test already closed, and reports a failing shutdown without leaking applications into the next test. The `app` and `app_mock` fixtures no longer need their own teardown, and `test_request_future_matching` waits for the OTA cluster's packet handling.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1907 +/- ##
==========================================
+ Coverage 99.51% 99.53% +0.02%
==========================================
Files 61 61
Lines 12773 12773
==========================================
+ Hits 12711 12714 +3
+ Misses 62 59 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Oct 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge order: #1900 first. This PR is stacked on #1900 and will be rebased onto
devonce #1900 is merged. Until then, the diff below also shows #1900's commits; the only commit that belongs to this PR is the last one, "Fail tests that leave tasks behind".verify_cleanuponly logged lingering tasks after a test, so leaks went unnoticed. The topology scan loop thatstop_periodic_scans()couldn't stop (#1904) had been visible for years only as one of those warnings, and since #1751, whenverify_cleanupstopped running, not even that. Like Home Assistant's version, a lingering task now fails the test, as timers and leaked threads already do with #1900.Changes
verify_cleanupcancels and awaits the test's leftover tasks and cancels its timers, then fails the test once, listing both. The message is now "Lingering tasks after test" instead of "Linger task after test".make_app()registers every application it creates in a list like Home Assistant'sINSTANCES. Unlike HA, which aborts the run on leftover instances,verify_cleanupshuts them down before its checks. It skips closing a database the test already closed, becauseControllerApplication.shutdown()isn't idempotent and would log a spurious error. It also takes all applications out of the list first, and reports a failing shutdown as a test failure, so one test can't leak an application into the next.verify_cleanuprun and close resources leaked by tests #1900's fixture teardown in place, 50 tasks lingered in 20 tests: 49 in 19 tests from applications started withmake_app()and never shut down (startup, backup, TX power, channel move and watchdog tests intest_application.py, and one OTA test), and one from the OTA cluster intest_request_future_matching.appandapp_mockfixtures are plain fixtures again, because the registry shuts their applications down.test_request_future_matchingwaits, with a timeout, for the background tasks in which the OTA cluster handles its packets.CatchingTaskMixin's task set is shared by all objects, so it only waits for the tasks the test started.expected_lingering_tasksopt-out. The suite is small enough to fix every leak, and an opt-out would only let new ones back in.No library code changes.
Testing: the full suite passes, including with
-W error::ResourceWarning -W error::pytest.PytestUnraisableExceptionWarning, in random order. A leaked task now fails both async and sync tests. With the plain fixtures but without the registry shutdown, 27 tests intest_application.pyandtests/otafail. The number of "Failed to disconnect from database" teardown warnings is unchanged from #1900.