[avmfritz] Fix itest - #21391
Conversation
There was a problem hiding this comment.
Pull request overview
This PR stabilizes and re-enables the AVMFritz discovery OSGi integration tests by preventing unintended real network authentication during test setup and by making the tests robust against asynchronous discovery callbacks introduced in openHAB Core.
Changes:
- Avoids initializing the real
BoxHandler(and therefore avoiding authentication/network access) during itest setup. - Re-enables the previously disabled discovery itests.
- Adds explicit waiting for asynchronous discovery listener callbacks to prevent race conditions in assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| itests/org.openhab.binding.avmfritz.tests/src/main/java/org/openhab/binding/avmfritz/internal/handler/AVMFritzThingHandlerOSGiTest.java | Simplifies test bridge/handler setup to avoid handler initialization and external network access. |
| itests/org.openhab.binding.avmfritz.tests/src/main/java/org/openhab/binding/avmfritz/internal/discovery/AVMFritzDiscoveryServiceOSGiTest.java | Re-enables tests and adds latch-based synchronization for async discovery callbacks. |
Suppressed comments (1)
itests/org.openhab.binding.avmfritz.tests/src/main/java/org/openhab/binding/avmfritz/internal/discovery/AVMFritzDiscoveryServiceOSGiTest.java:97
- The test registers a
DiscoveryListenerbut never unregisters it. With asynchronous notifications, callbacks can arrive after a test finishes and interfere with subsequent tests (e.g., counting down the next test's latch). Unregister the listener (and optionally dispose the service) in@AfterEach.
@AfterEach
public void cleanUp() {
discoveryResult = null;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wborn
left a comment
There was a problem hiding this comment.
AI reviewed this PR before manual review and found one issue in the updated integration tests.
The change correctly avoids initializing the real BoxHandler, removing the unnecessary network dependency that caused the original test hangs. The positive discovery assertions are also updated to account for asynchronous discovery callbacks.
The remaining issue is that the negative discovery tests still check discoveryResult immediately after onDeviceAdded(). Since discovery listener notification is asynchronous, those assertions can pass before an incorrectly generated discovery callback has executed.
A human maintainer review is still needed.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
AI found no further blocking issues in the current changes. A human maintainer review is still needed before merge.
wborn
left a comment
There was a problem hiding this comment.
Additional AI re-review:
The previous concern is resolved by using SameThreadExecutorService for the discovery service in the integration tests. Discovery listener callbacks now execute synchronously during these tests, making both the positive and negative assertions deterministic without fixed waits.
The simplified handler setup also avoids the network authentication that caused #16536 while retaining the state required by the discovery tests.
AI found no further issues in the current changes. A human maintainer should still review the PR before merge.
wborn
left a comment
There was a problem hiding this comment.
Thanks for making all the flaky tests more stable!
Signed-off-by: Leo Siepel <leosiepel@gmail.com> Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
The tests were previously disabled because they could block indefinitely. The available thread dump available in the linked issue is a few years old, but it showed that test setup initialized a real BoxHandler, causing an authentication request to fritz.box. This network connection was not needed for any of the discovery assertions and made the tests dependent on the local environment.
While re-enabling the tests, another source of instability became visible: discovery listeners are now notified asynchronously by openHAB Core, while the tests checked the result immediately. @wborn i remember that @wborn posted this somewhere in the last few weeks, but i couldn't find it again.
Anyway, this PR :
The existing discovery coverage and assertions remain unchanged.
Fixes: #16536