Skip to content

Conversation

@ralphweng2023
Copy link

Description

Adds comprehensive automated unit tests for animation functions in SplashKit. The existing test_animation.cpp provides manual/interactive testing, but lacked automated unit tests that can be run as part of CI/CD pipelines.

This PR adds unit_test_animation.cpp which provides automated tests for:

Animation Script functions:

  • load_animation_script, free_animation_script, free_all_animation_scripts
  • has_animation_script, animation_script_named, animation_script_name
  • animation_count, has_animation_named, animation_index

Animation functions:

  • create_animation (multiple overloads with/without sound)
  • free_animation, assign_animation (multiple overloads)
  • animation_name, animation_current_cell, animation_ended
  • animation_entered_frame, animation_frame_time, animation_current_vector
  • update_animation, restart_animation

Edge case handling:

  • Null/invalid pointer handling for all getter functions
  • Invalid script file loading
  • Animation lifecycle testing (create → update → end → restart)

The tests use the existing kermit.txt animation script resource and follow the same patterns as other unit tests in the project.

Type of change

  • New tests

How Has This Been Tested?

Built the test project using CMake as per build instructions. All tests pass successfully.

Automated Testing:

cd projects/cmake
cmake --preset macOS
cmake --build build/

cd ../../bin
./skunit_tests "[animation],[animation_script]" --success
# Result: All tests passed (86 assertions in 9 test cases)

./skunit_tests --order rand
# Result: All tests passed (1349 assertions in 88 test cases)

Testing Checklist

  • Code compiles without errors
  • All 9 new animation test cases pass (86 assertions)
  • All existing tests still pass (88 total test cases, 1349 assertions)
  • Tests run independently in random order

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
  • My changes generate no new warnings
  • New tests have been added to prove the functionality works

Copy link

@rory-cd rory-cd left a comment

Choose a reason for hiding this comment

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

General Information

Type of Change:

  • New unit tests

This PR is a valuable addition, with unit tests covering a wide range of animation functions. Some assertions could be strengthened slightly, as noted with inline comments.

Optional: For consistency with other tests, change all occurrences of std::string to string, and add #include "types.h"

Code Quality

  • Repository: This Pull Request is made to the correct repository
  • Readability: The code is easy to read and follow
  • Maintainability: The code could be easily maintained or extended in the future

Functionality

  • Correctness: The code meets the requirements of the task
  • Impact on Existing Functionality: The impact on existing functionality been considered

Testing

  • Test Coverage: Unit tests provided for new or modified code
  • Test Results: All tests passed

Documentation

  • Documentation: Applicable internal documentation is updated and clear

Pull Request Details

  • PR Description: The problem being solved is clearly described
  • Checklist Completion: All relevant checklist items have been reviewed and completed

Comment on lines +3 to +5
*
* Tests for animation script and animation functions in SplashKit.
* Covers loading, creation, updating, and lifecycle of animations.
Copy link

Choose a reason for hiding this comment

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

Optional: Remove these lines for consistency - other unit tests do not contain a description.

SECTION("can get animation count")
{
int count = animation_count(script);
REQUIRE(count > 0);
Copy link

Choose a reason for hiding this comment

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

To make this assertion more robust, it could be more specific, e.g. REQUIRE(count == 14);

Comment on lines +91 to +92
int idx = animation_index(script, "WalkFront");
REQUIRE(idx >= 0);
Copy link

Choose a reason for hiding this comment

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

To make this assertion more robust, it could be more specific, for example:

int idx = animation_index(script, "Dance");
REQUIRE(idx == 8);

SECTION("can get current cell")
{
int cell = animation_current_cell(anim);
REQUIRE(cell >= 0);
Copy link

Choose a reason for hiding this comment

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

To make this assertion more robust, it could be more specific, e.g. REQUIRE(cell == 0);

SECTION("can get frame time")
{
float frame_time = animation_frame_time(anim);
REQUIRE(frame_time >= 0.0f);
Copy link

Choose a reason for hiding this comment

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

To make this section more robust, it could have an additional assertion, e.g. REQUIRE(frame_time <= 12.0f);. This would place an upper bound on the frame time.


float time_after = animation_frame_time(anim);
REQUIRE(time_after == 0.0f);
REQUIRE_FALSE(animation_ended(anim));
Copy link

Choose a reason for hiding this comment

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

You could add another assertion to strengthen the test here, for example REQUIRE(animation_current_cell(anim) == 0);

update_animation(anim);
}

float time_before = animation_frame_time(anim);
Copy link

Choose a reason for hiding this comment

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

time_before is never used. It should be utilised or removed.

update_animation(anim, 0.5f);

float frame_time = animation_frame_time(anim);
REQUIRE(frame_time > 0.0f);
Copy link

Choose a reason for hiding this comment

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

If the frame has a frame_time of 0, this assertion will fail. Consider >= rather than >.

This would also benefit from an upper bound, e.g. REQUIRE(frame_time <= 12.0f);

REQUIRE(anim != nullptr);

int dance_idx = animation_index(script, "Dance");
assign_animation(anim, dance_idx);
Copy link

Choose a reason for hiding this comment

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

There is no assertion here to test the function. This could be REQUIRE(animation_name(anim) == "dance"); or similar.

SECTION("animation_name returns empty for null")
{
disable_logging(WARNING);
REQUIRE(animation_name(nullptr) == "");
Copy link

Choose a reason for hiding this comment

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

This could be REQUIRE(animation_name(nullptr).empty()); for consistency with earlier usage of empty strings (line 190).

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.

3 participants