Skip to content

Fix flaky test caused by .toArray()#403

Merged
wu-sheng merged 3 commits intoapache:mainfrom
bmk15897:flaky-fix
Dec 6, 2022
Merged

Fix flaky test caused by .toArray()#403
wu-sheng merged 3 commits intoapache:mainfrom
bmk15897:flaky-fix

Conversation

@bmk15897
Copy link
Contributor

@bmk15897 bmk15897 commented Dec 2, 2022

Description

Flaky Test found using NonDex by running the command -
mvn -pl apm-commons/apm-util edu.illinois:nondex-maven-plugin:1.1.2:nondex -Dtest=org.apache.skywalking.apm.util.ConfigInitializerTest#testInitialize

The logged failure -

[ERROR] Failures: 
[ERROR]   ConfigInitializerTest.testInitialize:70 arrays first differed at element [1]; expected:<[b]> but was:<[c]>

Investigation

The test fails at ConfigInitializerTest.testInitialize:70 with the error regarding a mismatch of elements of the expected array of Strings with the actual elements from the TestPropertiesObject.Level1Object.SET_STR_ATTR.toArray() on line 70. The SET_STR_ATTR is a SET and a set does not guarantee the order of its elements. According to the documentation, the toArray() function follows the iterator's order and the iterator returns elements in no particular order (unless this set is an instance of some class that provides a guarantee). This makes the test outcome non-deterministic. To fix this, the actual array needs to be sorted so that it can guarantee the order of its elements.
Similar fix is made for the line ConfigInitializerTest.testInitialize:71 to remove the flakiness.

Fix

Sorting the output returned by the toArray() function. This removes the non-determinism without needing any further changes and ensures that the flakiness of the test is removed.

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@bmk15897
Copy link
Contributor Author

bmk15897 commented Dec 2, 2022

Hi reviewers, is this fix satisfactory? If no, can you please suggest a better approach if you have it in mind?
Thank you.

@wu-sheng
Copy link
Member

wu-sheng commented Dec 2, 2022

Hi, thank you for digging deeply into our tests.

This test case actually doesn't measure the order of the set, we don't expect that. The test is just making sure the map field is initialized.
The case seems stable in most JDK, does the tool intentionally mix the order to produce unpredictability?

@bmk15897
Copy link
Contributor Author

bmk15897 commented Dec 2, 2022

Yes, it intentionally mixes the order to produce unpredictability which helps detect tests that can non-deterministically pass and fail when run on the same version of code.

@wu-sheng
Copy link
Member

wu-sheng commented Dec 2, 2022

OK. If you want to fix this, you could change to codes to compare the two HashSets. We don't require the order actually.

@bmk15897
Copy link
Contributor Author

bmk15897 commented Dec 2, 2022

Thanks for your feedback! I will make the necessary changes and update the pull request.

@bmk15897
Copy link
Contributor Author

bmk15897 commented Dec 5, 2022

Hello, I have updated the fix to just compare the two HashSets without caring about the order. Can you please review the change?

@wu-sheng wu-sheng added this to the 8.14.0 milestone Dec 6, 2022
@wu-sheng wu-sheng added the chore label Dec 6, 2022
@wu-sheng wu-sheng merged commit 9a62d72 into apache:main Dec 6, 2022
@wu-sheng
Copy link
Member

wu-sheng commented Dec 6, 2022

Thanks. Merged.

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.

2 participants