Add Unit tests for the hsb_color function #120

Broccoli811 · 2025-07-31T08:13:28Z

Description

The goal was to implement new test cases for a function called hsb_color from the color.cpp file to the unit_test_color.cpp file.

The Test case is named: "hsb_color converts HSB to RGB correctly". The associated test tag is "hsb_color"

Type of change

[✓] Adding new Test cases

How Has This Been Tested?

I have tested this using the documentation as a guide. CMake operations were used to test if certain colors were a certain correct value.

(Converting HSB of red to RGB of red should HAVE no values for green and blue)

Black and Gray colors were also tested to see if the 'brightness' and 'saturation' variables were changed

Testing Checklist

[✓] Tested with sktest
[✓] 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

dijidiji · 2025-08-04T07:37:16Z

coresdk/src/test/unit_tests/unit_test_color.cpp

    }
 }
+
+TEST_CASE("hsb_color converts HSB to RGB correctly", "[hsb_color]")


The docs specify that the input values of hsb_color must be between 0 and 1, so let's also check what happens with values outside of those bounds.

Unfortunately the docs don't specify what the behaviour should be, but from the code it looks like it will return the absolute value or the max for each component.

There's an opportunity here to also update the documentation with the expected behaviour. You could also go a step further and add handling for desired behaviour (for example, clamping values to the 0-1 range) and/or add logging when the function is called with invalid parameters. Feel free to post a new planner card related to this, even if you don't start on the task yourself. :)

dijidiji · 2025-08-04T07:40:06Z

coresdk/src/test/unit_tests/unit_test_color.cpp

+        REQUIRE(red_of(black) == 0);
+        REQUIRE(green_of(black) == 0);
+        REQUIRE(blue_of(black) == 0);


Check the alpha of the colour. HSB doesn't specify alpha so it should always be the max

Suggested change

REQUIRE(red_of(black) == 0);

REQUIRE(green_of(black) == 0);

REQUIRE(blue_of(black) == 0);

REQUIRE(red_of(black) == 0);

REQUIRE(green_of(black) == 0);

REQUIRE(blue_of(black) == 0);

REQUIRE(alpha_of(black) == 255);

dijidiji · 2025-08-10T11:08:21Z

coresdk/src/coresdk/color.cpp

    }

-    /// _returs a color from a combination of hue, saturation, and brightness.
+    /// _returns a color from a combination of hue, saturation, and brightness.


I'm not sure why some lines start with underscores in this file, so you should be good to tidy this comment up by removing them.

dijidiji · 2025-08-10T11:08:54Z

coresdk/src/coresdk/color.cpp

+        if (hue < 0.0) hue = 0.0;
+        if (hue > 1.0) hue = 1.0;
+
+        if (saturation < 0.0) saturation = 0.0;
+        if (saturation > 1.0) saturation = 1.0;
+
+        if (brightness < 0.0) brightness = 0.0;
+        if (brightness > 1.0) brightness = 1.0;


I think we can use std::clamp here for more concise code, since it's used in raspi_servo_driver.cpp

https://en.cppreference.com/w/cpp/algorithm/clamp.html

dijidiji · 2025-08-10T11:19:24Z

coresdk/src/test/unit_tests/unit_test_color.cpp

Out of bounds tests look good to me!

dijidiji

Looks good to me and your tests run fine, just remove these temp files and I'll approve :)

dijidiji · 2025-08-19T02:03:10Z

projects/cmake/Testing/Temporary/CTestCostData.txt

Remove these temporary files from the PR

dijidiji · 2025-08-19T02:03:16Z

projects/cmake/Testing/Temporary/LastTest.log

Remove these temporary files from the PR

dijidiji

Approved 👍

sam-stajnko

Hi @Broccoli811 ,
Nice job. Just a few changes before I approve this PR.

Remove the comments above the function definitions in color.cpp. These are already included in color.h which is the proper convention. I know you weren't responsible for these comments but they shouldn't have been there in the first place.
Change the warning log message in hsb_color to something clearer and more consistent with the rest of the Splashkit Library. I have added a comment with a suggestion.
Add REQUIRE conditions for the alpha channel in all of your test sections. I saw you did it for the first two, but it would be good if you could do all of them.

Apart from that, everything looks pretty good and I especially like the changes you made to hsb_color to incorporate clamping using std::clamp. One thing you might consider looking into for the future is implementing clamping for all of the color functions because I noticed that not all of them use clamping and if they do, they use a different method. If all of the color functions used std::clamp, I think the code would be much more consistent but don't feel obligated to do this. This is something you could create a card for and potentially do as a future task.

Anyways, sorry to be overly pedantic with my requested changes. They shouldn't take to long to fix and when you do, I can approve this PR.

Sincerely,
SAm Stajnko.

sam-stajnko · 2025-08-21T07:55:35Z

coresdk/src/coresdk/color.cpp

+    /// gets a color given its color components. Each of the components has
    /// a value between 0 and 1
    ///


Please delete this comment and all similar comments above the function definitions in color.cpp.

The declarations in color.h already have comments describing the function's purpose and therefore, the comments in color.cpp are redundant. If you look at other .cpp and .h files, you will see that this is the proper convention.

If there are any inline comments describing specific lines of code, you can leave these comments.

sam-stajnko · 2025-08-21T07:57:57Z

coresdk/src/coresdk/color.cpp

+    /// gets a color given its _r_g_b components. Each of the components has
    /// a value between 0 and 1.0f.
    ///


Please delete this comment and all similar comments above the function definitions in color.cpp.

The declarations in color.h already have comments describing the function's purpose and therefore, the comments in color.cpp are redundant. If you look at other .cpp and .h files, you will see that this is the proper convention.

If there are any inline comments describing specific lines of code, you can leave these comments.

sam-stajnko · 2025-08-21T07:58:37Z

coresdk/src/coresdk/color.cpp

+    /// gets a color given its _r_g_b components. Each of the components has
    /// a value between 0 and 1.
    ///


Please delete this comment and all similar comments above the function definitions in color.cpp.

The declarations in color.h already have comments describing the function's purpose and therefore, the comments in color.cpp are redundant. If you look at other .cpp and .h files, you will see that this is the proper convention.

If there are any inline comments describing specific lines of code, you can leave these comments.

sam-stajnko · 2025-08-21T07:59:13Z

coresdk/src/coresdk/color.cpp

+    /// returns a color from a combination of hue, saturation, and brightness.
    ///
-    /// @param hue, saturation, brightness: _values between 0 and 1
-    /// @returns _the matching color
+    /// @param hue, saturation, brightness: values between 0.0 and 1.0
+    /// @returns the matching RGB color
+    ///
+    /// @note: Values outside the range of 0.0 to 1.0 will be clamped to that range.
+    /// If clamping occurs, rounds the number to the nearest valid value (either 0.0 or 1.0), 
+    /// and the color may not be as expected and will be logged.
    ///


Please delete this comment and all similar comments above the function definitions in color.cpp.

The declarations in color.h already have comments describing the function's purpose and therefore, the comments in color.cpp are redundant. If you look at other .cpp and .h files, you will see that this is the proper convention.

If there are any inline comments describing specific lines of code, you can leave these comments.

sam-stajnko · 2025-08-21T08:11:44Z

coresdk/src/coresdk/color.cpp

+        brightness = std::clamp(brightness, 0.0, 1.0);
+
+        if (hue != original_hue || saturation != original_saturation || brightness != original_brightness) {
+            LOG(WARNING) << "Error in hsb_color received out-of-bounds input. Values have been clamped.";


Warning message could be improved. Maybe something like the following would be better:

"Attempting to create a color from out-of-bounds HSB components. Values should be between 0 and 1."

I think this aligns better with other warning logs in the Splashkit library, making it more consistent.

sam-stajnko · 2025-08-21T08:14:33Z

coresdk/src/test/unit_tests/unit_test_color.cpp

+    SECTION("pure green from HSB")
+    {
+        color green = hsb_color(1.0 / 3.0, 1.0, 1.0);
+        REQUIRE(red_of(green) == 0);
+        REQUIRE(green_of(green) == 255);
+        REQUIRE(blue_of(green) == 0);
+    }


Add a REQUIRE condition for the alpha channel.

sam-stajnko · 2025-08-21T08:15:00Z

coresdk/src/test/unit_tests/unit_test_color.cpp

+    SECTION("pure blue from HSB")
+    {
+        color blue = hsb_color(2.0 / 3.0, 1.0, 1.0);
+        REQUIRE(red_of(blue) == 0);
+        REQUIRE(green_of(blue) == 0);
+        REQUIRE(blue_of(blue) == 255);
+    }


Add a REQUIRE condition for the alpha channel.

sam-stajnko · 2025-08-21T08:15:22Z

coresdk/src/test/unit_tests/unit_test_color.cpp

+    SECTION("pure yellow from HSB")
+    {
+        color yellow = hsb_color(1.0 / 6.0, 1.0, 1.0);
+        REQUIRE(red_of(yellow) == 255);
+        REQUIRE(green_of(yellow) == 255);
+        REQUIRE(blue_of(yellow) == 0);
+    }


Add a REQUIRE condition for the alpha channel.

sam-stajnko · 2025-08-21T08:15:30Z

coresdk/src/test/unit_tests/unit_test_color.cpp

+    SECTION("pure cyan from HSB")
+    {
+        color cyan = hsb_color(0.5, 1.0, 1.0);
+        REQUIRE(red_of(cyan) == 0);
+        REQUIRE(green_of(cyan) == 255);
+        REQUIRE(blue_of(cyan) == 255);
+    }


Add a REQUIRE condition for the alpha channel.

sam-stajnko · 2025-08-21T08:15:47Z

coresdk/src/test/unit_tests/unit_test_color.cpp

+    SECTION("pure magenta from HSB")
+    {
+        color magenta = hsb_color(5.0 / 6.0, 1.0, 1.0);
+        REQUIRE(red_of(magenta) == 255);
+        REQUIRE(green_of(magenta) == 0);
+        REQUIRE(blue_of(magenta) == 255);
+    }


Add a REQUIRE condition for the alpha channel.

sam-stajnko

I have tested the changes made in this PR on my local machine and can confirm that they function as intended. The SKtests executable builds successfully and when ran, the unit tests for hsb_color return the expected outcome. I therefore have no issues approving this PR.

Nice work.
Regards,
Sam Stajnko.

Broccoli811 · 2025-08-31T10:20:08Z

I have discovered a critical mistake. I did not realize I was merging from my "main" branch instead of the intended branch, so I have copied over to a new branch with a proper name and have reset my main branch Here's the link to the new one:

#128

Broccoli811 changed the title ~~Main~~ Add Unit tests for the hsb_color function Aug 1, 2025

dijidiji suggested changes Aug 4, 2025

View reviewed changes

dijidiji reviewed Aug 10, 2025

View reviewed changes

dijidiji suggested changes Aug 19, 2025

View reviewed changes

dijidiji approved these changes Aug 19, 2025

View reviewed changes

sam-stajnko suggested changes Aug 21, 2025

View reviewed changes

sam-stajnko approved these changes Aug 25, 2025

View reviewed changes

Broccoli811 closed this Aug 31, 2025

Broccoli811 force-pushed the main branch from 8d4ae5f to 57e7cd6 Compare August 31, 2025 10:03

Add Unit tests for the hsb_color function #120

Add Unit tests for the hsb_color function #120

Uh oh!

Conversation

Broccoli811 commented Jul 31, 2025

Description

Type of change

How Has This Been Tested?

Testing Checklist

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dijidiji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dijidiji left a comment

Choose a reason for hiding this comment

Uh oh!

sam-stajnko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-stajnko left a comment

Choose a reason for hiding this comment

Uh oh!

Broccoli811 commented Aug 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sam-stajnko left a comment •

edited

Loading