Skip to content

test: Add Google Test and Google Benchmark targets - #3405

Open
xezon wants to merge 5 commits into
TheSuperHackers:mainfrom
xezon:xezon/add-unittest
Open

xezon wants to merge 5 commits into
TheSuperHackers:mainfrom
xezon:xezon/add-unittest

Conversation

@xezon

@xezon xezon commented Oct 3, 2026 •

Copy link
Copy Markdown

Merge with Rebase

This change adds the Google Test and Google Benchmark frameworks and creates the following executable targets:

z_googlebenchmark
z_googletest
g_googlebenchmark
g_googletest

Both games inherit the same tests and benchmarks from Dependencies and Core, so there will be duplication, but essentially they compile with RTS_GENERALS and RTS_ZEROHOUR respectively.

The tests and benchmarks have a few samples added for Dependencies, Game core and libraries.

The tests are enabled in the win32 CI builds, but the benchmarks are disabled for potential time constraints reasons.

AI Use

This was mostly generated with Claude Opus 5.5. Went through reviews and minor fixups.

TODO

  • Add pull id to commit titles

xezon added 2 commits October 3, 2026 11:46
…hared by Dependencies and Core and move setups to Dependencies/CMakeLists.txt (#3405)
@xezon xezon added Enhancement Is new feature or request Build Anything related to building, compiling Test Is testing related labels Oct 3, 2026
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cd9c0bf5-e111-49b7-9570-a8cfc06086c6
📥 Commits

Reviewing files that changed from the base of the PR and between 848c0a6 and f359d8b.

📒 Files selected for processing (1)
  • .github/workflows/reusable-build-toolchain.yml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.


Walkthrough

The change adds optional Google Test and Google Benchmark build paths, engine test and benchmark executables, crash-handler callbacks, and CI test execution.

Changes

Test and Benchmark Build Integration

Layer / File(s) Summary
Build options and dependency setup
cmake/config-build.cmake, cmake/config.cmake, cmake/googlebenchmark.cmake, cmake/googletest.cmake, CMakeLists.txt, Dependencies/*, Core/CMakeLists.txt, Generals/Code/CMakeLists.txt, GeneralsMD/Code/CMakeLists.txt
CMake adds test and benchmark options, shared dependency settings, and Google Test and Google Benchmark dependencies. The top-level build adds the Dependencies subdirectory. Core and both game builds add test and benchmark targets when their options are enabled.
Google Test harness and targets
Core/GameEngine/Include/Common/Debug.h, Core/GameEngine/Source/Common/System/Debug.cpp, Core/GoogleTest/*, Dependencies/GoogleTest/*, Generals/Code/GoogleTest/CMakeLists.txt, GeneralsMD/Code/GoogleTest/CMakeLists.txt
A crash-handler callback API is added and used by the Google Test runner. Both games receive test targets, with tests for AsciiString, compression, and strlcpy_t.
Google Benchmark harness and workloads
Core/GoogleBenchmark/*, Dependencies/GoogleBenchmark/*, Generals/Code/GoogleBenchmark/CMakeLists.txt, GeneralsMD/Code/GoogleBenchmark/CMakeLists.txt
Both games receive benchmark targets. The benchmark sources cover AsciiString formatting, compression and decompression, and string-copy operations.
CI test execution and instructions
.github/workflows/ci.yml, .github/workflows/reusable-build-toolchain.yml, CMakePresets.json, TESTING.md
The reusable workflow optionally builds tests and benchmarks and runs CTest when tests are enabled. Windows test presets and instructions for running tests and benchmarks are added.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to f359d

CI runs the new tests on supported Windows presets, and CTest discovery and test registration are configured. No outstanding merge-blocking risk is identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f359d

The new tooling is opt-in locally, but CI enables tests across both games’ Windows builds. Tests run with existing build permissions; no increased credential authority or production exposure was established. External dependency contents and effective runner isolation were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new execution reaches both games’ win32 CI matrices and locally invoked tooling processes. Its security-sensitive scope is the existing runner identity, workspace, and available credentials. The inspected changes do not establish a new production service or tenant boundary.

Trust Boundaries and Controls

  • inferred — PR-built tests execute after vcpkg jobs provision a cleartext NuGet token, without an explicit intervening sandbox or cleanup step. However, the same provisioning already preceded repository-controlled CMake execution in the base. Package-write permissions and inherited secrets are unchanged, and fork builds select read-only cache access. This is an existing build-trust exposure, not a demonstrated privilege expansion introduced by test execution.

Hardening Proposals

  • proposed — As defense in depth, execute tests in a separate least-privileged job without inherited secrets or package-write authority. This would reduce exposure at the existing build-trust boundary rather than remediate a proven vulnerability introduced by this PR.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding Google Test and Google Benchmark targets.
Description check ✅ Passed The description explains the new test and benchmark frameworks, executable targets, sample tests and benchmarks, and CI configuration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Comment thread .github/workflows/ci.yml
Comment thread GeneralsMD/Code/GoogleTest/CMakeLists.txt
@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Adds Google Test and Google Benchmark test infrastructure.

The PR appears functionally safe to merge, but the explicit GPL-header requirement should be satisfied first.

Summary

The PR adds Google Test and Google Benchmark executables for both games, sample tests and benchmarks, CMake options and presets, and CI test execution. No code changed since the previous review; the outstanding new comment concerns license headers in newly created CMake files.

Reviews (4) · Last reviewed commit: "ci: Build tests and benchmarks and run t..."

Comment thread Generals/Code/GoogleTest/CMakeLists.txt
Comment thread Core/GoogleTest/GoogleTestMain.cpp
Comment thread .github/workflows/reusable-build-toolchain.yml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 39ada8ed-dc78-4fa9-b708-3b88ee3e10c0
📥 Commits

Reviewing files that changed from the base of the PR and between 2817000 and 3738ca6.

📒 Files selected for processing (36)
  • .github/workflows/ci.yml
  • .github/workflows/reusable-build-toolchain.yml
  • CMakeLists.txt
  • CMakePresets.json
  • Core/CMakeLists.txt
  • Core/GameEngine/Include/Common/Debug.h
  • Core/GameEngine/Source/Common/System/Debug.cpp
  • Core/GoogleBenchmark/CMakeLists.txt
  • Core/GoogleBenchmark/GameEngine/Common/AsciiStringBenchmark.cpp
  • Core/GoogleBenchmark/GoogleBenchmarkMain.cpp
  • Core/GoogleBenchmark/Libraries/Compression/CompressionManagerBenchmark.cpp
  • Core/GoogleTest/CMakeLists.txt
  • Core/GoogleTest/GameEngine/Common/AsciiStringTest.cpp
  • Core/GoogleTest/GoogleTestMain.cpp
  • Core/GoogleTest/Libraries/Compression/CompressionManagerTest.cpp
  • Dependencies/Bink/CMakeLists.txt
  • Dependencies/CMakeLists.txt
  • Dependencies/DbgHelp/CMakeLists.txt
  • Dependencies/GoogleBenchmark/CMakeLists.txt
  • Dependencies/GoogleBenchmark/Utility/stringex_benchmark.cpp
  • Dependencies/GoogleTest/CMakeLists.txt
  • Dependencies/GoogleTest/Utility/stringex_test.cpp
  • Dependencies/Miles/CMakeLists.txt
  • Dependencies/Usp10/CMakeLists.txt
  • Dependencies/Utility/CMakeLists.txt
  • Generals/Code/CMakeLists.txt
  • Generals/Code/GoogleBenchmark/CMakeLists.txt
  • Generals/Code/GoogleTest/CMakeLists.txt
  • GeneralsMD/Code/CMakeLists.txt
  • GeneralsMD/Code/GoogleBenchmark/CMakeLists.txt
  • GeneralsMD/Code/GoogleTest/CMakeLists.txt
  • TESTING.md
  • cmake/config-build.cmake
  • cmake/config.cmake
  • cmake/googlebenchmark.cmake
  • cmake/googletest.cmake

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Comment thread CMakeLists.txt
@xezon
xezon force-pushed the xezon/add-unittest branch from f359d8b to 70e5c65 Compare October 3, 2026 20:23
@xezon

xezon commented Oct 3, 2026

Copy link
Copy Markdown
Author

Commit count reduced by 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build Anything related to building, compiling Enhancement Is new feature or request Test Is testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant