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 replaceSmilingPlaceholder #1373

Merged
merged 1 commit into from
Dec 2, 2023

Conversation

bbelide2
Copy link
Contributor

@bbelide2 bbelide2 commented Dec 2, 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.replaceSmilingPlaceholder

https://github.com/dromara/hertzbeat/blob/7f131a179089a3a7fdd530913826c0c28e0cb5fb/collector/src/test/java/org/dromara/hertzbeat/collector/util/CollectUtilTest.java#L114

Problem

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

[ERROR] Failures: 
[ERROR]   CollectUtilTest.replaceSmilingPlaceholder:123 expected: <{"visible":false,"name":"张三"}> but was: <{"name":"张三","visible":false}>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

[ERROR] Failures: 
[ERROR]   CollectUtilTest.replaceSmilingPlaceholder:136 expected: <[{"name":"张三","visible":false},{"name":"张三","visible":false}]> but was: <[{"visible":false,"name":"张三"},{"visible":false,"name":"张三"}]>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

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/7f131a179089a3a7fdd530913826c0c28e0cb5fb/collector/src/test/java/org/dromara/hertzbeat/collector/util/CollectUtilTest.java#L123

and

https://github.com/dromara/hertzbeat/blob/7f131a179089a3a7fdd530913826c0c28e0cb5fb/collector/src/test/java/org/dromara/hertzbeat/collector/util/CollectUtilTest.java#L136

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#replaceSmilingPlaceholder
  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#replaceSmilingPlaceholder

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 the enhancement New feature or request label 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 tomsun28 merged commit 29d7a5f into apache:master Dec 2, 2023
2 checks passed
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
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants