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 JSONUtilTest #1495

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Conversation

bbelide2
Copy link
Contributor

@bbelide2 bbelide2 commented Oct 5, 2023

Use JSONAssert instead of Assert in JSONUtilTest to fix a flaky test

Flaky test case: cn.hippo4j.common.toolkit.JSONUtilTest.cn.hippo4j.common.toolkit.JSONUtilTest.assertToJSONString

Problem

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

Failed tests: assertToJSONString(cn.hippo4j.common.toolkit.JSONUtilTest): expected:<{"[id":1,"name":"foo1","foo":{"id":2,"name":"foo2"}]}> but was:<{"[foo":{"id":2,"name":"foo2"},"id":1,"name":"foo1"]}>

Root cause

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

Assert.assertEquals(EXPECTED_FOO_JSON, JSONUtil.toJSONString(EXPECTED_FOO));

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, JSON object(Foo) contains a nested object of the same type leading to difference in the order of properties thus making it a flaky test.

In this test, a hardcoded JSON string "EXPECTED_FOO_JSON" is compared with another JSON string created by JSONUtil.toJSONString(). The input to this method is an object of class Foo (test class present in JSONUtilTest).

Fix

Changed Assert.assertEquals() to JSONAssert.assertEquals() which is order insensitive.

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 infra/common -am -DskipTests
  1. Regular test - Successful
    Command used -
mvn -pl infra/common test -Dtest=cn.hippo4j.common.toolkit.JSONUtilTest#assertToJSONString
  1. NonDex test - Failed
    Command used -
mvn -pl infra/common edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=cn.hippo4j.common.toolkit.JSONUtilTest#assertToJSONString

NonDex test passed after the fix.

Alternate fix

Add @JsonPropertyOrder({"id", "name", "foo"}) annotation to Foo class so that the order of properties whenever an object of this class is converted to JSON string remains same. Since the order now always remains same, the JSON string always matches with our expected string thus fixing the flaky test. This change will not impact code since Foo class is only used in tests.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (e98b106) 34.45% compared to head (148be1c) 34.33%.
Report is 16 commits behind head on develop.

❗ Current head 148be1c differs from pull request most recent head 2c91797. Consider uploading reports for the commit 2c91797 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #1495      +/-   ##
=============================================
- Coverage      34.45%   34.33%   -0.13%     
- Complexity       846      847       +1     
=============================================
  Files            262      262              
  Lines           5918     5936      +18     
  Branches         554      554              
=============================================
- Hits            2039     2038       -1     
- Misses          3686     3705      +19     
  Partials         193      193              
Files Coverage Δ
...essage/core/platform/WeChatSendMessageHandler.java 0.00% <0.00%> (ø)
...config/service/biz/impl/HisRunDataServiceImpl.java 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@magestacks magestacks merged commit 79dc8f5 into opengoofy:develop Oct 6, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants