feat: add mqtt docker integration test #4556
Conversation
onelson
requested review from
nathanielc and
noramullen1
and removed request for
a team
| qos: 0, | ||
| retain: false, | ||
| clientid: "fluxtest", | ||
| ), |
There was a problem hiding this comment.
This looks pretty straightforward, re our conversation in slack, did you end up playing any games with retain etc to ensure the message was published? Reading this test looks like we are relying on the client to report success/failure instead of validating through the server. Is that correct?
This is likely good enough because we are using a standard client and we don't need to test if the server itself is working rather just that our client can successfully communicate with it.
There was a problem hiding this comment.
The issue I ran into is we have no API in flux to subscribe or otherwise read from MQTT, so there's no reasonable way to verify the outcome from within fluxtest alone. You'd need to be subscribes/watching in another process, or... something else.
Proving the dialer pooling behavior for that other issue might be a challenge, but as a manual exercise we can at least watch the logs on the server to look at client connects/disconnects.
For an automated test, the best I could come up with was to assert we got the true back from the client.
This test does show an error if MQTT isn't running, but I don't know what the rest of the failure space looks like 🤷
There was a problem hiding this comment.
Addendum: I did use the spawner script to launch the broker and was able to watch (via a cli subscriber) messages hit the queue while using mqtt.publish from the REPL. (video in slack)
|
@nathanielc any other questions I can answer or changes to be made here? |
Looks like when the script was refactored to allow for individual services to be launched, the "all" case was not well tested. As such, when we didn't name any services to launch nothing would launch. After fixing this issue, it seems the hdb container doesn't launch properly in the current state of the script. Something about having those docker commands in a function or the function being sent to the background disconnects it from the main tty for the script causing it to fail to perform the readiness check properly. In practice it seemed this meant hdb would report "ready" too soon and the seed data insert would fail, as well as the tests. For now, I'm skipping hdb. We'll have to loop back to try and find a good alternative approach.
This test currently fails because the client dependency isn't injected.
Sometimes when we run `docker stop` the engine can take a beat before the container is removed from the active list. The follow-up `docker rm` can therefore try to destroy the container while it's stopping (and `rm`'ing itself). In such a case, ignore the failure. If the container truly sticks around beyond these 2 commands, we'll see a different failure when we try to relaunch the container with an identical name.
onelson
force-pushed
the
onelson/mqtt-docker
branch
from
c888bcd
to
2d944d5
Compare
|
@jsternberg wanted to ping you (for when you get back from holiday). We ran into some test failures in nightly (and here) with the new "closer" dialer. #4568 was our best guess for what should happen to make the dialer safe to use in test, but you might have a different idea for where to handle this. Seemed like this call needs to happen after the rest of the deps have been injected while also as close to the top of the call stack as possible. |
Fixes #4518
The heft of this diff:
mqtt.publishMakefileso that the test can be included/excluded by various test make targetsAdditionally, there are fixes for the bash script which had previously undergone some refactors that left it working only for some cases (ref: #4518 (comment))
Another set of PRs will be needed to update the skip lists for various CI jobs outside of the flux repo.
Done checklist