Skip to content

Conversation

@ConnorClancyDeakin
Copy link

Description

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

  • [✓] Adding additional tests

How Has This Been Tested?

In order to test this pull request you have to make the project, the cd into bin and run the command "./skunit_tests [json] -s" in your MSYS2 MINGW64 terminal, this will give you a read out of the unit test, the section labelled "Json can be created and read with different number types" will contain the changes made for the reading of float and double.

additionally you can test it by just running "./skunit_tests [json]" but this wont give you the same readout about the actual tests being conducted.

"json_read_number" should the number as a float and "json_read_number_as_double" should read it as double and the number should be the number same as the one set into the JSON

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

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.

Important

  • this code is good enough to approve but before the 2nd peer review, please change line 120 with Json in capitals since it doesn't line up with all other json's in the document and consistency is crucial for anything in SplashKit

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

@ConnorClancyDeakin
Copy link
Author

following the previous peer review I have changed the Json capitilization to json

Copy link

@dijidiji dijidiji left a comment

Choose a reason for hiding this comment

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

Just chiming in with some useful pointers for unit testing! I'll have to make a note of this in the unit testing guide.

@ConnorClancyDeakin
Copy link
Author

thank you for the suggestions, sorry about the delay github didn't notify me of them for some reason. The changes have been applied and they all work successfully with no issues.

Copy link

@dijidiji dijidiji left a comment

Choose a reason for hiding this comment

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

Sorry, experienced a similar issue to you and didn't receive a GitHub notification. To use WithinRel you'll need to add the relevant using directive (preferably after the includes at the top of the file):

using Catch::Matchers::WithinRel;

@rory-cd
Copy link

rory-cd commented Nov 26, 2025

This PR has been continued here.

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