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 flaky test in CollectUtilTest #1371

Merged
merged 1 commit into from
Dec 2, 2023
Merged

Conversation

bbelide2
Copy link
Contributor

@bbelide2 bbelide2 commented Dec 1, 2023

What's changed?

Use JSON_MAPPER.readTree() to compare two JSON strings instead of direct assertion to fix a flaky test

Flaky test case: org.dromara.hertzbeat.collector.util.CollectUtilTest.replaceCryPlaceholder

https://github.com/dromara/hertzbeat/blob/ce75254b8ec18422c48c40fe61354135d08b200d/collector/src/test/java/org/dromara/hertzbeat/collector/util/CollectUtilTest.java#L82

Problem

Test replaceCryPlaceholder in CollectUtilTest is detected as flaky with the NonDex tool. The test failed with the following error:

[ERROR] Failures: 
[ERROR]   CollectUtilTest.replaceCryPlaceholder:98 expected: <{"visible":false,"name":"张三"}> but was: <{"name":"张三","visible":false}>

[ERROR] Failures: 
[ERROR]   CollectUtilTest.replaceCryPlaceholder:111 expected: <[{"name":"张三","visible":false},{"name":"张三","visible":false}]> but was: <[{"visible":false,"name":"张三"},{"visible":false,"name":"张三"}]>

Root cause

In this test, two JSON strings are compared using Assert.assertEquals() in this part of the code:

https://github.com/dromara/hertzbeat/blob/ce75254b8ec18422c48c40fe61354135d08b200d/collector/src/test/java/org/dromara/hertzbeat/collector/util/CollectUtilTest.java#L92

and

https://github.com/dromara/hertzbeat/blob/ce75254b8ec18422c48c40fe61354135d08b200d/collector/src/test/java/org/dromara/hertzbeat/collector/util/CollectUtilTest.java#L105

Using Assert.assertEquals() will lead to flaky tests due to mismatch in the order of properties or if there are nested structures. In this particular test case, there is a mismatch in the order of properties and therefore the assertion failed making the test flaky when tested with NonDex.

Fix

JSON_MAPPER.readTree() for each JSON string while making the assertion to make the assertion order insensitive.

This change will not impact the code in anyways since the change is only done for the unit test.

How this has been tested?

Java: openjdk version "11.0.20.1"
Maven: Apache Maven 3.6.3

  1. Module build - Successful
    Command used -
mvn install -pl collector -am -DskipTests
  1. Regular test - Successful
    Command used -
mvn -pl collector test -Dtest=org.dromara.hertzbeat.collector.util.CollectUtilTest#replaceCryPlaceholder
  1. NonDex test - Failed
    Command used -
mvn -pl collector edu.illinois:nondex-maven-plugin:2.1.1:nondex -DnondexRuns=10 -Dtest=org.dromara.hertzbeat.collector.util.CollectUtilTest#replaceCryPlaceholder

NonDex test passed after the fix.

Checklist

  • I hereby agree to the terms of the HertzBeat CLA
  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

@tomsun28 tomsun28 added enhancement New feature or request good first pull request Good for newcomers bugfix labels Dec 2, 2023
@tomsun28 tomsun28 added this to the v1.4.3 milestone Dec 2, 2023
Copy link
Contributor

@tomsun28 tomsun28 left a comment

Choose a reason for hiding this comment

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

👍👍 LGTM!

@tomsun28
Copy link
Contributor

tomsun28 commented Dec 2, 2023

👍👍 @all-contributors please add @bbelide2 for code

@tomsun28 tomsun28 merged commit d1cf54e into apache:master Dec 2, 2023
2 checks passed
Copy link
Contributor

@tomsun28

I've put up a pull request to add @bbelide2! 🎉

tomsun28 pushed a commit that referenced this pull request Jan 16, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 9, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 9, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 10, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 10, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 11, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 11, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 11, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
tomsun28 pushed a commit that referenced this pull request Mar 11, 2024
Co-authored-by: balasukesh <bbelide2@fa23-cs527-002.cs.illinois.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix enhancement New feature or request good first pull request Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants