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: fix flaky test io.dropwizard.health.HealthCheckConfigValidatorTest#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered #1

Open
wants to merge 1 commit into
base: release/4.0.x
Choose a base branch
from

Conversation

hofi1
Copy link
Owner

@hofi1 hofi1 commented Sep 26, 2023

Problem:

The HealthCheckConfigValidator#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered test is flaky due to the use of the non-deterministic Set<> instance used in the class HealthCheckConfigValidator.
HealthCheckConfigValidator is intended to throw an exception with an error message containing specific words. Due to the use of a Set to generate the error message, this error message is not deterministic because the ordering, in which the elements are returned from the Set, is not deterministic. This results in a flaky test.
The flaky test was found by using the NonDex tool.

Solution:

Instead of checking if the error message contains an array with a specific ordering of the expected strings, check if the error message contains either the one or the other ordering (random ordering as intended by the use of Sets).

Result:

The test is deterministic and not flaky. This improves the quality of the test and reduces the time to search for the bug during future development.

Reproduce:
mvn -pl dropwizard-health edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=io.dropwizard.health.HealthCheckConfigValidatorTest#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered -DnondexSeed=933178

@hofi1 hofi1 changed the title fix: fix flakiness in test HealthCheckConfigValidator#startValidation… fix: fix flakiness in test HealthCheckConfigValidator#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered Sep 26, 2023
@Sujishark
Copy link

Using the contains() method repeatedly becomes inefficient as the number of values to check grows. Can you check on the alternative data structures, apart from 'Set', that you can suggest to preserve the order of elements ?

@ThugJudy
Copy link

Using the contains() method for every element of the list does not seem scaleable. Consider using another data structure that maintains order. It would be nice if you could add the nondex command used to reproduce the issue.

@hofi1 hofi1 changed the title fix: fix flakiness in test HealthCheckConfigValidator#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered fix: fix flakiness in test io.dropwizard.health.HealthCheckConfigValidatorTest#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered Sep 29, 2023
@hofi1 hofi1 force-pushed the bugfix/fix-flakiness-in-startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered branch from 231a752 to 0899fac Compare September 29, 2023 03:20
@hofi1
Copy link
Owner Author

hofi1 commented Sep 29, 2023

Hey @Sujishark and @ThugJudy ,
thanks for your good feedback, I appreciate it!
I updated the original comment and added the command to reproduce the bug.
Furthermore, I changed the two .contains calls to only use one.

I don't think it is right to force the order of the values in the string, because the ordering just doesn't matter.
But I am open for further suggestions and a discussion on that.

@hofi1 hofi1 changed the title fix: fix flakiness in test io.dropwizard.health.HealthCheckConfigValidatorTest#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered fix: fix flaky test io.dropwizard.health.HealthCheckConfigValidatorTest#startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered Oct 3, 2023
@zzjas
Copy link

zzjas commented Oct 7, 2023

Maybe you also want to assert that the message length. The test should fail if the message contains 2,3,4.

It's your call to spend more time addressing the comments or not, but we approve you to proceed to open a real PR. Once you open a real PR, please mark this tentative PR as Opened in your tentative_pr.csv file and also raise a PR to IDoFT marking this as Opened. Thanks!

…sShouldFailIfAHealthCheckConfiguredButNotRegistered
@hofi1 hofi1 force-pushed the bugfix/fix-flakiness-in-startValidationsShouldFailIfAHealthCheckConfiguredButNotRegistered branch from c2be776 to cb15b9a Compare October 13, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants