Add unit test for function bitmap_filename #137
Conversation
There was a problem hiding this comment.
Description
This update adds a new unit test for the bitmap_filename function in images.cpp.
The test is named "bitmap filename can be retrieved" and checks three cases:
- A loaded bitmap returns the correct filename
- A null bitmap returns an empty string
- A newly created bitmap returns an empty string
The function currently returns a full file path.
For the comparison, the test takes only the last part of the path.
A rename to bitmap_filepath could help long-term clarity, but that is outside this change.
Type of change
- Unit test addition
How Has This Been Tested?
The test suite was built and run using the normal unit test flow.
skunit_tests completed with all tests passing.
Output strings were also printed to confirm each value manually.
Testing Checklist
- Tested with
skunit_tests
Checklist
- Code follows project style
- Self-review completed
- Comments added where helpful
- Documentation not required for this update
- No new warnings
- Reviewer will be added when opening PR
Files Modified
- Updated:
unit_test_bitmap.cpp— added new sections testingbitmap_filename
Additional Notes
No API changes or behaviour changes included.
| // Extract filename | ||
| std::string filepath = bitmap_filename(bmp); | ||
| size_t idx = filepath.size() - expected_filename.size(); | ||
| std::string filename = filepath.substr(idx, expected_filename.size()); |
There was a problem hiding this comment.
Unsafe substring extraction
This code has a potential unsigned integer underflow if filepath is shorter than expected_filename:
size_t idx = filepath.size() - expected_filename.size(); // Can underflow!
std::string filename = filepath.substr(idx, expected_filename.size());I would suggest do it this way:
std::string filepath = bitmap_filename(bmp);
REQUIRE(filepath.size() >= expected_filename.size());
std::string filename = filepath.substr(filepath.size() - expected_filename.size());
REQUIRE(filename == expected_filename);
Peer Review SummaryThanks for this contribution! I've reviewed the PR against the SplashKit peer review checklist. Overall, the unit tests are well-structured and follow the existing conventions, but there are a couple of issues that need to be addressed before approval. ✅ Code Quality
✅ Functionality
✅ Testing
|
Adds requirement ensuring the filepath being read exceeds or equals the length of the expected filename. This removes the potential for integer underflow.
|
Thanks @ralphweng2023, nice catch! I have updated to address the issue. I've followed your suggestion, but used an |
rory-cd
changed the title
3 participants
Description
Adds unit test for the
bitmap_filenamefunction from images.cpp (documentation here). The new test is named "bitmap filename can be retrieved".This test includes sections covering the following cases:
Note: Although the function uses the term filename, it actually returns the full file path. So in the first section, the test extracts the file name from the full path before making its comparison. It may be worth renaming the function to
bitmap_filepathand adding a newbitmap_filenameto make this clearer for the end user, but that is beyond the scope of this update.Type of change
How Has This Been Tested?
Built the test project as per unit testing instructions, running skunit_tests with all tests passed. The returned strings were also manually verified by outputting each using
std::cout.Testing Checklist
Checklist