eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.32k stars 2.08k forks source link

Fixed non-idempotent test `DeploymentTest#testDeployClass` #5190

Closed kaiyaok2 closed 5 months ago

kaiyaok2 commented 6 months ago

Motivation:

The test io.vertx.core.DeploymentTest.testDeployClass is not idempotent and fails in the second run in the same JVM, because it pollutes state shared among tests. Specifically, it deploys a vertical but did not clean up ReferenceSavingMyVerticle.myVerticles after the deployment, hence the assertion assertEquals(deploymentId, myVerticle.deploymentID); can fail when myVerticles still contains verticals from the previous run. It shall be good to clean this state pollution so that other tests do not fail in the future due to the shared state polluted by this test.

Stacktrace of failure in the second run:

org.junit.ComparisonFailure: expected:<3[7b3fe25-12dd-4a61-b70d-734f577c4b0a]> but was:<3[e168fd5-0d9b-40ff-8801-d20b74837de8]>
    at org.junit.Assert.assertEquals(Assert.java:117)
    at org.junit.Assert.assertEquals(Assert.java:146)
    at io.vertx.test.core.AsyncTestBase.assertEquals(AsyncTestBase.java:360)
    at io.vertx.core.DeploymentTest.lambda$testDeployClass$126(DeploymentTest.java:1189)
    at java.base/java.lang.Iterable.forEach(Iterable.java:75)
    at io.vertx.core.DeploymentTest.lambda$testDeployClass$127(DeploymentTest.java:1188)

Proposed Fix

Clear ReferenceSavingMyVerticle.myVerticles before starting the individual tests.

Conformance:

ECA Submitted

tsegismont commented 6 months ago

Thanks for sharing your findings. Can you explain in which circumstances the same test would be executed twice in the same JVM ? Afaik, we've never seen this problem in CI.

kaiyaok2 commented 6 months ago

@tsegismont You're right that tests usually don't get re-executed in the same environment in the CI/CD pipeline. Certain mechanisms do support retrying tests that does not pass in the first run due to timeout / race conditions though. However, the major reason of executing a test twice in one environment is to ensure that all unit tests are self-contained.

This would help maintaining test isolation by ensuring that the state of the system under test is consistent at the beginning of each test, regardless of previous test runs. Fixing non-idempotent tests can help proactively avoid state pollution that may result in test order dependency (which could then hurt regression testing under test selection / prioritization / parallelization).

tsegismont commented 5 months ago

Certain mechanisms do support retrying tests that does not pass in the first run due to timeout / race conditions

Which mechanisms are you referring to?

kaiyaok2 commented 5 months ago

@tsegismont examples include the @RepeatedTest annotation introduced since JUnit 5, and the retryAnalyzer attribute of TestNG.

tsegismont commented 5 months ago

Thanks for providing the details. So this test is not marked for repeated execution. Should this change, the test would need to be adapted of course. But for now this is not necessary.