Skip to content

Conversation

@stelaukin
Copy link

@stelaukin stelaukin commented Jan 11, 2026

Description

Added unit test for the timer function from timers.cpp (documentation here). The new tests are named "create_timer and free_timer" and "start_timer and pause_timer".

This test includes sections:

  • create_timer
  • free_timer
  • start_timer
  • pause_timer

Fixes # (issue)

Type of change

  • Unit test addition

How Has This Been Tested?

Built the test project as per unit testing instructions, running skunit_tests with all tests passed.

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

@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

These unit tests cover the essential functionality of timers. To prevent potential hangs or memory issues, some adjustments are required to make the tests more robust.

Code Quality

  • Repository: Pull Request is made to the correct repository
  • Readability: The code is easy to read and follow
  • Maintainability: The code could easily be maintained or extended in the future
  • Robustness: The code structure could cause errors in certain circumstances

Functionality

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

Testing

  • Test Coverage: Unit tests are provided for new or modified code
  • Test Results: Ran skunit_test and skunit_test "[timers]" with all tests passed.

Documentation

  • Documentation: Inline 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

Required Changes

  • Remove potential infinite loop (see inline comment below)
  • Timer in second test case should be freed

Suggested changes

  • Use a single TEST_CASE with multiple SECTIONs to avoid duplicate setup/cleanup (see existing tests, e.g. unit_test_bitmap.cpp from line 78)
  • Test case descriptions should be plain sentences, e.g. "timer can be created and freed" rather than "create_timer and free_timer"

@stelaukin stelaukin requested a review from rory-cd January 16, 2026 09:10
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.

Thanks for the update, just a couple of points I think are worth addressing:

  • There seems to be a splashkit-core folder accidentally added inside the repo. This should be removed.
  • 2 more changes listed inline below.

Optional improvement:
My earlier comment about using multiple sections in a "single" test case was poorly worded and misleading. Ideally each test case should check one functionality. In this case creating, advancing, pausing, and freeing would each warrant one test case. Then SECTIONs can be used to isolate different aspects of each, e.g. "can free one timer", "can free multiple timers", etc. Our unit tests are a bit inconsistent with this anyway, so it's not worth holding up the PR, but something to consider as an improvement.

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.

2 participants