[mqtt.ruuvigateway] Fix flaky itest and NPE on thing disposal (test teardown) - #21360
[mqtt.ruuvigateway] Fix flaky itest and NPE on thing disposal (test teardown)#21360ssalonen wants to merge 2 commits into
Conversation
The `testHappyFlow` itest failed upstream with
OFFLINE (BRIDGE_OFFLINE) ==> expected: <UNKNOWN> but was: <OFFLINE>
The tests assert the thing status updates of the Ruuvi thing by their
index, and the sequence they expect (INITIALIZING, UNKNOWN, ONLINE) only
holds when the MQTT broker bridge is ONLINE while the Ruuvi thing is
initialized. `AbstractMQTTThingHandler.initialize()` immediately reports
OFFLINE (BRIDGE_OFFLINE) when the bridge is OFFLINE at that moment, which
shifts all the following indices and makes the test fail for good.
- Wait for the broker bridge to reach ONLINE before creating Ruuvi
things, instead of only waiting for its handler to exist.
- Actually remove the things in `afterEach`. The previous
`things.stream().map(...)` had no terminal operation, so nothing was
removed and the handlers were disposed only later, when JavaOSGiTest
unregisters the volatile storage service, i.e. after the MQTT broker
had already been stopped. The broker connection of such a late disposed
bridge handler keeps on reconnecting in the background and leaks into
the next test. Things are now removed while the broker is still
running, and the test waits for the handlers to be disposed.
- Report the whole captured status update sequence when a status
assertion fails, so that this kind of failure can be diagnosed from the
build log.
Signed-off-by: Sami Salonen <sami.salonen@mailhub.fi>
`thenCompose(unsubscribeSuccessful -> null)` returns a null
`CompletionStage`, so the returned future always completed
exceptionally with a NullPointerException. Every disposal of a Ruuvi
thing therefore logged
unsubscription on disposal failed for mqtt:ruuvitag_beacon:...
java.util.concurrent.ExecutionException: java.lang.NullPointerException
from `AbstractMQTTThingHandler.dispose()`. Use `thenAccept` instead,
which gives the `CompletableFuture<Void>` the method is supposed to
return.
Signed-off-by: Sami Salonen <sami.salonen@mailhub.fi>
|
Not sure if you are aware, but the DCO is failing, could you add the sign-off message to the existing commits ? |
lsiepel
left a comment
lsiepel
left a comment
There was a problem hiding this comment.
My codex friend found these two comments. See below. The requested test is usefull but optional. The catch block seems essential to get stable results.
| @AfterEach | ||
| public void afterEach() throws Exception { | ||
| unregisterService(statusSubscriber); | ||
| removeThings(); |
There was a problem hiding this comment.
If removeThings() times out or a removal throws, afterEach() exits here and never stops mqttConnection, shuts down the scheduler, or calls super.afterEach(). That leaves the very broker resources this change is intended to contain leaking into subsequent tests (and can mask the original failure). Please preserve the required thing-before-broker removal order, but protect the remaining cleanup with try/finally so it always runs.
| String localTopic = topic; | ||
| if (localConnection != null && localTopic != null) { | ||
| return localConnection.unsubscribe(localTopic, this).thenCompose(unsubscribeSuccessful -> null); | ||
| return localConnection.unsubscribe(localTopic, this).thenAccept(unsubscribeSuccessful -> { |
There was a problem hiding this comment.
Could we add a focused regression test for this completion-stage fix? The updated integration-test teardown only waits until thing.getHandler() is null. AbstractMQTTThingHandler.dispose() catches the ExecutionException produced by the old thenCompose(... -> null) implementation, after which the handler is still detached, so the old bug would satisfy the new assertion. A test should verify that unsubscribeAll() completes normally (or that the subscription is actually removed).
3 participants
Fix attempt for #21349
The
RuuviGatewayTest#testHappyFlowitest fails intermittently on CI withThe tests assert the Ruuvi thing's status updates by absolute index, expecting
INITIALIZING, UNKNOWN, ONLINE("Waiting for initial data"). That sequence onlyholds if the MQTT broker bridge is ONLINE when the Ruuvi thing is initialized:
AbstractMQTTThingHandler.initialize()callsbridgeStatusChanged(getBridgeStatus()),which reports
OFFLINE (BRIDGE_OFFLINE)straight away when the bridge is OFFLINE.That extra update shifts every following index, and the bridge's default reconnect
delay (10 s, then 60 s) is longer than the
waitForAssertbudget, so the test cannever recover.
Two things in the test made it unable to cope:
createMqttBrokerBridge()only waited for the bridge handler to exist, neverfor the bridge to reach ONLINE, so Ruuvi things could be created against a bridge
that was still connecting or had just dropped.
afterEach()'s cleanup was a no-op:things.stream().map(thing -> thingProvider.remove(...))has no terminal operation. Things were therefore disposed only later, when
JavaOSGiTestunregisters the volatile storage service — after the Moquette brokerhad already been stopped — leaving bridge handlers whose connections keep
reconnecting in the background and leak into the next test.
Changes:
afterEach, while the broker is still running, andwait for the handlers to be disposed.
so this class of failure is diagnosable from the build log.
RuuviHandler.unsubscribeAll()usedthenCompose(unsubscribeSuccessful -> null),which returns a null
CompletionStage, so every disposal of a Ruuvi thing loggedunsubscription on disposal failed ... NullPointerExceptionfromAbstractMQTTThingHandler.dispose(). Changed tothenAccept.Developed with AI assistance.