Add unit test for json_read_number (original PR #122 by @ConnorClancyDeakin)

[rory-cd]

Description

This PR continues on from #122, created by former contributor @ConnorClancyDeakin.

The original PR had 2 reviews. The last reviewer asked to include the relevant using directive at top of file:
using Catch::Matchers::WithinRel;

This PR addresses the request, amends a formatting issue (the last test case was incorrectly nested) and adjusts the sequence of free_json calls accordingly.

Original Description (#122)

The goal here was to implement additional testing so that the JSON_unit_test also checks that it can read different number types, other sections already test the INT reading so this just tests the double and float reading in order to make sure the number is correct and the type is correct

The test section is Json can be created and read with different number types, in the test case of json can be created and read.

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Built the test project as per unit testing instructions, running skunit_tests with all tests passed. Test was run with more specific output using ./skunit_tests [json_as_number] -s

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 commented my code in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have requested a review from ... on the Pull Request

[ralphweng2023]

Documentation

No documentation changes are required for unit tests, so this section is not applicable. The test case name is descriptive enough to understand its purpose.

Code Quality

The code addresses the reviewer's request to include the using Catch::Matchers::WithinRel; directive and fixes the nesting issue. However, there are several concerns:

1. Removal of free_json(person): The change removes the explicit free_json(person); call before free_all_json(). While free_all_json() should handle all JSON objects, this modifies the behavior of an existing test case. This change should be either explained in the PR description or reverted, with only free_all_json() kept if that's the intended cleanup pattern for this project.

2. Unnecessary braces: The new test case has an extra set of braces {} wrapping the test content that appears unnecessary and inconsistent with the style of other test cases in the file.

3. Missing newline at end of file: The file should end with a newline character following standard conventions.

4. Redundant test tags: The test uses [json_read_number][json_read_number_as_double] which seems redundant. Consider using just [json] for consistency with the existing test, or a single descriptive tag like [json_read_number].

Testing

The test covers the basic functionality of reading float and double values from JSON, which addresses the original issue. However:

1. Incomplete type verification: The original description states "this just tests the double and float reading in order to make sure the number is correct and the type is correct" (emphasis added), but the test only verifies the values, not the types. Consider adding assertions that verify the returned types are correct.

2. Limited test coverage: The test only covers successful cases. Consider adding edge cases such as:

   • Reading a non-existent key
   • Reading the wrong type (e.g., trying to read a string as a number)
   • Boundary values (very large/small numbers, precision limits)

3. Floating-point comparison: Using WithinRel with default epsilon is appropriate, but consider whether the default tolerance is suitable for your use case.

Suggested Changes

• Remove the unnecessary braces {} around the test content
• Add a newline at the end of the file
• Simplify test tags to [json] or a single appropriate tag
• Either revert the removal of free_json(person); or explain why it should be removed
• Consider adding type verification assertions to match the stated goal
• Consider adding edge case testing for more robust coverage

Minor formatting suggestion:

TEST_CASE("json can be created and read with different number types", "[json]")
{
    json number_types = create_json();

    json_set_number(number_types, "float", 21.2f);
    json_set_number(number_types, "double", 30.1);

    REQUIRE_THAT(json_read_number(number_types, "float"), WithinRel(21.2f));
    REQUIRE_THAT(json_read_number_as_double(number_types, "double"), WithinRel(30.1));
    
    free_json(number_types);
}

[rory-cd]

1. Removal of free_json(person): The change removes the explicit free_json(person); call before free_all_json(). While free_all_json() should handle all JSON objects, this modifies the behavior of an existing test case. This change should be either explained in the PR description or reverted, with only free_all_json() kept if that's the intended cleanup pattern for this project.

Agreed. I have added this line back in, to keep the PR related to this one test case. I will add a new ticket to fix this issue and investigate any similar occurrences, as I suspect the free_x functions have inconsistent usage across many of the test cases.

4. Redundant test tags: The test uses [json_read_number][json_read_number_as_double] which seems redundant. Consider using just [json] for consistency with the existing test, or a single descriptive tag like [json_read_number].

This was explicitly requested in the original PR. Tag usage appears to be inconsistent across all unit tests (project-wide) - many of the tests have function-specific tags for granular testing (e.g. [json_read_number] or [gpio_pin_value]), while some have generic tags like [json]. Both have valid uses (running all tests related to json vs. running all tests which include a particular function). For now I have added the generic [json] tag, but retained the function specific tags too. Assessing and updating the use of these tags to be consistent project-wide is a good cause for a new ticket.

1. Incomplete type verification: The original description states "this just tests the double and float reading in order to make sure the number is correct and the type is correct" (emphasis added), but the test only verifies the values, not the types. Consider adding assertions that verify the returned types are correct.

The returned type is guaranteed by the function signature. Therefore despite the original description mentioning this, I have left it out. I don't believe unit tests should be checking this particular issue, but please advise if I am incorrect about this.

2. Limited test coverage: The test only covers successful cases. Consider adding edge cases such as:

• Reading a non-existent key
• Reading the wrong type (e.g., trying to read a string as a number)

SplashKit JSON is permissive, and uses implicit conversions. For example the boolean true and string "1.0" will both be read as 1.0. EDIT: The wrong types should return 0.0. So I have added cases for reading the "wrong" type, but they expect a predictable value (0.0), rather than throwing an error. The same goes for reading non-existent keys - this should return 0.0.

All other changes have been made as requested. Thanks for the review!

[rory-cd]

Update: My understanding of the conversion from different data types was incorrect - these should always return 0.0, even a numeric string (e.g. "1.3"). I have removed these lines from the test. Please re-review.