Skip to content

fix: fix flaky test in org.apache.helix.rest.server.TestInstancesAccessor#2

Open
hofi1 wants to merge 1 commit into
masterfrom
fix/fix-flaky-test-org.apache.helix.rest.server.TestInstancesAccessor#testGetAllInstances
Open

fix: fix flaky test in org.apache.helix.rest.server.TestInstancesAccessor#2
hofi1 wants to merge 1 commit into
masterfrom
fix/fix-flaky-test-org.apache.helix.rest.server.TestInstancesAccessor#testGetAllInstances

Conversation

@hofi1

@hofi1 hofi1 commented Oct 3, 2023

Copy link
Copy Markdown
Owner

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

Fixes apache#2644

Description

This fix changes the assertion of the tests. Sets return the elements in a non-deterministic order, which means that this assertion is not correct, because it checks whether the collections contain the same elements in the same order. This leads to a flack test. To fix this problem, the assertion has been rewritten to check if the collections contain the same amount of elements as well as booth collections contain all values of the other collection.

This problem was found by the NonDex Engine – to reproduce run

mvn -pl helix-rest edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.apache.helix.rest.server.TestInstancesAccessor

Tests

There have been no tests added, one test condition was changed.

  • The following is the result of the "mvn test" command on the appropriate module:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 02:54 min
[INFO] Finished at: 2023-10-04T23:01:45-05:00
[INFO] ------------------------------------------------------------------------

@hofi1 hofi1 force-pushed the fix/fix-flaky-test-org.apache.helix.rest.server.TestInstancesAccessor#testGetAllInstances branch 2 times, most recently from 43ea8ac to 0c98411 Compare October 5, 2023 04:21

@harshith2000 harshith2000 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct the spelling.
Otherwise, this fix LGTM!

Comment thread helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java Outdated
@wuh3

wuh3 commented Oct 13, 2023

Copy link
Copy Markdown

The desciption is brief and clear. The fix is also optimal. It Looks Good To Me.

in org.apache.helix.rest.server.
TestInstancesAccessor#testGetAllInstances
@hofi1 hofi1 force-pushed the fix/fix-flaky-test-org.apache.helix.rest.server.TestInstancesAccessor#testGetAllInstances branch from 92638af to da3c274 Compare October 13, 2023 23:10
@hofi1

hofi1 commented Oct 13, 2023

Copy link
Copy Markdown
Owner Author

@wuh3 @harshith2000 thank you!

@zzjas

zzjas commented Oct 17, 2023

Copy link
Copy Markdown

You can consider if it makes sense to combine some of your PRs for helix into a larger one.

You can 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!

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.

flaky test org.apache.helix.rest.server.TestInstancesAccessor#testGetAllInstances

4 participants