Refactor tests to actually run and not exit silently as well as add new one for data-dir parameter #200

soswow · 2025-09-22T11:28:18Z

No description provided.

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

soswow · 2025-09-22T11:29:02Z

tests/test_image_converter.cpp


 using namespace rta::util;

+std::string convert_linux_path_to_windows_path( const std::string &path )


simplifies tests related to path parsing.

soswow · 2025-09-22T11:30:01Z

tests/test_image_converter.cpp

+///             For example, {"--list-cameras"} or {"--list-illuminants", "--verbose"}.
+///
+/// @return std::string containing the captured stdout output from the rawtoaces execution
+std::string run_rawtoaces_command( const std::vector<std::string> &args )


I've discovered that not all tests were running previouslu, because of the exit(0) inside parse_parameters.

soswow · 2025-09-22T11:30:20Z

tests/test_image_converter.cpp

+std::string run_rawtoaces_command( const std::vector<std::string> &args )
+{
+    // Build the command line - from build/tests/ to build/src/rawtoaces/
+    std::string command = "../src/rawtoaces/rawtoaces";


[ ] TODO: deal with windows

soswow · 2025-09-22T11:30:39Z

tests/test_image_converter.cpp

-    OIIO_CHECK_EQUAL( paths[0], "/usr/local/share/rawtoaces/data" );
-    OIIO_CHECK_EQUAL( paths[1], "/usr/local/include/rawtoaces/data" );
-#else
+#ifdef WIN32


simpler then #if !defined( WIN32 ) && !defined( WIN64 )

I assume you've made sure that you don't need to check for WIN64

you assumed wrong. I believe it was just a crap AI code that thought that would be the way to say "If UNIX like OS" 🤷

soswow · 2025-09-22T11:31:42Z

tests/test_image_converter.cpp

 }

+/// Tests convert_linux_path_to_windows_path utility function
+void test_convert_linux_path_to_windows_path()


a bit meta. testing testing utility. otherwise how do I know it works 🤷🏻‍♂️

soswow · 2025-09-22T11:32:41Z

tests/test_image_converter.cpp

+        lines[0],
+        "Spectral sensitivity data is available for the following cameras:" );
+    OIIO_CHECK_EQUAL( lines[1], "Mamiya / Mamiya 7" );
+    OIIO_CHECK_EQUAL( lines[2], "Canon / EOS R6" );


much simpler assertion then confoluted previously

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

codecov-commenter · 2025-09-22T12:11:31Z

Codecov Report

❌ Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.27%. Comparing base (f2192c3) to head (f4992a7).

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

❌ Your patch status has failed because the patch coverage (42.85%) 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     #200      +/-   ##
==========================================
+ Coverage   71.33%   72.27%   +0.93%     
==========================================
  Files          10       10              
  Lines        2348     2348              
  Branches      311      311              
==========================================
+ Hits         1675     1697      +22     
+ Misses        673      651      -22

Files with missing lines	Coverage Δ
src/rawtoaces_util/image_converter.cpp	`54.11% <42.85%> (+1.98%)`	⬆️

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 f2192c3...f4992a7. Read the comment docs.

🚀 New features to boost your workflow:

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

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

soswow · 2025-09-22T22:47:35Z

tests/test_image_converter.cpp

+#endif
+
+#include "../src/rawtoaces_util/rawtoaces_util_priv.h"
+#include <rawtoaces/image_converter.h>


I think it's merge conflict artifact. to be corrected.

antond-weta · 2025-09-23T08:35:06Z

tests/test_image_converter.cpp

+    OIIO_CHECK_EQUAL( exit_status, 0 );
+#else
+    // Unix implementation using popen
+    FILE *pipe = popen( command.c_str(), "r" );


the code seems identical, can't we just set up a macro holding either popen or _popen; same for pclose

antond-weta · 2025-09-23T08:36:14Z

tests/test_image_converter.cpp

 */
 #ifdef WIN32
-void set_env_var( const char *name, const char *value )
+void set_env_var( std::string name, std::string value )


better to use const ref

antond-weta

looks good

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

lgritz · 2025-09-23T18:37:55Z

tests/test_image_converter.cpp

+
+    // Get exit status and validate
+    int exit_status = platform_pclose( pipe );
+    OIIO_CHECK_EQUAL( exit_status, 0 );


Just as an aside...

I'm glad you find OIIO's little unittest library helpful!

We make the unittest.h header available in case it's of use to others (as it is here), but we don't really consider it a supported part of the OIIO public APIs, so I can't promise that we'll be careful to never do anything that might break your downstream usage.

I think it's fine for you to copy it and rename to R2A_CHECK_FOO so that you aren't directly dependent on the version you get from OIIO, and aren't brittle to any future changes made to these macros on the OIIO side. That also would allow you to customize them on your end to your particular taste or use cases.

That's the approach that OpenColorIO did many years ago.

soswow added 3 commits September 22, 2025 18:48

Add tests for default path argument

efb72a6

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

Add utility for less ifdef win32 code

412d41f

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

Fix exiting tests and add two more

bbb062a

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

soswow commented Sep 22, 2025

View reviewed changes

soswow added 3 commits September 22, 2025 21:56

Fix windows part

a9eb90e

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

Fix test so order doesn't matter

ca8759f

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

Merge remote-tracking branch 'upstream/main' into add-and-refactor-tests

63130d1

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

soswow added 3 commits September 22, 2025 22:46

Find path for test in Win32

54a7b8d

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

correct path

8ef014c

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

try another path

5856f55

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

soswow commented Sep 22, 2025

View reviewed changes

antond-weta reviewed Sep 23, 2025

View reviewed changes

antond-weta approved these changes Sep 23, 2025

View reviewed changes

Last touch ups / PR review address

f4992a7

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

lgritz reviewed Sep 23, 2025

View reviewed changes

antond-weta merged commit 4f1a2ce into AcademySoftwareFoundation:main Sep 23, 2025
14 checks passed


		using namespace rta::util;

		std::string convert_linux_path_to_windows_path( const std::string &path )

Refactor tests to actually run and not exit silently as well as add new one for data-dir parameter #200

Refactor tests to actually run and not exit silently as well as add new one for data-dir parameter #200

Uh oh!

Conversation

soswow commented Sep 22, 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!

Choose a reason for hiding this comment

Uh oh!

soswow Sep 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

antond-weta left a comment

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!

4 participants

soswow Sep 23, 2025 •

edited

Loading

codecov-commenter commented Sep 22, 2025 •

edited

Loading