OGSMOD-6769: Run Vulkan unit tests after OpenGL for Viewport Toolbox #76

sm-adsk · 2025-09-25T22:10:23Z

The Vulkan tests are currently disabled. This PR introduces the HVT_TEST macro that will allow to parameterize which backends (OpenGL, Vulkan, etc...) are to be tested in a future PR.
Currently only OpenGL is tested.

hodoulp · 2025-09-29T17:02:25Z

Global comments:

The title does explain the content of the pull request i.e., the utest does not run after OpenGL.
put a description of the pull request.

test/RenderingFramework/TestHelpers.cpp

hodoulp · 2025-09-29T17:09:29Z

test/RenderingFramework/TestHelpers.cpp

    return fullFilepath;
 };

 bool HydraRendererContext::compareImages(


Should this method be removed?

It is still used by other functions.

test/RenderingFramework/TestHelpers.cpp

hodoulp · 2025-09-29T17:10:40Z

test/RenderingFramework/TestHelpers.cpp

+    return compareImages(computed, baseLine, threshold, 1);
+}
+
 bool HydraRendererContext::compareOutputImages(const std::string& fileName1,


Should this method be removed?

It is still used in other places.

hodoulp · 2025-09-29T17:11:58Z

test/tests/testFramePass.cpp

 #include <gtest/gtest.h>

-TEST(TestViewportToolbox, framePassUID)
+#include <RenderingFramework/TestFlags.h>


This include is already in RenderingFramework/TestContextCreator.h (at line 25)

hodoulp · 2025-09-29T17:29:56Z

test/howTos/howTo02_CreateOneFramePass.cpp


 #include <gtest/gtest.h>

+#include <RenderingFramework/TestFlags.h>


Already included in #include <RenderingFramework/TestContextCreator.h>

@hodoulp True, but isn't it good practice to include what is used in a cpp?
This makes the code of this file more robust to future changes in TestContextCreator.h, and it is also clearer, in my opinion.

That being said, this file will compile without the include, so you are right about that.

In principle yes. In that specific case it adds useless changes everywhere.
Anyway, that's really minor.

test/RenderingFramework/TestFlags.h

hodoulp · 2025-09-29T17:33:18Z

test/RenderingFramework/TestFlags.h

+            TestHelpers::getTestNames(::testing::UnitTest::GetInstance()->current_test_info());    \
+        AGPTestDefaultBackend##TestName();                                                         \
+    }                                                                                              \
+    void AGPTestDefaultBackend##TestName()


As a method is created the name must be unique but TestName could not be unique.
To be safe it should be:
void HVTTestDefaultBackend##TestSuiteName##TestName();

hodoulp · 2025-09-29T17:33:44Z

test/RenderingFramework/TestFlags.h

+        {                                                                                          \
+            TestHelpers::gParameterizedTests = false;                                              \
+            TestHelpers::gTestNames          = TestHelpers::getTestNames(                          \
+                ::testing::UnitTest::GetInstance()->current_test_info());                 \


Few missing spaces.

hodoulp · 2025-09-29T17:35:08Z

test/RenderingFramework/TestFlags.h

+            // If parameterized tests are used, test_suite_name and name returns
+            // SuiteName/TestName/Param. Hence the following filtering is
+            // needed.
+            if (TestHelpers::gParameterizedTests == true)


if (TestHelpers::gParameterizedTests == true) -> if (TestHelpers::gParameterizedTests)

Copilot

Pull Request Overview

This PR introduces the HVT_TEST macro system to enable parameterized testing for different rendering backends (OpenGL, Vulkan, etc.) in the Viewport Toolbox, laying groundwork for future Vulkan test enablement while currently only testing OpenGL.

Replaces standard TEST macros with HVT_TEST and HVT_TEST_DEFAULT_BACKEND macros
Updates image comparison logic to use new helper functions for parameterized test naming
Adds backend parameterization infrastructure in TestFlags.h

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/tests/testTaskManager.cpp	Converts TEST macros to HVT_TEST variants and updates image comparison calls
test/tests/testSearches.cpp	Converts TEST macros to HVT_TEST variants and updates image comparison calls
test/tests/testFramePasses.cpp	Converts TEST macros to HVT_TEST variants and updates image comparison calls
test/tests/testFramePass.cpp	Converts TEST macros to HVT_TEST variants, adds TestFlags.h include, and updates image comparison calls
test/howTos/*.cpp	Converts TEST macros to HVT_TEST variants, adds TestFlags.h includes, and updates image comparison calls
test/RenderingFramework/TestHelpers.h	Adds compareImage method declaration
test/RenderingFramework/TestHelpers.cpp	Implements compareImage method
test/RenderingFramework/TestFlags.h	Adds HVT_TEST macro definitions and test naming helper functions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-29T19:08:29Z

test/RenderingFramework/TestFlags.h

+        {                                                                                          \
+        public:                                                                                    \
+            void AGPTest##TestName();                                                              \
+        };                                                                                         \


The commented out 'Vulkan' parameter suggests incomplete implementation. Consider adding a TODO comment explaining when Vulkan will be enabled or use a more explicit conditional compilation approach.

Suggested change

}; \

}; \

/* TODO: Enable "Vulkan" backend when Vulkan support is complete and stable.

Currently, only "OpenGL" is enabled for testing. */

Copilot · 2025-09-29T19:08:29Z

test/tests/testSearches.cpp

    // Validates the rendering result.

-    std::string imageFile = std::string(test_info_->name());
+    const std::string computedFileName = TestHelpers::getComputedImagePath();


Assignment to const variable 'computedFileName' will not compile. The variable should be declared as non-const or the logic should be restructured.

Suggested change

const std::string computedFileName = TestHelpers::getComputedImagePath();

std::string computedFileName = TestHelpers::getComputedImagePath();

sebastienberube-adsk · 2025-09-29T19:14:35Z

test/RenderingFramework/TestFlags.h

+        public:                                                                                    \
+            void AGPTest##TestName();                                                              \
+        };                                                                                         \
+        INSTANTIATE_TEST_SUITE_P(TestSuiteName, TestName, ::testing::Values(/*"Vulkan",*/ "OpenGL"),   \


I am guessing this is the line that will need to be changed to activate Vulkan:
INSTANTIATE_TEST_SUITE_P(TestSuiteName, TestName, ::testing::Values(/*"Vulkan",*/ "OpenGL"),
->
INSTANTIATE_TEST_SUITE_P(TestSuiteName, TestName, ::testing::Values("Vulkan", "OpenGL"),

sebastienberube-adsk · 2025-09-29T19:26:18Z

test/howTos/howTo02_CreateOneFramePass.cpp


 #include <gtest/gtest.h>

+#include <RenderingFramework/TestFlags.h>


@hodoulp True, but isn't it good practice to include what is used in a cpp?
This makes the code of this file more robust to future changes in TestContextCreator.h, and it is also clearer, in my opinion.

That being said, this file will compile without the include, so you are right about that.

sebastienberube-adsk · 2025-09-29T19:42:58Z

test/RenderingFramework/TestFlags.h

+                ::testing::UnitTest::GetInstance()->current_test_info());                 \
+            AGPTestDefaultBackend##TestName();                                                     \
+        }                                                                                          \
+        void AGPTestDefaultBackend##TestName()


Note: The block is the exact same as, defined above:

#define HVT_TEST_DEFAULT_BACKEND(TestSuiteName, TestName) \ void AGPTestDefaultBackend##TestName(); \ TEST(TestSuiteName, TestName) \ { \ TestHelpers::gParameterizedTests = false; \ TestHelpers::gTestNames = TestHelpers::getTestNames( \ ::testing::UnitTest::GetInstance()->current_test_info()); \ AGPTestDefaultBackend##TestName(); \ } \ void AGPTestDefaultBackend##TestName()

The only difference is the name.
I am guessing you could define it as follow to prevent repetition:
#define HVT_TEST(TestSuiteName, TestName) HVT_TEST_DEFAULT_BACKEND(TestSuiteName, TestName)

sebastienberube-adsk · 2025-09-29T19:48:43Z

@sm-adsk
I have approved, but I believe these comments should be addressed, at the minimum:
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388970787
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388713260
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2389079932
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388719809

hodoulp · 2025-09-29T21:18:25Z

@sm-adsk do not squash your commits because I cannot see what you changed.
Note: The squash will be done before merging.

hodoulp · 2025-09-29T21:20:08Z

@sm-adsk I have approved, but I believe these comments should be addressed, at the minimum:
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388970787
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388713260
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2389079932
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388719809

These links do not highlight any code section i.e., seem to be invalid.

hodoulp

My main questions/comments are:

sebastienberube-adsk · 2025-09-29T22:49:47Z

@sm-adsk I have approved, but I believe these comments should be addressed, at the minimum:
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388970787
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388713260
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2389079932
https://github.com/Autodesk/hydra-viewport-toolbox/pull/76/files#r2388719809

These links do not highlight any code section i.e., seem to be invalid.

The squash commit might have invalidated these links.
For reference, the 4 things I though were most important were:

reuse HVT_TEST_DEFAULT_BACKEND() in HVT_TEST() when vulkan is disabled
if (TestHelpers::gParameterizedTests == true) -> if (TestHelpers::gParameterizedTests)
AGPTestDefaultBackend -> HVTTestDefaultBackend
remove const that can break the compilation in test/tests/testSearches.cpp:
const std::string computedFileName = TestHelpers::getComputedImagePath();
->
std::string computedFileName = TestHelpers::getComputedImagePath();

hodoulp · 2025-09-30T12:45:25Z

test/RenderingFramework/TestFlags.h

+        {                                                                                            \
+            return info.param;                                                                       \
+        }                                                                                            \
+        class TestName : public ::testing::TestWithParam<std::string>                                \


As discussed:
Param##TestSuiteName##TestName -> Param##TestSuiteName##TestName
class TestName -> class UnitTest##TestSuiteName##TestName

BTW, the method can be a static method from class UnitTest##TestSuiteName##TestName

class UnitTest##TestSuiteName##TestName : public ::testing::TestWithParam<std::string> { public: std::string GetParam(testing::TestParamInfo<std::string> const& info) { return info.param; } void execute(); };

hodoulp · 2025-09-30T12:58:32Z

test/RenderingFramework/TestFlags.h

+
+    inline bool gRunVulkanTests = false;
+    inline bool gParameterizedTests = false;
+    inline TestNames gTestNames = TestNames {};


These three declarations are 'strange' i.e., inline !?
Does it mean that they will be created every time this file header is included?

My current understand is that they must be static to have only one.

Replacing inline with static gives the following error on Jenkins:
'TestHelpers::gRunVulkanTests' defined but not used [-Werror=unused-variable]

hodoulp · 2025-09-30T12:59:06Z

test/RenderingFramework/TestFlags.h

+    {
+        std::string suiteName;
+        std::string fixtureName;
+        std::string paramName;


Add Doxygen comments to explain these three data members.

hodoulp · 2025-09-30T13:01:28Z

test/RenderingFramework/TestFlags.h

+            TestHelpers::gTestNames          = TestHelpers::getTestNames(                            \
+                ::testing::UnitTest::GetInstance()->current_test_info());                            \
+            HVTTest##TestName();                                                                     \
+        }                                                                                            \


Why a global data for gRunVulkanTests & gTestNames when you can change the code to be:

const auto backendIsVulkan = (GetParam() == "Vulkan"); const auto testInfo = (::testing::UnitTest::GetInstance()->current_test_info()); HVTTest##TestName(backendIsVulkan, testInfo);

BTW, I don't understand gParameterizedTests ?

gParameterizedTests has been deprecated and is now removed.

test/RenderingFramework/TestHelpers.h

hodoulp · 2025-10-01T14:58:54Z

test/RenderingFramework/TestFlags.h

+    public:                                                                                      \
+        void HVTTest##TestName(                                                                  \
+            [[maybe_unused]] const std::string& computedImageName,                               \
+            [[maybe_unused]] const std::string& imageFile);                                      \


[Suggestion] We missed perhaps something to limit changes and/or build issues. The parameter names are those used everywhere by default, but in some case some utests customize their content for specific needs. If you rename computedImageName -> defaultComputedImageName & imageFile -> defaultImageFile it could perhaps ease the refactoring work.

Ok, we can do it in a future PR.

hodoulp

The direction is the right one but I understand that it could take some time to finalize HVT & the other dependent repos & I don't want to block you so I approve the pull request to unlock it. But it must still have an explicit @sebastienberube-adsk approval before merging.

…mages.

sm-adsk force-pushed the manars/OGSMOD-6769/parameterize-unit-tests branch 7 times, most recently from 9786129 to ed8c718 Compare September 29, 2025 13:20

sm-adsk marked this pull request as ready for review September 29, 2025 13:25

sm-adsk requested review from hodoulp and sebastienberube-adsk September 29, 2025 13:26

hodoulp reviewed Sep 29, 2025

View reviewed changes

sebastienberube-adsk requested a review from Copilot September 29, 2025 19:07

Copilot AI reviewed Sep 29, 2025

View reviewed changes

sebastienberube-adsk approved these changes Sep 29, 2025

View reviewed changes

sm-adsk force-pushed the manars/OGSMOD-6769/parameterize-unit-tests branch from ed8c718 to 9e6bc1d Compare September 29, 2025 20:07

hodoulp requested changes Sep 29, 2025

View reviewed changes

hodoulp requested changes Sep 30, 2025

View reviewed changes

sm-adsk force-pushed the manars/OGSMOD-6769/parameterize-unit-tests branch 3 times, most recently from 961b6d1 to 86dfd10 Compare October 1, 2025 14:02

hodoulp reviewed Oct 1, 2025

View reviewed changes

hodoulp self-requested a review October 1, 2025 15:00

hodoulp approved these changes Oct 1, 2025

View reviewed changes

sm-adsk added 2 commits October 2, 2025 21:00

OGSMOD-6769: Run Vulkan unit tests after OpenGL for Viewport Toolbox

2a11a88

Refactor the macro HVT_TEST and simplify tests.

a03f8ff

sm-adsk added 6 commits October 2, 2025 21:00

Replace all HVT_TEST_DEFAULT_BACKEND's with HVT_TEST.

98bde86

Add 'const uint8_t pixelCountThreshold = 1' to TestContext::validateI…

4e9a16e

…mages.

Replace inline with static for global variables.

d414f15

Revert static to inline for some variables.

2ec443a

Remove obsolete gParameterizedTests variable and its use.

d02add7

Add Doxygen comments.

0f86209

sm-adsk force-pushed the manars/OGSMOD-6769/parameterize-unit-tests branch from 93fad29 to 0f86209 Compare October 3, 2025 01:00

sm-adsk added 2 commits October 2, 2025 21:39

Remove extra spaces.

f16df9c

Merge branch 'main' into manars/OGSMOD-6769/parameterize-unit-tests

5bc25e1

sm-adsk merged commit 116c931 into main Oct 7, 2025
10 checks passed

sm-adsk deleted the manars/OGSMOD-6769/parameterize-unit-tests branch October 7, 2025 15:21


		#include <gtest/gtest.h>

		#include <RenderingFramework/TestFlags.h>

-        };                                                                                         \
+        };                                                                                         \
+        /* TODO: Enable "Vulkan" backend when Vulkan support is complete and stable.
+           Currently, only "OpenGL" is enabled for testing. */

	const std::string computedFileName = TestHelpers::getComputedImagePath();
	std::string computedFileName = TestHelpers::getComputedImagePath();

OGSMOD-6769: Run Vulkan unit tests after OpenGL for Viewport Toolbox #76

OGSMOD-6769: Run Vulkan unit tests after OpenGL for Viewport Toolbox #76

Uh oh!

Conversation

sm-adsk commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hodoulp commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 29, 2025

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!

sebastienberube-adsk commented Sep 29, 2025

Uh oh!

hodoulp commented Sep 29, 2025

Uh oh!

hodoulp commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hodoulp left a comment

Choose a reason for hiding this comment

Uh oh!

sebastienberube-adsk commented Sep 29, 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!

hodoulp Sep 30, 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!

Choose a reason for hiding this comment

sm-adsk commented Sep 25, 2025 •

edited

Loading

hodoulp commented Sep 29, 2025 •

edited

Loading

hodoulp commented Sep 29, 2025 •

edited

Loading

sebastienberube-adsk commented Sep 29, 2025 •

edited

Loading

hodoulp Sep 30, 2025 •

edited

Loading

hodoulp Oct 1, 2025 •

edited

Loading

hodoulp left a comment •

edited

Loading