Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Assert condition to check on both map orders in a flaky test in LogMessageTest to make it non-flaky.
Flaky test
https://github.com/bbelide2/hippo4j/blob/fec58a5ecaca0f749abcdf076f13d6f0ba6267b2/infra/common/src/test/java/cn/hippo4j/common/toolkit/logtracing/LogMessageTest.java#L81
Problem
Test
testToStringShouldPrintMessageAndAllKeyAndValuePairs
inLogMessageTest
is detected as flaky with the NonDex tool. The test failed with the following error:Root cause
In this test, 2 key-value pairs are set to the
kvs
field (ConcurrentHashMap) oflogMessage
object along with s string message. Then,logMessage.toString()
is called and the response is compared with hard-coded strings. logMessage.toString() converts the given map to string form along with some other processing. But, ConcurrentHashMap/HashMap may not maintain the order of elements. Therefore, when NonDex tests are run, the order of elements are shuffled and the output is reversed and incorrect thus making the test flaky.Key-values are inserted this way:
https://github.com/bbelide2/hippo4j/blob/fec58a5ecaca0f749abcdf076f13d6f0ba6267b2/infra/common/src/test/java/cn/hippo4j/common/toolkit/logtracing/LogMessageTest.java#L83-L84
Output is compared this way:
https://github.com/bbelide2/hippo4j/blob/fec58a5ecaca0f749abcdf076f13d6f0ba6267b2/infra/common/src/test/java/cn/hippo4j/common/toolkit/logtracing/LogMessageTest.java#L85
Fix
Usually, switching from HashMap to LinkedHashMap will make tests non-flaky because LinkedHashMap will preserve order of elements. But, we cannot change a ConcurrentHashMap to LinkedHashMap because LinkedHashMap is not thread-safe like ConcurrentHashMap. Therefore, I updated the assert statement to check the output for both the possible orders of elements.
This fix will not affect the code since the change is only made in tests.
How this has been tested?
Java: openjdk version "11.0.20.1"
Maven: Apache Maven 3.6.3
Command used -
Command used -
Command used -
NonDex tests passed after the fix.