Skip to content

fix: fix flaky test in org.apache.helix.rest.server.TestResourceAccessor#3

Open
hofi1 wants to merge 1 commit into
masterfrom
fix/fix_flaky-test-org.apache.helix.rest.server.TestResourceAccessor
Open

fix: fix flaky test in org.apache.helix.rest.server.TestResourceAccessor#3
hofi1 wants to merge 1 commit into
masterfrom
fix/fix_flaky-test-org.apache.helix.rest.server.TestResourceAccessor

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#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
Copy Markdown

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!

@219sansim 219sansim 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.

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.

Comment thread helix-rest/src/test/java/org/apache/helix/rest/server/TestResourceAccessor.java Outdated
Comment thread helix-rest/src/test/java/org/apache/helix/rest/server/TestResourceAccessor.java Outdated
@hofi1

hofi1 commented Oct 13, 2023

Copy link
Copy Markdown
Owner Author

@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
Comment thread helix-rest/src/test/java/org/apache/helix/rest/server/TestResourceAccessor.java Outdated
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

@219sansim 219sansim 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.

LGTM

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

4 participants