Adjust WebsocketManager check to avoid location usage#6577
Adjust WebsocketManager check to avoid location usage#6577TimoPtr merged 8 commits intohome-assistant:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors WebsocketManager’s per-server run eligibility checks to short-circuit earlier, avoiding unnecessary internal-network detection work (and its side effects, like triggering location-related checks) when the configured WebSocket setting will prevent the worker from running anyway.
Changes:
- Reordered
shouldRunForServerto return early forNEVER, no connectivity, and not-registered states before evaluating additional conditions - Deferred the internal-network (
isInternal) evaluation to only when the setting isHOME_WIFI - Replaced a non-null assertion on
PowerManageraccess with a null-safe check when evaluating theSCREEN_ONsetting
|
If this is the real fix, then I would like a test that verify that we are not calling isInternal in this scenarios, so if we refactor we don't break it again and a small comment. |
It's not really a 'fix', just being more mindful of unnecessary location usage where it isn't expected or needed, which now stands out more 🔵. I'll try adding tests to make sure it isn't accidentally changed. |
|
Testing WorkManager seems to require instrumentation tests. Useful to learn but more complicated than expected. Please let me know what you think, I tried to avoid refactoring the actual worker. |
app/src/androidTest/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Outdated
Show resolved
Hide resolved
TimoPtr
left a comment
There was a problem hiding this comment.
I'm reluctant to use an instrumentation test here to test the logic. I would like to test the logic in a unit test instead. If Robolectric+mocks doesn't fit then we should extract the logic into a private function and unit test that.
Instrumentation test would be to test the behavior of the worker given some conditions (if we don't find a way to do it in unit tests).
|
I tried Robolectric but it doesn't seem to like that with the worker. All examples and code from others I could find also do integration tests. All logic is already in one private function in the worker which returns true/false, so could maybe be used, do you mean other adjustments or how would you test that? |
I guess that's the issue you have is because some stuff are initialized in the constructor or within super. You could do it in 2 ways
|
b267147 to
ba1ce7b
Compare
|
With more trying managed to adjust the tests, see the latest commit. Afterwards I rebased and force pushed to resolve a merge conflict before realizing I could have merged upstream into my branch instead, sorry. |
app/src/test/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Show resolved
Hide resolved
app/src/test/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Fixed
Show fixed
Hide fixed
app/src/test/kotlin/io/homeassistant/companion/android/websocket/WebsocketManagerTest.kt
Fixed
Show fixed
Hide fixed
- Use sentences in function names - Add HiltTestApplication - Merge more verify/coVerify into one block
TimoPtr
left a comment
There was a problem hiding this comment.
Thanks for the extra effort on the tests
Summary
Adjust how WebsocketManager checks whether to run to return early if conditions won't be met, instead of getting all possible info for all settings and only then returning. This should reduce the latest Android 16 QPR showing a 'blue location dot' because the app checks internal/external on screen on/off for the app, when there might not be a reason to do so.
Logs showed activity from ServerConnectionStateProviderImpl immediately after the WebsocketManager is started, which this change should avoid.
Includes a minor change for
!!usage to handle null instead.Checklist
Screenshots
n/a
Link to pull request in documentation repositories
n/a
Any other notes