Skip to content

Add unit tests for utility functions#68

Merged
WhyPenguins merged 2 commits intothoth-tech:t2-2024from
matt439:add-utility-unit-tests
Oct 18, 2024
Merged

Add unit tests for utility functions#68
WhyPenguins merged 2 commits intothoth-tech:t2-2024from
matt439:add-utility-unit-tests

Conversation

@matt439
Copy link

@matt439 matt439 commented Sep 2, 2024

Description

There are currently no unit tests for the functions listed under 'Utilities' in the SplashKit documentation. I have created exhaustive tests for each of these functions apart from display_dialog as it is generates purely graphical content and would be better suited for sktests.

In the process of creating the tests, I have identified issues with replace_all, rnd and rnd(min, max). I can consistently cause an infinite loop in replace_all, and an unhanded exception in the two overloads of rnd. Keeping those tests in skunit_tests will cause it to fail. As a compromise, I have left the tests in place and commented them. My intention is to create separate pull requests to rectify the faulty functions. Once they are fixed, the tests can be uncommented.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Adding the new tests was achieved by running cmake -G "Unix Makefiles" . followed by make when in the /splashkit-core/projects/cmake directory.

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
  • My changes generate no new warnings
  • I have requested a review from ... on the Pull Request

Copy link

@Liquidscroll Liquidscroll left a comment

Choose a reason for hiding this comment

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

Looks good to me! All tests passed on WSL2.

However, I think that this should be updated to uncomment the relevant tests, and then ensure this is merged after these functions are fixed. This could occur later, so I'll approve this for now.

Copy link

@hugglesfox hugglesfox left a comment

Choose a reason for hiding this comment

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

Looks good but it might be nice to tag the tests better. For instance tagging all the string related tests with string, the random related tests with random, time related tests with time, resource tests as resource, etc.

Also want to just mention the idea of splitting each section into a separate file though as all the functions tested are in the same source (iirc) then keeping them all in the one file could make sense.

Copy link

@WhyPenguins WhyPenguins left a comment

Choose a reason for hiding this comment

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

This looks good! Just uncomment those tests and this should be good to merge 😃

@WhyPenguins
Copy link

Ah sorry one more thing, with the latest change to rnd the "min is 1 and max is 0" test doesn't pass since the logic has changed. Mind updating that test as well? Thanks!

@matt439
Copy link
Author

matt439 commented Oct 2, 2024

I uncommented the tests and fixed the min is 1 and max is 0 test case.

@WhyPenguins WhyPenguins changed the base branch from develop to t2-2024 October 18, 2024 11:54
@WhyPenguins WhyPenguins merged commit 77af8e6 into thoth-tech:t2-2024 Oct 18, 2024
@matt439 matt439 deleted the add-utility-unit-tests branch November 1, 2024 01:35
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.

4 participants