Vulkan-Hpp icon indicating copy to clipboard operation
Vulkan-Hpp copied to clipboard

Tests: Enabling CTest alongside module testing with import std

Open M2-TE opened this issue 3 months ago • 12 comments

Builds on changes from #2248. Progress for #1993. Resolves #2331.

The tests were not well isolated and had to go through hoops to get e.g. modules with separate compilation flags. They also did not use any of the convenient options that are provided in our top-level CMakeLists.txt.

Running the tests via CTest gives us full control over them, avoiding current issues like compile definitions bleeding over, adding special compilation flags for certain tests (partiularly for dynamic vs static dispatch testing, it was previously impossible to run both). It will also allow to selectively enable import std (soon).

CTest also allows running individual tests only, making debugging easier. Also leads to easier error reading in CI logs by showing exactly which tests fail.

I've tried to logically separate the commits, but here's a summary of the changes:

  • All targets in tests now use the default Vulkan::Hpp or Vulkan::HppModule (or Video variant), with no manual include directories that muddy the waters. The target setup was placed into vulkan_hpp__setup_vulkan_targets to get consistent path setups, no matter where it is included from. Modules would occasionally break until now due to relative paths, for example.
  • Tests use e.g. set(VULKAN_HPP_NO_CONSTRUCTORS ON) as settings, rather than manually setting compile definitions. This required raising the minimum CMake version to 3.13 for the proper policy.
  • More module tests were fixed and enabled
  • CI was updated to run ctest

Note that the current setup will, unlike before, try to test both dynamic and static dispatch. I don't know how CI will handle this and I remember there being an issue with an internal git repo before. Likely a good idea to just conditionally disable static dispatch testing again before merging.

M2-TE avatar Sep 19 '25 14:09 M2-TE

Where have all the tests gone? In VS I just have the UniqueHandle test, that's all.

asuessenbach avatar Sep 22 '25 15:09 asuessenbach

UniqueHandle is the only test that I have not converted to CTest, as it requires lots of dependency juggling and is not really standalone.

The tests are handled by the CTest executable, should be installed alongside CMake. Does VS not have CMake tools integration? If you want to use it manually, cd into your build folder and do ctest -j and it should work.

M2-TE avatar Sep 22 '25 16:09 M2-TE

@asuessenbach if you are used to the workflow of having add_subdirectory for testing and want to keep it that way, let me know! I will redo the PR without CTest integration. I just thought to attempt the conversion as CTest seems pretty standard across CMake projects.

M2-TE avatar Sep 23 '25 09:09 M2-TE

Having CTests would be cool. But not having the test files available in the VS solution feels wrong. Can't we have both, CTest and files listed in VS? If that's not (easily) possible, I'd prefer to not use CTest for now.

asuessenbach avatar Sep 23 '25 12:09 asuessenbach

I will try to find a solution to have both. As for CTest in VS, you can view all the tests with the Test Explorer (Test > Test Explorer). Under the same menu in Test, you can find Run CTests for VulkanHppGenerator if you just want to run all tests (it seems to not run them in parallel though).

M2-TE avatar Sep 24 '25 14:09 M2-TE

Most of the tests can now handle modules and import std. Remains to be seen if CI is able to handle it (currently only enabled for windows and ubuntu-ext).

All tests can be built as before via subdirectory (default behavior) or optionally be built via CTest instead as via VULKAN_HPP_TESTS_CTEST. Can you check if this works as expected for your workflow @asuessenbach?

Default setup of the std module is now handled in vulkan.hpp directly, so that the module no longer needs to include the vulkan_hpp_macro.hpp header. Module fails to build at current head due to lack of this definition, so give it a quick read if it's okay @sharadhr!

M2-TE avatar Oct 14 '25 14:10 M2-TE

so that the module no longer needs to include the vulkan_hpp_macro.hpp header

It should; we still have VULKAN_HPP_STRINGIFY for the warnings! But this wasn't removed anyway...

Also, I'm not quite able to parse this statement:

Module fails to build at current head due to lack of this definition

In which scenario does the module fail to build?

sharadhr avatar Oct 14 '25 21:10 sharadhr

@sharadhr the error I get is:

In file included from Vulkan-Hpp/vulkan/vulkan.cppm:28:
Vulkan-Hpp/vulkan/vulkan.hpp:41:8: fatal error: module 'VULKAN_HPP_STD_MODULE' not found
   41 | import VULKAN_HPP_STD_MODULE;

but you are right, it does include the macro header (was I drunk?). It is once again an issue with the

defined( __cpp_modules ) && defined( __cpp_lib_modules )

so this could probably just be shortened to check for VULKAN_HPP_CXX_MODULE instead (defined at top of vulkan.cppm). What do you think?

@asuessenbach do you know of a way to test static dispatch in CI? It obviously won't be able to find Vulkan packages, so it likely isn't possible (same as before).

M2-TE avatar Oct 15 '25 09:10 M2-TE

D:\a\Vulkan-Hpp\Vulkan-Hpp\tests\ExtensionInspection\ExtensionInspection.cpp(82,100): warning C4996: 'vk::EXTDebugReportExtensionName': The VK_EXT_debug_report extension has been deprecated by VK_EXT_debug_utils.
D:\a\Vulkan-Hpp\Vulkan-Hpp\tests\ExtensionInspection\ExtensionInspection.cpp(86,98): warning C4996: 'vk::AMDNegativeViewportHeightExtensionName': The VK_AMD_negative_viewport_height extension has been obsoleted by VK_KHR_maintenance1.

Warnings pop up for both VK_EXT_debug_report and VK_AMD_negative_viewport_height. It tests for whether these are deprecated, but throws the warning regardless, simply because the enum is used, e.g.:

  static_assert( vk::isObsoletedExtension( vk::AMDNegativeViewportHeightExtensionName ), "static assert test failed" );
  static_assert( vk::getExtensionObsoletedBy( vk::AMDNegativeViewportHeightExtensionName ) == vk::KHRMaintenance1ExtensionName,
                 "static assert test failed" );

The guard for these deprecation tests is also a bit peculiar, why the restrictions?

#if ( 201907 <= __cpp_constexpr ) && ( !defined( __GNUC__ ) || ( 110400 < GCC_VERSION ) )

M2-TE avatar Oct 20 '25 11:10 M2-TE

Current state of my descent into CI debugging insanity:

  • Clang++-20: CMake does not provide the [[ TARGET __CMAKE::CXX23 ]] target.
  • Clang++-19: CMake does provide the [[ TARGET __CMAKE::CXX23 ]] target, but has issues with the standard library (using -stblib=libc++) as seen here: https://github.com/M2-TE/Vulkan-Hpp/actions/runs/18653373179/job/53176398437. Seems to affect compilation of utils.hpp.

I believe this is because it defaults to an older standard library version, as clang is installed in the runners. Overriding the specific libc++ version seems like a pain: https://releases.llvm.org/14.0.0/projects/libcxx/docs/UsingLibcxx.html

Identical setup in ubuntu:24.04 docker setup works just fine with modules, so not exactly sure what is different.

M2-TE avatar Oct 20 '25 14:10 M2-TE

I think it's best to postpone the CI testing of modules. I will work on this as a separate PR, so things don't get too messy. All the tests will now work with a properly set up toolchain that can handle modules and import std.

Some other things added:

  • Properly set the build type on windows via --config $BUILD_TYPE
  • Fixed the extension inspection test on windows by ignoring the deprecation warning via pragma

With this, I would say the PR is pretty much complete. The concerns discussed with @sharadhr related to CI testing should be handled in a separate PR imo. On windows, building with Ninja seems to still have an issue or two in CI anyways.

M2-TE avatar Oct 28 '25 12:10 M2-TE

Side note: I realised #warning is in the C++23 standard, but MSVC hasn't implemented it yet (sigh).

sharadhr avatar Nov 04 '25 15:11 sharadhr

Brought the PR up-to-date and managed to get module testing working. C++Modules are being tested via ~Ninja+Clang on Windows~ and Ninja+Clang on ubuntu-ext. Testing on MSVC would required a separate runner that uses Ninja (this PR already ballooned enough as is).

M2-TE avatar Nov 25 '25 10:11 M2-TE

PR is ready for review, so please let me know what you think @sharadhr.

M2-TE avatar Nov 25 '25 11:11 M2-TE

A cursory glance shows no glaring issues. That singular build failure should probably be resolved by rebasing atop main. However I am surprised it did not fail on any other platform, especially Windows + MSVC, which does support import std. There might be a missing test case scenario; see this action failure. (I've been thinking about using that CI setup, which does seem very comprehensive in terms of platform coverage, and yet quite simple in its implementation).

I do recall there was discussion of splitting up this PR: one to set up CTest, and another to set up the module tests.

sharadhr avatar Dec 01 '25 14:12 sharadhr

Well at least now we know that the module tests work ;)

But yes I remember us talking about splitting this up. The changes here are quite interlinked (CTest and Modules), so if it is still desired to split it into two separate PRs, I would simply exclude the module-related things from CI. Is that acceptable or should I tear these apart on a deeper level?

I am surprised it did not fail on any other platform

I only "activated" the ubuntu-ext one so far. Windows Clang has too old of a CMake version, same for MSVC. Since you mentioned wanting to adopt those other CI workflows, I simply focused on having one single working test to improve the current.. lack of module tests.

I do quite like those workflows too, so I would vote to adopt those as well! Hopefully after merging this though, upkeep is a bit of work with the merge conflicts..

M2-TE avatar Dec 01 '25 14:12 M2-TE

Windows Clang has too old of a CMake version, same for MSVC

This is surprising! Visual Studio bundles CMake 4.1. Or we can always install the upstream CMake with the same get-cmake workflow.

sharadhr avatar Dec 01 '25 14:12 sharadhr

Yea I was suprised as well. Check the GitHub hosted runners: Windows 2022 and Windows 2025.

We can probably just use a baseline like

      - name: Setup CMake and Ninja
        if: matrix.env.modules
        uses: lukka/get-cmake@latest
        with:
          cmakeVersion: 4.1.2 # (or latest?)
          ninjaVersion: latest

Should be a better solution than the get-cmake thing I've done before.

M2-TE avatar Dec 01 '25 14:12 M2-TE

I would simply exclude the module-related things from CI. Is that acceptable or should I tear these apart on a deeper level?

I think just having the module stuff separate would be sufficient. Although I am decidedly not the final arbiter, and maybe @asuessenbach should also input.

sharadhr avatar Dec 01 '25 15:12 sharadhr

It seems, you're running this PR on a slightly outdated Vulkan-Headers version. Would be great, if you'd rebase it accordingly.

asuessenbach avatar Dec 02 '25 08:12 asuessenbach

I would simply exclude the module-related things from CI. Is that acceptable or should I tear these apart on a deeper level?

I think just having the module stuff separate would be sufficient. Although I am decidedly not the final arbiter, and maybe @asuessenbach should also input.

Agreed, "simply excluding the module-related things from CI" is probably the right thing to do.

asuessenbach avatar Dec 02 '25 08:12 asuessenbach

Agreed, "simply excluding the module-related things from CI" is probably the right thing to do.

Done!

It seems, you're running this PR on a slightly outdated Vulkan-Headers version

It should be on the newest version, as I've kept it up to date with main and it is currently at 2fa2034. Could you check it again or tell me which commit hash it should be using @asuessenbach?

M2-TE avatar Dec 02 '25 08:12 M2-TE

It should be on the newest version

You're right. Don't know what I've seen before. Sorry, please.

asuessenbach avatar Dec 02 '25 09:12 asuessenbach

I had accidentally removed the ctest calls for Windows CI before.. sorry about that!

M2-TE avatar Dec 02 '25 10:12 M2-TE