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 UtilTest.testEnhanceResponseWithShippingAddressList #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sujishark
Copy link
Owner

@Sujishark Sujishark commented Nov 13, 2023

Description of changes:

This PR aims to fix a test:

com.amazon.pay.api.UtilTest.testEnhanceResponseWithShippingAddressList

This was found by using the NonDex tool.

REASON AND FIX

Encountered the following error while using NonDex:

com.amazon.pay.api.UtilTest.testEnhanceResponseWithShippingAddressList -- Time elapsed: 0.002 s <<< FAILURE!
org.junit.ComparisonFailure: expected:<...pingAddressList":[{"[city":null,"addressId":"amzn1.address.ABC","phoneNumber":"8910111213","postalCode":"123-4567","stateOrRegion":"MNO","county":null,"addressLine3":null,"name":"DEF","district":null,"countryCode":"JP","addressLine2":"JKL","addressLine1":"GHI"]}]}> but was:<...pingAddressList":[{"[postalCode":"123-4567","county":null,"addressId":"amzn1.address.ABC","phoneNumber":"8910111213","countryCode":"JP","addressLine3":null,"addressLine1":"GHI","addressLine2":"JKL","name":"DEF","stateOrRegion":"MNO","city":null,"district":null]}]}>
	at org.junit.Assert.assertEquals(Assert.java:117)
	at org.junit.Assert.assertEquals(Assert.java:146)
	at com.amazon.pay.api.UtilTest.testEnhanceResponseWithShippingAddressList(UtilTest.java:257)

The tests fails when the function testEnhanceResponseWithShippingAddressList() compares both strings (expectedCheckoutSessionResponse.RawResponse and actualCheckoutSessionResponseAfterEnhancing.RawResponse) with key-value pairs in it.

The JSON object is unordered since it uses Maps as their internal data structure. According to documentation,

"This class makes no guarantees as to the order of the map; in particular, it does not guarantee that the order will remain constant over time."

Due to this a non-deterministic order of key-value pairs are maintained and hence the assertEquals fails.

Assert.assertEquals(expectedCheckoutSessionResponse.getRawResponse(), actualCheckoutSessionResponseAfterEnhancing.getRawResponse());

One possible solution is to have those strings as JSONObjects and check using JSONObject.similar(), which compares if the contents of two JSON objects are the same, perhaps having a different order of attributes.

JSONObject expected = new JSONObject(expectedCheckoutSessionResponse.getRawResponse());
JSONObject actual = new JSONObject(actualCheckoutSessionResponseAfterEnhancing.getRawResponse());
Assert.assertTrue(expected.similar(actual));

Reproduce:

This was found by using the NonDex tool.

The following command can be used to replicate the failures and validate the fix:

mvn edu.illinois:nondex-maven-plugin:2.1.7-SNAPSHOT:nondex -Dtest=com.amazon.pay.api.UtilTest#testEnhanceResponseWithShippingAddressList

Environment:

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

No user-facing change.
No dependency upgrade.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zzjas
Copy link

zzjas commented Nov 14, 2023

You might first want to explain the cause (JSON is unordered) before calling it flaky.

The JSON object is unordered and generally uses Maps as their internal data structure.

I understand the point of introducing HashMap but even if an implementation does not use HashMap, JSON still doesn't guarantee order. So I'm not sure where the focus should be for the root cause. You can think more about it.

Following your discussion on Campuswire, you would also want to include the description of similar and maybe add the link to the doc.

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