Skip to content

Conversation

@Broccoli811
Copy link

@Broccoli811 Broccoli811 commented Aug 31, 2025

Description

The goal was to implement missing test cases for a function called unit_test_json from the json.cpp file to the unit_test_json.cpp file.

The added TEST_CASE is named: "json_set_number can store different number types". With the test tag being: "[json_set_number]"

Type of change

  • [✓] Added new Test cases

How Has This Been Tested?

I have tested this using float and double data types. CMake operations were used to test if the function could retain positive/negative number types AND I have also tested if we overwrite existing keys/overwrite across data types

Testing Checklist

  • [✓] Tested with skunit_tests

Checklist

  • [✓] My code follows the style guidelines of this project
  • [✓] I have performed a self-review of my own code
  • [✓] I have made corresponding changes to the documentation
  • [✓] My changes generate no new warnings
  • [✓] I have requested a review from ... on the Pull Request

# Description

The goal was to implement missing test cases for a function called unit_test_json from the json.cpp file to the unit_test_json.cpp file.

The added TEST_CASE is named: "json_set_number can store different number types". With the test tag being: "[json_set_number]"

## Type of change

- [✓] Added new Test cases

## How Has This Been Tested?

I have tested this using float and double data types. CMake operations were used to test if the function could retain positive/negative number types AND I have also tested if we overwrite existing keys/overwrite across data types

## Testing Checklist

- [✓] Tested with sktest
- [✓] Tested with skunit_tests

## Checklist

- [✓] My code follows the style guidelines of this project
- [✓] I have performed a self-review of my own code
- [✓] I have made corresponding changes to the documentation
- [✓] My changes generate no new warnings
- [✓] I have requested a review from the teams chat on the Pull Request
Copy link

@JPF2209 JPF2209 left a comment

Choose a reason for hiding this comment

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

Summary

  • this code is good enough to approve for the 2nd peer review

Type of Change

  • This piece of code correctly identifies the changes made being adding additional tests

Code Readability

  • The code is very understandable and clear in what it needs to be comparing it to the other code.

Maintainability

  • As this is in the same style as the other code, it's quite maintainable being simple while doing the job.

Code Simplicity

  • This code is simple in it's execution following established design patterns and best practices

Edge Cases

  • The code deals with edge cases simply since the inner functionality of the test code manages this

Test Thoroughness

  • For all the key tests in this type of scenario, this covers it all and the code has been tested in multiple environments and it works.

Backward Compatibility

  • The code doesn't break existing functionality in the way it works.

Performance Considerations

  • Performance is usually more of a concern with the function itself but this works out well for performance.

Security Concerns

  • This code has no impact negatively or positively for security.

Dependencies

  • There are no new added dependencies.

Documentation

  • The documentation provided is through and simple to follow at the same time

Copy link

@sam-stajnko sam-stajnko left a comment

Choose a reason for hiding this comment

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

Hi @Broccoli811 ,
Nice job. The unit test you wrote for json_set_number is nicely organized and the sections provide good coverage of the different edge cases that could impact the function. One minor thing I did pick up on is that you created the json object outside the sections so that each section could reuse the same json object. It is fine to reuse the json object in this way; however, just note that the section named "can overwrite existing key" is kind of redundant because you have been inadvertently overwriting the same keys in previous sections. I don't think there is any need to change this because its probably good to have a section that explicitly tests the case when keys are overwritten.

As per the peer review guidelines, I have pulled this branch onto my local machine and the code builds successfully, with all tests for json_set_number passing.

I thereby approve this PR and send it onto mentor review.

Regards,
SAm Stajnko.

Copy link

@ayushindapure ayushindapure left a comment

Choose a reason for hiding this comment

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

I've added a comment regarding the use of == for float comparisons. It's safer to use Approx due to precision issues—for example: REQUIRE(json_read_number(j, "float_value") == Approx(3.14f));. This also applies to -120.38f, 0.0f, 1.23f, and 4.56f.

json_set_number(j, "float_value", float_test);

REQUIRE(json_has_key(j, "float_value"));
REQUIRE(json_read_number(j, "float_value") == 3.14f);

Choose a reason for hiding this comment

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

Currently, some float comparisons use == directly like the above line of code but actually this is risky because of floating-point precision issues. Try to replace with Approx maybe something like REQUIRE(json_read_number(j, "float_value") == Approx(3.14f));. would be great. Same applies for -120.38f, 0.0f, 1.23f, and 4.56f.

Copy link

@monicavtasmin monicavtasmin left a comment

Choose a reason for hiding this comment

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

approved

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.

5 participants