add unit testing json read number#122
Open
ConnorClancyDeakin wants to merge 7 commits intothoth-tech:mainfrom
Open
add unit testing json read number#122ConnorClancyDeakin wants to merge 7 commits intothoth-tech:mainfrom
ConnorClancyDeakin wants to merge 7 commits intothoth-tech:mainfrom
Conversation
JPF2209
approved these changes
Aug 5, 2025
There was a problem hiding this comment.
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
Author
|
following the previous peer review I have changed the Json capitilization to json |
dijidiji
suggested changes
Aug 28, 2025
dijidiji
left a comment
There was a problem hiding this comment.
Just chiming in with some useful pointers for unit testing! I'll have to make a note of this in the unit testing guide.
Co-authored-by: Daniel Carroll <6995669+dijidiji@users.noreply.github.com>
Co-authored-by: Daniel Carroll <6995669+dijidiji@users.noreply.github.com>
Co-authored-by: Daniel Carroll <6995669+dijidiji@users.noreply.github.com>
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. |
dijidiji
suggested changes
Sep 22, 2025
dijidiji
left a comment
There was a problem hiding this comment.
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;
8 tasks
|
This PR has been continued here. |
monicavtasmin
approved these changes
Jan 18, 2026
8 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
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
Checklist