Skip to content

fix: flakiness in org.json.junit.JSONObjectTest#valueToString #2

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

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

Conversation

hofi1
Copy link
Owner

@hofi1 hofi1 commented Sep 19, 2023

Problem:

The string in the assertion can change, because the Map which is used to store the data in the JSONObject returns the data in non-deterministic order (the order of child elements in JSON strings do not matter, and JSON strings are equal regardless of the ordering of the elements on the same hierarchy level)
The flaky test was found by using the NonDex tool.

The flakiness was discovered on the following lines

assertTrue("jsonObject valueToString() incorrect",
JSONObject.valueToString(jsonObject).equals(jsonObject.toString()));

as well as

assertTrue("map valueToString() incorrect",
jsonObject.toString().equals(JSONObject.valueToString(map)));

Solution:

Changed the assert statement from a JUnit Assertion to a JSON Assertion, so the strings are not compared charwise but are compared the way as specified for JSON strings (not taking care of the order of the elements on the same level).
The JSONAssertion library was chosen because it provides general assertions to compare JSON strings without the need of writing complex JUnit Assertions, what would need a lot of work to convert the strings in a different data structure, a lot of boilerplate code as well as the chance of adding more errors than fixing.

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.

Reproduce:

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

@vulcanpint
Copy link

If you can expand the PR description to include reasons for importing org.skyscreamer dependancy to fix the flakiness and maybe call out the specific lines in the code in which you identified the flakiness, it would be great.

Apart from that, good to go!

@kavvya97
Copy link

I have some comments on the PR.Adding a dependency in pom.xml might not be recommended since Developers might run some security checks on the code. It would be great to remove the dependency or look for alternate dependency that already exists in the package

@zzjas
Copy link

zzjas commented Oct 5, 2023

Just be prepared that some developers might not like to add extra dependencies.

You can proceed to open a real PR. Once you open a real PR, please mark this tentative PR as Opened in your tentative_pr.csv file and also raise a PR to IDoFT marking this as Opened. Thanks!

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.

4 participants