More unit tests for util #192

soswow · 2025-09-10T12:00:31Z

No description provided.

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

soswow · 2025-09-10T12:09:32Z

src/rawtoaces_util/image_converter.cpp

    }
    else
    {
-        check_and_add_file( path, batches[0] );


this was actually a bug cought by the unit test. Horray for unit tests.

what makes you think this was a bug? the code does exactly what was intended, see main.cpp#40

right. That is rather specific expectation. If it was private API and used internally, I'd see no problem with that. But expecting caller to know that expectation seems specific. Happy to revert and modify the test. You can imagine test is failing when std::vector<std::vector<std::string>> batches; is empty

I agree that the split between main/util is not optimal here, perhaps the outer loop (main.cpp#44) should be also moved inside collect_image_files.
My point is, creating a new batch for each individual file was not the intended use.
The logic was: rawtoaces file1 file2 dir1 dir2 file3 should create 3 batches: [file1, file2, file3], [dir1/*], [dir2/*]

you are right. I didn't know it was intended behaviour. Now I know. I've refactored basd on your suggestion. Now there is no need for boolean return, so batches is the return now. I've added a test that checks that exact behaviour.

codecov-commenter · 2025-09-10T12:13:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.29%. Comparing base (bced9df) to head (2642889).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
+ Coverage   71.40%   73.29%   +1.89%     
==========================================
  Files          10       10              
  Lines        2196     2198       +2     
  Branches      236      237       +1     
==========================================
+ Hits         1568     1611      +43     
+ Misses        628      587      -41

Files with missing lines	Coverage Δ
include/rawtoaces/image_converter.h	`100.00% <ø> (ø)`
src/rawtoaces_util/image_converter.cpp	`54.87% <100.00%> (+4.26%)`	⬆️

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 bced9df...2642889. 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-09-10T13:37:41Z

src/rawtoaces_util/rawtoaces_util_priv.h

+namespace util
+{
+
+bool collect_image_files(


this is public API, so I guess I can drop it and use one from /include if you want

yes please, no need to duplicate things.

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

soswow · 2025-09-10T13:42:12Z

src/rawtoaces_util/image_converter.cpp

 *         false if the file should be ignored
 */
-bool check_and_add_file(
+void check_and_add_file(


boolean is not used in any way

antond-weta · 2025-09-11T00:57:36Z

src/rawtoaces_util/image_converter.cpp

    }
    else
    {
-        check_and_add_file( path, batches[0] );


what makes you think this was a bug? the code does exactly what was intended, see main.cpp#40

antond-weta · 2025-09-11T00:58:37Z

src/rawtoaces_util/rawtoaces_util_priv.h

+namespace util
+{
+
+bool collect_image_files(


yes please, no need to duplicate things.

antond-weta · 2025-09-11T01:07:50Z

tests/test_image_converter.cpp

+    catch ( const std::exception &e )
+    {
+        std::cerr << "Exception caught in main: " << e.what() << std::endl;
+        return 1;


not that this is important, but should this be return unit_test_failures + 1;, as returning 1 discards all other failures we've found so far

antond-weta · 2025-09-11T01:09:01Z

tests/test_image_converter.cpp

+    OIIO::ImageSpec spec;
+
+    // Add integer attribute (this should be ignored by fix_metadata)
+    spec["Make"] = 42; // Integer, not string or float


why do we think that camera make can be float, but not int

I don't know 🤷 That's the main problem not doing TDD and writing tests (contract) after impl. was written. (especially by a different brain)

if ( type.basetype == OIIO::TypeDesc::STRING ) spec[dst_name] = src_attribute->get_string(); else if ( type.basetype == OIIO::TypeDesc::FLOAT ) spec[dst_name] = src_attribute->get_float();

You tell me why we want attribute to be allows in as a float and a string, but not int. Happy to fix prod. code if there is a way to remove it.
This test just tests functionality that is already there.

Oh, I see what happened. The fix_metadata() originally had more attributes than just make and model. Could you delete that "else if float" branch for now, we'll add the allowed types to the mapping dictionary later when needed.

I see. makes sense. I am very big fan of removing code =)

antond-weta · 2025-09-11T01:09:45Z

tests/test_image_converter.cpp

+}
+
+/// Tests database_paths with Windows-style path separator
+void test_database_paths_windows_separator()


why do we test the windows separator, but no the unix one?

I don't need it. it's kinda stupid. If someone changes window separator, test_database_paths_default would brake.

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

include/rawtoaces/image_converter.h

soswow · 2025-09-11T08:35:41Z

src/rawtoaces_util/image_converter.cpp

+            } ),
+        batches.end() );
+
+    return batches;


I can see how it is used later on in main

for ( auto const &batch: batches ) { for ( auto const &input_filename: batch ) {

so, this ^ would deal with empty batches no problem, but from API perspective it doesn't make sense to have empty batch in return. So removing empty won't brake consumer, but make result cleaner.

It does make sense though. It really depends on how the client needs the pipeline organised.

Example: rawtoaces dir1 dir2 dir3, if it returns 2 batches, there is no way to know which one was empty on the client side if it is removed in lib.

ok. I've reverted it back. Empty batches are back.

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

soswow added 12 commits September 8, 2025 21:33

Unit test collect_image_files function

e8ea9e5

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

Unit tests for database_paths function

a12aed5

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

Unit test for fix_metadata

6689447

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

fix win32 env tests

2404161

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

fix win32

285b2fa

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

debugging to find where win32 fails

5005ad8

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

trying fix win32

8fa37b3

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

Fix win32

4d6610b

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

format

64a9683

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

Fix win32 fix

8027ccb

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

formatting

bb207fe

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

Merge remote-tracking branch 'upstream/main' into more-unit-tests-util

422ad82

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

soswow commented Sep 10, 2025

View reviewed changes

formatting and such

3b7d449

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

soswow commented Sep 10, 2025

View reviewed changes

antond-weta reviewed Sep 11, 2025

View reviewed changes

soswow added 2 commits September 11, 2025 18:29

Move outside loop from main.cpp into collect_image_files

32fb368

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

cleanup

416c5f5

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

soswow commented Sep 11, 2025

View reviewed changes

include/rawtoaces/image_converter.h Show resolved Hide resolved

soswow commented Sep 11, 2025

View reviewed changes

soswow and others added 3 commits September 11, 2025 18:42

fix up float type validation

9f4fc09

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

Merge branch 'main' into more-unit-tests-util

cd9b9d0

Bring back empty batches

2642889

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

antond-weta approved these changes Sep 11, 2025

View reviewed changes

antond-weta merged commit f6ad40c into AcademySoftwareFoundation:main Sep 11, 2025
14 checks passed

More unit tests for util #192

More unit tests for util #192

Uh oh!

Conversation

soswow commented Sep 10, 2025

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!

soswow Sep 11, 2025 • 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!

codecov-commenter commented Sep 10, 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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

soswow Sep 11, 2025 •

edited

Loading

codecov-commenter commented Sep 10, 2025 •

edited

Loading