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 Md5UtilTest #1494

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

bbelide2
Copy link
Contributor

@bbelide2 bbelide2 commented Oct 5, 2023

Updated a flaky test case in Md5UtilTest to avoid making internal function calls thus making it not flaky.

Flaky test case: cn.hippo4j.common.toolkit.Md5UtilTest.assetGetTpContentMd5

Problem

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

[ERROR] Errors: 
[ERROR]   Md5UtilTest.assetGetTpContentMd5:58 » IllegalArgument [Assertion failed] - this expression must be true

Root cause

In this test, an object of class ThreadPoolParameterInfo is created and Md5Util.getTpContentMd5() is called with this object. Then the result is compared with a hardcoded string "md5Result"(ef5ea7cb47377fb9fb85a7125e76715d) here

Assert.isTrue(md5Result.equals(Md5Util.getTpContentMd5(threadPoolParameterInfo)));

During the execution, at one point here

return JSONUtil.toJSONString(threadPoolParameterInfo);

the object is converted to a JSON string using JSONUtil.toJSONString() which internally uses Jackson library ObjectMapper to convert the object to string. But, since the order of properties is not preserved, we get the JSON string with different order of properties each time and this is making this test flaky. Due to different JSON string values, final md5 string value is also different thus making this test flaky.

Fix

Ideally, a unit test should test the functionality of the method and not the functionalities of the methods called from inside this method. Therefore, we need to mock all the internal calls and test only the functionality of the method. I mocked ContentUtil class (static class) and tested only the Md5Util functionality. This way, the problem mentioned in the above root cause section is avoided and this test doesn't become flaky anymore.

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

NonDex test passed after the fix.

@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 (c9b6fc6) 34.34%.
Report is 16 commits behind head on develop.

❗ Current head c9b6fc6 differs from pull request most recent head 776dce6. Consider uploading reports for the commit 776dce6 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #1494      +/-   ##
=============================================
- Coverage      34.45%   34.34%   -0.11%     
- Complexity       846      848       +2     
=============================================
  Files            262      262              
  Lines           5918     5936      +18     
  Branches         554      554              
=============================================
  Hits            2039     2039              
- Misses          3686     3704      +18     
  Partials         193      193              
Files Coverage Δ
...essage/core/platform/WeChatSendMessageHandler.java 0.00% <0.00%> (ø)
...config/service/biz/impl/HisRunDataServiceImpl.java 0.00% <0.00%> (ø)

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

@magestacks magestacks merged commit cf854b3 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