Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky connackNotOk test by explicitly closing server #253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

williamlin0825
Copy link

Fix OD flakiness by closing Vert.x MQTT server after connackNotOk test in MqttClientConnectIT

This PR closes the MQTT server after the connackNotOk test to prevent it from remaining active. Previously, the server was still running after the test finished, causing other tests (e.g., testValidClientIdentifier in MqttServerClientIdentifierTest) to fail when they couldn't start a new server instance. This OD flakiness was detected by iDFlakies.

{
    "dts": [
        {
            "name": "io.vertx.mqtt.test.server.MqttServerClientIdentifierTest.testValidClientIdentifier",
            "intended": {
                "order": [
                    "io.vertx.mqtt.it.MqttClientConnectIT.connackNotOk"
                ],
                "result": "ERROR",
                "testRunId": "1730911774733-f1313efd-d311-41b9-b99e-e99db69d4f7c"
            },
            "revealed": {
                "order": [],
                "result": "PASS",
                "testRunId": "1730911782385-e91a008c-d3a6-49b5-bcdc-8136b7868a3f"
            },
            "type": "OD"
        }
    ]
}

@ppatierno ppatierno requested a review from vietj November 7, 2024 11:38
@tsegismont
Copy link
Contributor

Thanks for starting this PR @williamlin0825

I haven't seen a failing job in CI, can you tell us more about why this test in particular fails intermittently.

I'm not familiar with vertx-mqtt, but looking at the test class, it seems the Vertx and MqttClient objects should be stored in test class fields. Then, they could be properly closed in a tearDown method.

@williamlin0825
Copy link
Author

@tsegismont iDFlakies runs a bunch of tests with arbitrary sequences, and I found that many tests that require the MQTT server to start before them show a connection failure after this test runs.

The other tests in the MqttClientConnectIT class don't create a server instance themselves but rely on the existing container setup. Also, there is no tearDown method in the MqttClientBaseIT class, which is the base class of MqttClientConnectIT. Therefore, the bug occurs when the connackNotOk test starts the MQTT server but doesn't close it explicitly.

@tsegismont
Copy link
Contributor

I still don't understand why it doesn't fail in the CI environment.

Anyway, I think closing the server and Vert.x objects at the end of the test is not a good practice. Instead, it should be closed in a tearDown. Can you take care of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants