Cover the Siri exposure settings edge cases - #5821
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several moderate test coverage and UI test isolation issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds edge-case coverage for Siri exposure persistence, refresh behavior, migrations, cleanup, and reload UI handling.
Changes:
- Adds database, migration, and server-deletion cleanup tests.
- Tests the reload progress state.
- Simplifies reload button action wiring.
File summaries
| File | Summary | Review notes |
|---|---|---|
Tests/App/Utilities/SiriServerConfigurationViewModelTests.swift |
Tests refresh and database failure paths. | Nit: nest the recording updater helper. |
Tests/App/Utilities/SiriEntityExposureTests.swift |
Tests persistence failures and migrations. | Moderate: exercise an actual schema change. |
Tests/App/Utilities/ServerDeletionSiriCleanupTests.swift |
Tests Siri cleanup during server deletion. | Moderate: verify rows/defaults are removed directly. |
Tests/App/Settings/SiriServerConfigurationView.test.swift |
Tests reload progress state. | Moderate: assert hosted loading UI and avoid replacing the key window. |
Sources/App/Settings/Siri/SiriServerConfigurationView.swift |
Passes the reload method directly. | No findings. |
Review details
Suppressed comments (3)
Tests/App/Settings/SiriServerConfigurationView.test.swift:76
makeKeyAndVisible()steals the process-wide key window, and this suite's serialization does not protect other UI/snapshot suites running in parallel; leaving that replacement window hidden can make those tests flaky. Existing render tests deliberately use a non-key window and clear its root controller (for exampleTests/App/Settings/SettingsViewRender.test.swift:29-40), so use that teardown pattern here.
window.makeKeyAndVisible()
Tests/App/Utilities/ServerDeletionSiriCleanupTests.swift:24
- Because an absent exposure row is intentionally treated as exposed, these assertions cannot distinguish deletion from a row that remains with
isExposed == true; they also would miss stale entity defaults. Query both tables and assert the server and entity rows are absent (or otherwise verify the default is removed), rather than checking only the fail-open read API.
#expect(SiriServerExposure.isExposed(serverId: serverId))
#expect(SiriEntityExposure.isExposed(serverId: serverId, entityId: "todo.a"))
Tests/App/Utilities/SiriServerConfigurationViewModelTests.swift:191
- Please move
RecordingSiriAppDatabaseUpdaterinsideSiriServerConfigurationViewModelTests(or give it its own file). This change adds a second top-level type to the test file, whereas the repository's one-type-per-file convention allows private helper types only when nested.
private final class RecordingSiriAppDatabaseUpdater: AppDatabaseUpdaterProtocol {
struct Update {
let serverId: String
let forceUpdate: Bool
let showProgress: Bool
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5821 +/- ##
==========================================
+ Coverage 48.51% 48.80% +0.29%
==========================================
Files 1185 1189 +4
Lines 78770 79012 +242
==========================================
+ Hits 38214 38564 +350
+ Misses 40556 40448 -108
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AI Policy
Select exactly one option that describes AI usage in this contribution:
Summary
Follow-up to #5819. Adds tests for the lines Codecov reported as uncovered there: the default refresh path through the app database updater, the error branches of the Siri exposure queries, the table's in-place migration, the toolbar's progress state while reloading, and the Siri exposure cleanup on server deletion. The reload button now takes the view model method directly instead of wrapping it in a closure.
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes