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 in org.apache.helix.rest.server.TestResourceAccessor #3

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

Conversation

hofi1
Copy link
Owner

@hofi1 hofi1 commented Oct 3, 2023

Issues

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

Fixes apache#2643

Description

This fix changes the flaky assertions of the tests in the class org.apache.helix.rest.server.TestResourceAccessor.
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 was changed in the library.)This leads to a flaky test. To fix this problem, the assertion has been rewritten to check if the collections contain the same amount of elements as well as both collections contain all values of the other collection.

The flaky test has been found by using the NonDex tool – to reproduce run

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

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-04T22:44:58-05:00

@hofi1 hofi1 force-pushed the fix/fix_flaky-test-org.apache.helix.rest.server.TestResourceAccessor branch 2 times, most recently from 346afa2 to 3216a09 Compare October 5, 2023 04:23
@deekshacheruku
Copy link

The fix looks good to me. But I guess you can add few more details to the PR description like linking which test is fixed and may be images. Good Work!

Copy link

@219sansim 219sansim left a comment

Choose a reason for hiding this comment

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

General comments on your PR description summary-

  1. Spelling error for 'flaky' - 'flack'
  2. Give more context about NonDex tool and flaky tests in general
  3. Point the developers to relevant resources (java docs) to understand that HashMap order is not guaranteed.
  4. Explain that AssertEquals checks ordering as well
  5. I don't think you would need to include 'mvn test' command output as the developers would have automated pipelines to check it.

@hofi1
Copy link
Owner Author

hofi1 commented Oct 13, 2023

@219sansim thanks for the great feedback!
the mvn test command output is required by the owner of the repo

@hofi1 hofi1 force-pushed the fix/fix_flaky-test-org.apache.helix.rest.server.TestResourceAccessor branch from e1050de to 3517468 Compare October 13, 2023 21:59
in org.apache.helix.rest.server.
TestResourceAccessor#testGetResources
@hofi1 hofi1 force-pushed the fix/fix_flaky-test-org.apache.helix.rest.server.TestResourceAccessor branch from b8f5c20 to a5d25cf Compare October 14, 2023 21:16
Copy link

@219sansim 219sansim left a comment

Choose a reason for hiding this comment

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

LGTM

@zzjas
Copy link

zzjas commented Oct 17, 2023

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 in org.apache.helix.rest.server.TestResourceAccessor#testGetResources
4 participants