Skip to content

Conversation

@HildoYe
Copy link

@HildoYe HildoYe commented Dec 8, 2021

This PR fixes several flaky tests:

  1. There are several flaky tests in DynamicModelSerializationTest. The root cause of them is that equals() compares two HashMap<String, T> dynamicProperties in DynamicModel.
    In this PR, LinkedHashMap is used instead of HashMap to resolve the ordering issue. This solution considers 2 DynamicModels are same if their elements have the same insertion order. However, the equals() function should be modified to completely resolve the ordering issue.
  2. There are several flaky tests in RequestBuilderTest. In these tests multiple pairs of JSON strings are compared with each other.
    In this PR, JSONAssert library is used to ignore the order difference when comparing 2 JSON strings.
  3. There are a couple of flaky test in RequestUtilsTest. The root cause of them is the inconsistency when comparing 2 maps.
    In this PR, the comparations are changed to match the style of other tests in the same files while ignoring the ordering issue.
@CLAassistant
Copy link

CLAassistant commented Dec 8, 2021

CLA assistant check
All committers have signed the CLA.

@padamstx padamstx self-requested a review December 8, 2021 17:37
@HildoYe HildoYe changed the title Fix Flay Test: DynamicModelSerializationTest Fix Flay Tests Dec 8, 2021
@padamstx
Copy link
Contributor

padamstx commented Dec 8, 2021

Hi @HildoYe, could you perhaps provide more info about the environment where you are seeing test failures occur?
Which OS are you using and which version of Java, for example?
That would help me understand a bit more about how/when these failures might occur.

On our team, we typically use either Mac or Linux for development work along with Java 8, and we have not seen these "flakey" test results in the past.

@HildoYe
Copy link
Author

HildoYe commented Dec 8, 2021

Hi @HildoYe, could you perhaps provide more info about the environment where you are seeing test failures occur? Which OS are you using and which version of Java, for example? That would help me understand a bit more about how/when these failures might occur.

On our team, we typically use either Mac or Linux for development work along with Java 8, and we have not seen these "flakey" test results in the past.

The tests I've done are in Linux and Java 8. It can be reproduced with mvn -pl . edu.illinois:nondex-maven-plugin:1.1.2:nondex -Dtest=TESTNAME
List:

DynamicModelSerializationTest#testAddlPropsNull DynamicModelSerializationTest#testAlternatePropertyNames DynamicModelSerializationTest#testDynamicModelMethods DynamicModelSerializationTest#testModelAPFoo DynamicModelSerializationTest#testModelAPFooNullTypeToken DynamicModelSerializationTest#testModelAPInteger DynamicModelSerializationTest#testModelAPObject DynamicModelSerializationTest#testModelAPProtectedCtor DynamicModelSerializationTest#testModelAPString DynamicModelSerializationTest#testNullValues RequestBuilderTest#testBodyContent1 RequestBuilderTest#testBodyContent3 RequestBuilderTest#testBodyContentList RequestUtilsTest#testOmitWithNulls RequestUtilsTest#testPickWithNulls 
@padamstx
Copy link
Contributor

padamstx commented Dec 9, 2021

@HildoYe did you see the unit test failures only when using your edu.illinois:nondex-maven-plugin:1.1.2:nondex plugin?
The reason I ask is that we haven't seen any actual unit test failures on Mac or Linux.
Also a bit curious about the reason for your involvement here. Are you using an IBM Cloud SDK that depends on the java core library or are you simply experimenting with the java core to exercise your maven plugin?

Edit: I was able to run a couple of the tests with the "nondex" plugin and see the failures. Since these failures seem to occur only when the plugin is used, I think we'll hold off on merging in this PR for now. Thank you for bringing this to our attention though.

@padamstx padamstx added the do-not-merge Do NOT merge this PR label Dec 9, 2021
@padamstx padamstx changed the title Fix Flay Tests WIP: Fix Flay Tests Dec 9, 2021
@padamstx padamstx changed the title WIP: Fix Flay Tests WIP: Fix test failures that occur with "nondex" plugin Dec 9, 2021
@padamstx
Copy link
Contributor

closing without merging as we don't plan to merge this one in...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Do NOT merge this PR

3 participants