Add resource bundle unit tests and fix bug in free_resource_bundle#131
Open
dijidiji wants to merge 2 commits intothoth-tech:mainfrom
Open
Add resource bundle unit tests and fix bug in free_resource_bundle#131dijidiji wants to merge 2 commits intothoth-tech:mainfrom
dijidiji wants to merge 2 commits intothoth-tech:mainfrom
Conversation
Adapted from Andrew Cain's test_bundles.cpp
ae67a25 to
3e64321
Compare
When loading, the IMAGE_RESOURCE case had the following check: if ( ! has_resource_bundle(line_name) ) return; This would always return (because it was checking resource bundles rather than bitmaps), bitmaps to be loaded but never be added to the list of bundled resources. This in turn resulted in free_resource_bundle never freeing that resource.
JPF2209
approved these changes
Sep 15, 2025
JPF2209
left a comment
There was a problem hiding this comment.
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 by having code that doesn't seem to need to deal with edge case
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 works well but it isn't usually a consideration for this type of change
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
48 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
Adapts Andrew Cain's resource bundle tests from sktest for skunit_tests. They were a good candidate for unit tests since it's simple to confirm the loading and unloading of resources.
Also fixes an issue discovered when porting these tests over:
When loading, the
IMAGE_RESOURCEcase had the following check:if ( ! has_resource_bundle(line_name) ) return;This would always return (because it was checking resource bundles
rather than bitmaps), bitmaps to be loaded but never be added to
the list of bundled resources.
This in turn resulted in free_resource_bundle never freeing that resource.
Type of change
How Has This Been Tested?
Testing Checklist
Ran both test executables to ensure the tests functioned the same way and made sure the output was as expected after the bug fix (bitmaps successfully unloaded).
Checklist