[huesync] Scheduled runnable ended with an exception#20512
Draft
pgfeller wants to merge 5 commits intoopenhab:mainfrom
Draft
[huesync] Scheduled runnable ended with an exception#20512pgfeller wants to merge 5 commits intoopenhab:mainfrom
pgfeller wants to merge 5 commits intoopenhab:mainfrom
Conversation
…ssion tests for safe connection disposal Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Issue #19079 by preventing NoSuchElementException during scheduled Hue Sync status updates when handler lifecycle transitions cause incomplete/invalid update data, and adds regression tests around the new exception-handling behavior.
Changes:
- Update polling task now only invokes the update consumer on fully successful fetches and routes failures to the exception handler.
- Handler update processing no longer throws on missing update fields and
dispose()no longer throws when connection is absent. - Add regression tests for update-task exception contract and handler dispose/initialize safety.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncUpdateTask.java | Stop calling the consumer from finally; call it only on successful fetch; adjust logging/exception flow. |
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/handler/HueSyncHandler.java | Update consumer wiring, safer update handling, and dispose() no longer throws on empty connection. |
| bundles/org.openhab.binding.huesync/src/test/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncUpdateTaskTest.java | New regression tests validating consumer/exception-handler invocation rules. |
| bundles/org.openhab.binding.huesync/src/test/java/org/openhab/binding/huesync/internal/handler/HueSyncHandlerTest.java | New regression tests ensuring dispose() and basic lifecycle calls don’t throw. |
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/i18n/ResourceHelper.java | Javadoc author note updated. |
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncRegistrationTask.java | Javadoc author note updated. |
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/exceptions/HueSyncConnectionException.java | Javadoc author note updated. |
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncDeviceConnection.java | Javadoc author note updated. |
| bundles/org.openhab.binding.huesync/src/main/java/org/openhab/binding/huesync/internal/connection/HueSyncConnection.java | Javadoc author note updated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...sync/src/main/java/org/openhab/binding/huesync/internal/handler/tasks/HueSyncUpdateTask.java
Outdated
Show resolved
Hide resolved
...nding.huesync/src/main/java/org/openhab/binding/huesync/internal/handler/HueSyncHandler.java
Show resolved
Hide resolved
...nding.huesync/src/main/java/org/openhab/binding/huesync/internal/handler/HueSyncHandler.java
Outdated
Show resolved
Hide resolved
Comment on lines
+58
to
+68
| @Test | ||
| @DisplayName("dispose() should not throw NoSuchElementException when connection is empty") | ||
| void disposeWithEmptyConnection() throws Exception { | ||
| // Given: handler with empty connection (default state) | ||
| // When: dispose is called | ||
| // Then: should not throw NoSuchElementException | ||
| handler.dispose(); | ||
|
|
||
| // Test passes if no exception was thrown | ||
| assertThat(true, is(true)); | ||
| } |
There was a problem hiding this comment.
Test assertions like assertThat(true, is(true)) don’t validate behavior and will pass even if the method under test is never executed meaningfully. Replace these with JUnit assertions that directly express intent (e.g., assertDoesNotThrow(handler::dispose) / assertDoesNotThrow(() -> { ... })).
...g.huesync/src/test/java/org/openhab/binding/huesync/internal/handler/HueSyncHandlerTest.java
Show resolved
Hide resolved
Comment on lines
+70
to
+80
| @Test | ||
| @DisplayName("Multiple successive dispose() calls should be safe") | ||
| void multipleSideDisposeCalls() throws Exception { | ||
| // Given: handler initialized | ||
| // When: dispose is called multiple times | ||
| handler.dispose(); | ||
| handler.dispose(); | ||
|
|
||
| // Then: should not throw exception | ||
| assertThat(true, is(true)); | ||
| } |
There was a problem hiding this comment.
Typo in test method name: multipleSideDisposeCalls reads like “side” rather than “successive”. Rename to something like multipleSuccessiveDisposeCalls to match the display name and intent.
Comment on lines
+52
to
+53
|
|
||
| private HueSyncDeviceConnection connection = mock(HueSyncDeviceConnection.class); |
There was a problem hiding this comment.
This test class is @NonNullByDefault, but Mockito mock(...) fields are not annotated/suppressed for null analysis. In this codebase, tests commonly use @SuppressWarnings("null") on mocked fields to satisfy Eclipse/JDT null analysis (e.g., bundles/org.openhab.binding.evcc/src/test/java/.../EvccBaseThingHandlerTest.java:55-60). Add the appropriate suppression/annotations here to avoid null-analysis build failures.
Suggested change
| private HueSyncDeviceConnection connection = mock(HueSyncDeviceConnection.class); | |
| @SuppressWarnings("null") | |
| private HueSyncDeviceConnection connection = mock(HueSyncDeviceConnection.class); | |
| @SuppressWarnings("null") |
…mprove logging Route failures from the update task to the exception handler instead of propagating Optional-related exceptions. Include device uniqueId and the exception in log calls for better diagnostics. Make dispose() null-safe when the connection is absent. Add regression tests for update-task exception handling and handler lifecycle safety. Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
Replaced log calls that only used e.getMessage() (losing stack traces) with SLF4J calls that pass the exception object and provide contextual messages (initialization, disposal, registration parsing, unregistering). Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
Replace duplicated uniqueId in the trace with the device IP and pass the exception object to the logger (instead of e.getMessage()) so the full stacktrace is recorded. Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
Replace manual Mockito.mock(...) usage with field-level @mock injection using MockitoExtension. Add a class-level ScheduledExecutorService @mock and use it to inject BaseThingHandler.scheduler in the test. Remove redundant manual mock creation in @beforeeach. Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
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
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.
Summary
Fixes #19079 —
NoSuchElementException: No value presentthrown from the scheduled update task whenOptionalfields (connection,deviceInfo) become empty during handler lifecycle transitions (e.g.dispose()called while a scheduled task is mid-execution).