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 a flaky test #11771

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Fix a flaky test #11771

merged 4 commits into from
Oct 11, 2023

Conversation

dserfe
Copy link
Contributor

@dserfe dserfe commented Oct 10, 2023

This PR is to fix a flaky test org.apache.pinot.core.operator.filter.NotFilterOperatorTest#testNotOperator in module pinot-core.

Test failures

  • Run the following commands to reproduce test failures:
mvn -pl pinot-core edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.apache.pinot.core.operator.filter.NotFilterOperatorTest#testNotOperator
  • Then we get the following failures:
java.lang.AssertionError: expected [14] but found [0]

Root cause and fix

In line 38, expectedResult is defined as a HashSet, which does not guarantee the orders of its elements, so in line 46, when iterating the elements of a HashSet, the elements returned are not in a consistent order, which leads to test failures. To fix the flakiness, use LinkedHashSet instead of HashSet.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Good catch!

Comment on lines 39 to 40
Set<Integer> expectedResult = new LinkedHashSet();
expectedResult.addAll(Arrays.asList(0, 1, 4, 5, 6, 7, 8, 9, 11, 12, 13, 14, 19, 20, 25, 27, 29));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set<Integer> expectedResult = new LinkedHashSet();
expectedResult.addAll(Arrays.asList(0, 1, 4, 5, 6, 7, 8, 9, 11, 12, 13, 14, 19, 20, 25, 27, 29));
List<Integer> expectedResult = Arrays.asList(0, 1, 4, 5, 6, 7, 8, 9, 11, 12, 13, 14, 19, 20, 25, 27, 29);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the prompt reply! I have changed the patch based on the suggestion.

@Jackie-Jiang
Copy link
Contributor

Please fix the import

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

Merging #11771 (40fb944) into master (13c2901) will decrease coverage by 0.16%.
Report is 9 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #11771      +/-   ##
============================================
- Coverage     63.13%   62.98%   -0.16%     
  Complexity     1117     1117              
============================================
  Files          2342     2342              
  Lines        125916   125996      +80     
  Branches      19370    19378       +8     
============================================
- Hits          79501    79362     -139     
- Misses        40749    40968     +219     
  Partials       5666     5666              
Flag Coverage Δ
integration <0.01% <ø> (ø)
integration1 <0.01% <ø> (ø)
integration2 ?
java-11 14.44% <ø> (-48.63%) ⬇️
java-17 62.97% <ø> (+0.02%) ⬆️
java-20 62.94% <ø> (-0.04%) ⬇️
temurin 62.98% <ø> (-0.16%) ⬇️
unittests 62.98% <ø> (-0.16%) ⬇️
unittests1 67.06% <ø> (-0.17%) ⬇️
unittests2 14.47% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 24 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xiangfu0 xiangfu0 merged commit 7124a03 into apache:master Oct 11, 2023
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants