Skip to content

test: fix cpp_tests build and add CI gate#32

Merged
alandtse merged 2 commits into
devfrom
fix/cpp-tests-msvc-modules
May 22, 2026
Merged

test: fix cpp_tests build and add CI gate#32
alandtse merged 2 commits into
devfrom
fix/cpp-tests-msvc-modules

Conversation

@alandtse
Copy link
Copy Markdown
Owner

@alandtse alandtse commented May 22, 2026

Summary

Two related changes — fix the broken cpp_tests build, then wire CI to catch the next regression of the same shape.

Part 1: Make cpp_tests compile under MSVC C++23 modules

The two regression tests added in #23 (stereo SaveSettings preset round-trip + malformed right_uv fallback) don't compile under MSVC's C++23 module-aware parse. The build fails with a misleading cascade of errors starting with C3329 (expected '}' not ')') at what appears to be a correctly-balanced closing paren.

Root cause: json::array({ json{ {k,v}, {k,v} } }) is ambiguous between two nlohmann::json constructor overloads. MSVC's parser falls into the wrong overload, fails inside it, and the displayed error column points at the inner closing ) rather than the actual ambiguity site.

Fix: build the preset object via direct mutation (preset["name"] = ...) before placing it into the outer json::array(...). Identical semantics, no parse-time ambiguity.

Part 2: Gate cpp_tests on changed files in CI

CI was green at #23's merge because the cpp_tests target isn't built or run by any workflow:

  • _shared-build.yaml:246 explicitly targets --target shader_tests only
  • The ctest filter -R ShaderTests would skip CppUtilTests even if the binary existed

Mirrored the shader_tests pattern:

pr-checks.yaml: new cpp_tests files_yaml category (tests/cpp/**) and cpp-tests-should-build output combining cpp_tests / cpp / cmake / build_ci changes. The cpp category is conservative-but-correct since cpp_tests compiles src/Utils/Subrect.cpp directly (per tests/cpp/CMakeLists.txt), so any cpp change could affect it.

_shared-build.yaml: new run-cpp-tests (boolean) and cpp-tests-should-build (string) inputs; new cpp-unit-tests job parallel to shader-unit-tests that builds --target cpp_tests and runs ctest -R CppUtilTests. Gated inline (no composite action — single branch isn't worth the indirection).

Verification

  • cpp_tests.exe builds and 54 assertions / 17 cases pass on a clean build off origin/dev (Part 1 fix)
  • New CI job will fire on this PR since it touches .github/workflows/** (build_ci) and tests/cpp/** (cpp_tests) — self-validates the gate

🤖 Generated with Claude Code

The two regression tests added in PR #23 (stereo SaveSettings preset
round-trip + malformed right_uv) use a nested `json::array({ json{
{k,v}, {k,v} } })` construction that MSVC's C++23 module-aware parse
cannot disambiguate against the `initializer_list<initializer_list<
json>>` overload of `nlohmann::json`'s constructor. The compiler
emits C3329 ("expected '}' not ')'") at what appears to be a
correctly-balanced closing paren, then cascades into a stream of
nonsense errors (C3484, C3613, C2086 "int src: redefinition") that
look like wholly different problems but are all downstream of the
initial parse failure.

CI green at merge time because the cpp_tests target wasn't included
in the workflow's `run_shader_tests` step.

Fix: build the preset json by mutation (`preset["name"] = ...`)
rather than a single nested initializer-list. Identical semantics,
no parse-time ambiguity. Suite is now 54 assertions / 17 cases, all
passing on a clean build off origin/dev.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 22, 2026 08:10
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Warning

Rate limit exceeded

@alandtse has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 42 minutes and 36 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9beb54cc-bcda-4e89-a64c-a5df3ac2fead

📥 Commits

Reviewing files that changed from the base of the PR and between 97556f0 and f8570c7.

📒 Files selected for processing (3)
  • .github/workflows/_shared-build.yaml
  • .github/workflows/pr-checks.yaml
  • tests/cpp/test_subrect.cpp
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cpp-tests-msvc-modules

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

No actionable suggestions for changed features.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSVC (VS 2022/2026) C++23 module-aware parsing failures in the C++ regression tests by avoiding an ambiguous nlohmann::json brace-initialization pattern, ensuring the cpp_tests target compiles locally under that toolchain mode.

Changes:

  • Reworks JSON construction in the stereo SaveSettings preset round-trip regression test to build the preset object via direct mutation before wrapping it in json::array(...).
  • Applies the same construction pattern to the malformed right_uv fallback regression test to avoid the same MSVC parsing ambiguity.

Mirrors the shader_tests pattern so cpp_tests is built and run only
when relevant files change (and always on push / workflow_dispatch /
release events).

pr-checks.yaml:
- New `cpp_tests` files_yaml category: `tests/cpp/**`
- New `cpp-tests-should-build` output combining cpp_tests +
  cpp (since the target compiles src/Utils/Subrect.cpp directly per
  tests/cpp/CMakeLists.txt — conservative-but-correct) + cmake +
  build_ci. Falls through to true on changed-files failure.
- New `run-cpp-tests` and `cpp-tests-should-build` inputs passed
  into _shared-build.yaml.

_shared-build.yaml:
- New `run-cpp-tests` (boolean, default true) and
  `cpp-tests-should-build` (string, default "true") inputs.
- New `cpp-unit-tests` job parallel to `shader-unit-tests`. Builds
  the `cpp_tests` target and runs `ctest -R CppUtilTests`. Gated
  inline (no composite action — the single branch is too small for
  the indirection).

Without this gate, the PR #23 stereo Subrect tests broke the cpp
build but CI stayed green because `--target shader_tests` never
touched the cpp_tests source files. Next time a cpp_tests test
fails to compile, this gate catches it at PR-check time.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment thread .github/workflows/_shared-build.yaml Dismissed
@alandtse alandtse changed the title fix(tests): cpp_tests doesn't compile under MSVC C++23 modules test: fix cpp_tests build under MSVC C++23 modules + CI gate May 22, 2026
@alandtse
Copy link
Copy Markdown
Owner Author

CodeQL alert triage

The CodeQL "Cache Poisoning via execution of untrusted code" alert (#15) fires on the new cpp-unit-tests job's checkout step (_shared-build.yaml:303-312), but the pattern it flags — actions/checkout@v6 with ref: ${{ inputs.ref }} under a pull_request_target-triggered caller — is identical to every existing job in this file:

$ grep -n 'actions/checkout\|ref: ' .github/workflows/_shared-build.yaml
72:  actions/checkout@v6
74:      ref: ${{ inputs.ref }}        # Build plugin and addons
149: actions/checkout@v6
151:     ref: ${{ inputs.ref }}        # Validate shader compilation
227: actions/checkout@v6
229:     ref: ${{ inputs.ref }}        # Run Shader Unit Tests
279: actions/checkout@v6
281:     ref: ${{ inputs.ref }}        # Run C++ Unit Tests  (this PR)

So the alert is a pre-existing repo-wide architectural concern that this PR merely surfaces by adding a fourth instance of the same shape. Fixing it in this PR would require refactoring pull_request_target handling across all four jobs and the pr-checks.yaml caller — well outside the scope of "add CI gate for cpp_tests".

Recommend dismissing alert #15 as "won't fix / accepted risk" with the same justification used implicitly for the other three jobs, or opening a separate hardening PR if you'd like to restructure the trigger model. Not blocking this PR.

@github-actions
Copy link
Copy Markdown

✅ A pre-release build is available for this PR:
Download

@alandtse alandtse changed the title test: fix cpp_tests build under MSVC C++23 modules + CI gate test: fix cpp_tests build and add CI gate May 22, 2026
@alandtse alandtse merged commit dc96878 into dev May 22, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants