Add more unit tests #212

soswow · 2025-10-19T01:55:11Z

Description

Adding more tests

Checklist:

[x I have read the contribution guidelines.
I have updated the documentation, if applicable. (Check if there is no
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
I have ensured that the change is tested somewhere in the testsuite
(adding new test cases if necessary).
My code follows the prevailing code style of this project. If I haven't
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.

soswow · 2025-10-19T01:56:00Z

src/bindings/CMakeLists.txt

-    target_compile_options ( nanobind-static    PRIVATE -Wno-error=zero-length-array )
-    target_compile_options ( rawtoaces_bindings PRIVATE -Wno-error=zero-length-array )
+    target_compile_options ( nanobind-static    PRIVATE -Wno-error=zero-length-array -Wno-zero-length-array )
+    target_compile_options ( rawtoaces_bindings PRIVATE -Wno-error=zero-length-array -Wno-zero-length-array )


without this it was giving bunch of warning for stuff coming from python binding library headers

could you try if suppressing the warnings locally around the nanobind includes in rawtoaces_bindings would work?

I don't know what that means, tbh. Can you expand? Don't forget, not a native c++'er here

ignore that, it is minor. I may do that later separately

soswow · 2025-10-19T01:56:34Z

src/rawtoaces_core/rawtoaces_core.cpp

    {
        std::cerr << "ERROR: camera needs to be initialised prior to calling "
                  << "SpectralSolver::calculate_WB()" << std::endl;
+        // TODO: Should here be return false?


@antond-weta is that on purpose? If not, I'll make a change. All other similar places (There are two more we return false)

not on purpose, please fix. thanks.

soswow · 2025-10-19T01:56:56Z

src/rawtoaces_util/image_converter.cpp


        if ( !success )
        {
+            // TODO: Potentially remove, since this code path is not reachable due to camera lookup success in the previous step.


calling @antond-weta to decide if I understand that right

please replace with assert(success);

codecov-commenter · 2025-10-19T01:58:05Z

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.81%. Comparing base (1b86a2f) to head (9ec91f3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/rawtoaces_util/image_converter.cpp	0.00%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
+ Coverage   72.56%   79.81%   +7.25%     
==========================================
  Files          10       11       +1     
  Lines        2114     2150      +36     
  Branches      317      325       +8     
==========================================
+ Hits         1534     1716     +182     
+ Misses        580      434     -146

Files with missing lines	Coverage Δ
src/rawtoaces_core/rawtoaces_core.cpp	`87.90% <100.00%> (+1.83%)`	⬆️
src/rawtoaces_util/image_converter.cpp	`68.58% <0.00%> (+13.83%)`	⬆️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b86a2f...9ec91f3. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

soswow · 2025-10-19T02:00:26Z

tests/test_image_converter.cpp

 }

+/// Tests that prepare_transform_spectral fails when no camera manufacturer information is available (should fail)
+void test_missing_camera_manufacturer()


I was not able to change .dng we have such that it has no make 🤷 no matter what I did with exiftools, new .dng always has "DNG" as a make. Hence I couldn't make this test go through main gate like other ones.

antond-weta

looks good

soswow · 2025-11-13T20:28:18Z

build_scripts/vcpkg.json

        "ceres",
        "nlohmann-json",
-        "openimageio",
+        { "name": "openimageio", "features": [ "libraw" ] },


In previous windows build dependency was brought in as:

openimageio:[email protected]#1

with this change it is now

openimageio[core,libraw]:[email protected]#1

Windows tests are green now.
Previously, when I was testing on local window machine the problem was that oiio I had didn't have libraw plugin enabled when I was trying to do convertion as part of a test. So, this most likely what fixed it.

soswow · 2025-11-13T21:30:09Z

cmake/modules/Findlibraw.cmake

My LLM convinced that this change is required. Though I am not 100% understrand what's going on here and I Am not convinced it is required. I feel like vcpkg.json change is where the problem was.

soswow · 2025-11-17T11:10:44Z

~~I had this realisation - all this stuff I did with running executable directly via _open (which is good from end-to-end persepective) will not count into coverage counter =\ most likely~~

Actually it's all good seems like

antond-weta · 2025-11-20T19:39:58Z

tests/test_image_converter.cpp

+
+    // Use the DNG file with camera make but no model
+    std::string test_file =
+        "../../tests/materials/blackmagic_cinema_camera_cinemadng_no_model.dng";


Adding a 5mb image file for testing just this small use case seems a bit excessive to me. I would suggest doing this: let's skip this part in the end-to-end tests. We have a configure() method which takes an ImageSpec instead of a file path. We can try using that to modify the ImageSpec for testing purposes: load a file into an ImageBuf, grab its ImageSpec, delete the camera model atribute, feed to configure(). This way we could test all sorts of bad metadata use cases.

antond-weta

LGTM

Signed-off-by: Aleksandr Motsjonov <[email protected]>

…oduce a test utility wrapper that allow capturing of stderr buffer Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow commented Oct 19, 2025

View reviewed changes

antond-weta approved these changes Oct 19, 2025

View reviewed changes

soswow commented Nov 13, 2025

View reviewed changes

soswow requested a review from antond-weta November 20, 2025 11:47

soswow marked this pull request as ready for review November 20, 2025 11:47

antond-weta reviewed Nov 21, 2025

View reviewed changes

antond-weta approved these changes Nov 22, 2025

View reviewed changes

soswow added 11 commits November 23, 2025 11:17

Add tests to cover empty cases of prepare_transform_spectral()

9f5e65d

Signed-off-by: Aleksandr Motsjonov <[email protected]>

More tests

ec848f7

Signed-off-by: Aleksandr Motsjonov <[email protected]>

More tests

17db3aa

Signed-off-by: Aleksandr Motsjonov <[email protected]>

formatting

726d369

Signed-off-by: Aleksandr Motsjonov <[email protected]>

remove duplicate test

6dd8e2c

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Trying fix windows build

6996a70

Signed-off-by: Aleksandr Motsjonov <[email protected]>

remove unused cmake file

1d4b33a

Signed-off-by: Aleksandr Motsjonov <[email protected]>

PR review comment address

da7fe79

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Add main.cpp to coverage.

7667add

Signed-off-by: Aleksandr Motsjonov <[email protected]>

comment out comment

9802e88

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Removed extra DNG test file; replace test with direct usage one; intr…

9ec91f3

…oduce a test utility wrapper that allow capturing of stderr buffer Signed-off-by: Aleksandr Motsjonov <[email protected]>

antond-weta force-pushed the add-more-util-tests branch from 907750a to 9ec91f3 Compare November 22, 2025 22:17

antond-weta merged commit 07036e1 into AcademySoftwareFoundation:main Nov 22, 2025
17 of 18 checks passed

Add more unit tests #212

Add more unit tests #212

Uh oh!

Conversation

soswow commented Oct 19, 2025

Description

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!

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!

codecov-commenter commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antond-weta 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!

Choose a reason for hiding this comment

Uh oh!

soswow commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antond-weta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Oct 19, 2025 •

edited

Loading

soswow commented Nov 17, 2025 •

edited

Loading