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

[#1878] Updating SystemTestUtil::assertJson to compare Json objects instead of line-by-line analysis #2087

Merged

Conversation

asdfghjkxd
Copy link
Contributor

Fixes #1878

Proposed commit message

`SystemTestUtil::assertJson` currently checks if two JSON files are
equal by comparing the file contents line by line rather than by
converting the files directly into JSON objects, which can then be
directly compared using the built-in `equals` methods for
`JsonElement`.

This process also does not automatically check if the file is indeed a
JSON file, and implicitly assumes that the input paths are paths to
JSON files. This may cause expected behaviours if a user accidentally
or intentionally includes a path to a non-JSON file within the method
calls.

Let's move to refactor the code, and utilise the built-in methods for
`JsonElement` objects to check for the equality of JSON objects.

Other information

The original issue alludes to the use of JsonObject in the fixes, however, while trying to get the system test cases to run, it appears that this method is to be used on JsonObject and JsonArray as well. JsonElement was hence chosen as a suitable class that encapsulates the information of the parsed JSON objects, while allowing for equality of the JSON objects to be checked.

@asdfghjkxd asdfghjkxd requested a review from a team January 21, 2024 06:07
@asdfghjkxd asdfghjkxd self-assigned this Jan 21, 2024
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is strictly necessary, but you might want to put both FileReader instantiations in the try () block so that they will automatically close. Otherwise, LGTM!

@asdfghjkxd
Copy link
Contributor Author

That's a good suggestion! I will look into moving FileReader into the try-with-resources clause!

@asdfghjkxd
Copy link
Contributor Author

@jonasongg Just an update, but after having reviewed the code, it seems that both instances of FileReader are instantiated within the try-with-resources clause. Both FileReaders should close after the assertion is executed!

@asdfghjkxd asdfghjkxd requested a review from a team January 30, 2024 05:39
Copy link
Contributor

@sopa301 sopa301 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gok99 gok99 removed the s.Ongoing label Jan 31, 2024
Copy link
Contributor

@gok99 gok99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gok99 gok99 requested a review from a team January 31, 2024 02:01
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice job

@ckcherry23 ckcherry23 merged commit 30ef450 into reposense:master Feb 6, 2024
10 checks passed
Copy link
Contributor

github-actions bot commented Feb 6, 2024

The following links are for previewing this pull request:

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.

Updating SystemTestUtil::assertJson to compare Json objects instead of line-by-line analysis
7 participants