Fix firmware update error message missing exception type#760
Fix firmware update error message missing exception type#760TheJulianJES merged 2 commits intozigpy:devfrom
Conversation
A bare `TimeoutError()` from zigpy's `asyncio.timeout()` has an empty `str()`, which produced the unhelpful user-facing message `Update was not successful: ` with no clue about the failure. Fall back to `repr(ex)` when `str(ex)` is empty so the exception type is always visible.
Patches `Device.update_firmware` to raise a bare `TimeoutError()` and asserts the resulting `ZHAException` carries an identifiable message and preserves cause chaining.
There was a problem hiding this comment.
Pull request overview
Improve user-facing firmware update failure messaging by ensuring exceptions with empty str(ex) (e.g., bare TimeoutError()) still surface an identifiable error string in ZHA’s update platform.
Changes:
- Update firmware install exception handling to fall back to
repr(ex)whenstr(ex)is empty. - Add a regression test covering the empty-exception-message case and verifying exception chaining.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| zha/application/platforms/update.py | Adjusts the raised ZHAException message to include exception type/details when str(ex) is empty. |
| tests/test_update.py | Adds a regression test asserting the improved error message for bare TimeoutError() and preserved __cause__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with ( | ||
| patch( | ||
| "zigpy.device.Device.update_firmware", | ||
| AsyncMock(side_effect=raised), | ||
| ), | ||
| pytest.raises(ZHAException) as exc_info, | ||
| ): | ||
| await entity.async_install( | ||
| version=f"0x{fw_image.firmware.header.file_version:08x}" | ||
| ) |
There was a problem hiding this comment.
This pattern is copied from the existing tests. This is technically somewhat correct, but we generally don't guard for failure cases in tests like this. I'd just leave this as is.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #760 +/- ##
=======================================
Coverage 97.64% 97.64%
=======================================
Files 62 62
Lines 10873 10873
=======================================
Hits 10617 10617
Misses 256 256 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed change
This fixes an issue where a
TimeoutErrorduring an update showed "Update was not successful: " as an error message. With this PR, we now show the exception type itself (repr(ex)) if the exception string (str(ex)) is empty.A test is also added, though I'm not sure how necessary it really is. It does run and test the added code, so I've kept it for now (as this isn't done otherwise).
When this happens
This only really happens with devices that have a
PollControlcluster. Otherwise, we should always get aDeliveryError. Maybe something to address in a future zigpy PR(?)AI summary
Long AI summary (click to expand)
Preserve exception type in firmware update error messages
Problem
When a firmware update fails because the underlying
update_firmware()call raises a bareTimeoutError()— the canonical case beingasyncio.timeout()cancelling its inner await in zigpy (zigpy/device.py:689,async with asyncio_timeout(timeout): await future) — the user-facing error message is empty after the colon:BaseFirmwareUpdateEntity.async_installcatches the exception and re-raises it as:For
TimeoutError()(and any other exception constructed without arguments),str(ex) == "", so the f-string interpolates nothing and the user sees no clue about what went wrong.Fix
Fall back to
repr(ex)whenstr(ex)is empty:Now a bare
TimeoutError()renders asUpdate was not successful: TimeoutError(). Exceptions with non-emptystr()are unaffected —"bad version"still shows up asUpdate was not successful: bad version, notValueError('bad version').The
orform is safe across exceptions reachable fromexcept Exception:KeyboardInterrupt/SystemExit/asyncio.CancelledErrorderive fromBaseException, so they don't reach this clause.repr(ex)whenstr(ex)is empty, andBaseException.__repr__is built-in and never raises."0"-style strings stay as-is (truthy inor); contrived but accurate.The second
raise ZHAException(f"Update was not successful: {result}")branch (theresult != Status.SUCCESSpath) is left alone —Statusalways renders meaningfully.Tests
test_firmware_update_empty_exception_message— patchesDevice.update_firmwareto raise a bareTimeoutError()and asserts the resultingZHAExceptionmessage is exactlyUpdate was not successful: TimeoutError()and that__cause__chaining is preserved. Confirmed to fail before the fix (message wasUpdate was not successful:with a trailing space).