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: flakiness in org.json.junit.XMLTest#testIndentComplicatedJsonObjectWithArrayAndWithConfig #798

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

hofi1
Copy link
Contributor

@hofi1 hofi1 commented Oct 14, 2023

Problem:

In this test, the contents of an XML file and a JSON file is getting parsed and compared. But XML does not specify the order of the elements in the String, what means that the actual and the expected string cannot be directly compared for equality.
The flaky test was found by using the NonDex tool.

assertEquals(expected.toString(), actualString);

Solution:

To fix this assertion without the import of a new XMLAssertion library, just like in other PRs discussed, the XML String gets converted to a JSON Object and afterward compared for similarity. This is needed due to the fact that XML Objects do not offer a similarity() method.

assertTrue(XML.toJSONObject(expected.toString()).similar(XML.toJSONObject(actualString)));

Result:

The test is deterministic and not flaky. This improves the quality of the test and reduces the time to search for the bug during future development, as well as the need for reruns of the pipeline.

Reproduce

mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.json.junit.XMLTest#testIndentComplicatedJsonObjectWithArrayAndWithConfig

@stleary
Copy link
Owner

stleary commented Oct 15, 2023

What problem does this code solve?
Fixes a potential ordering problem in a unit test by replacing the string compare with JSONObject.similar()

Risks
Low

Changes to the API?
N/A

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No, it fixes a potential break in unit tests

Was any code refactored in this commit?
No

Review status
APPROVED

Starting 3-day comment window

@stleary stleary merged commit d677a99 into stleary:master Oct 17, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants