test: add initial tests #1612
test: add initial tests #1612
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a unit test harness (Catch2), many new test suites and test utilities, CMake support to optionally build tests, GitHub Actions workflows for running tests and uploading coverage to Codecov, pre-commit hook and PowerShell test runner, documentation and Codecov configuration, and a vcpkg dependency for Catch2. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as "GitHub Actions"
participant Repo as "Repository"
participant CMake as "CMake / vcpkg"
participant Build as "MSVC/Clang"
participant Test as "CommunityShaders_Tests.exe"
participant Coverage as "llvm-profdata / lcov"
participant Codecov as "Codecov"
Dev->>Repo: push / open PR
GH->>Repo: workflow triggered (test + coverage)
GH->>CMake: checkout + setup vcpkg + configure (BUILD_TESTS)
CMake->>Build: build test targets
Build->>Test: produce test executable
GH->>Test: run tests (JUnit XML) → upload test artifacts
note right of GH: Coverage job runs (profraw generation)
Test->>Coverage: run with coverage -> profraw
Coverage->>Coverage: llvm-profdata -> profdata -> lcov + HTML
Coverage->>Codecov: upload coverage data
GH->>Repo: upload HTML artifact and (PR) comment with summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 YAMLlint (1.37.1).pre-commit-config.yaml[Errno 2] No such file or directory: '.pre-commit-config.yaml' 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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit testing infrastructure to the Community Shaders project, introducing 766 test assertions across 56 test cases. The implementation uses Catch2 for the test framework and includes CI/CD integration through GitHub Actions with code coverage tracking via Codecov.
Key Changes
- Adds standalone unit tests for critical utilities (path normalization, GPU alignment, shader validation, game unit conversions, configuration parsing)
- Integrates Catch2 testing framework via vcpkg
- Establishes CI/CD pipeline with automated test execution and coverage reporting
- Provides comprehensive documentation and helper scripts for test execution
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Adds Catch2 dependency for unit testing framework |
| tests/test_simple_utilities.cpp | Tests path normalization, math utilities, and formatting functions |
| tests/test_shader_validation.cpp | Validates shader define names, version parsing, and permutation calculations |
| tests/test_shader_constants.cpp | Tests critical shader register mappings for VR and flat builds |
| tests/test_gpu_structures.cpp | Validates GPU buffer alignment requirements (16-byte and 64-byte) |
| tests/test_enums_and_types.cpp | Tests enum values, type safety, and flag combinations |
| tests/test_conversion_utilities.cpp | Validates game unit conversions and weather data transformations |
| tests/test_configuration.cpp | Tests JSON/INI parsing and configuration validation |
| tests/run_tests.ps1 | PowerShell script to build and execute tests locally |
| tests/TROUBLESHOOTING.md | Troubleshooting guide for common test setup issues |
| tests/README.md | Main documentation covering test coverage, design philosophy, and usage |
| tests/QUICKSTART.md | Quick start guide for getting tests running in 5 minutes |
| tests/CODECOV_SETUP_INSTRUCTIONS.md | Step-by-step guide for setting up Codecov integration |
| tests/CMakeLists.txt | CMake configuration for building test executable |
| tests/.gitignore | Git ignore rules for test build artifacts |
| cmake/XSEPlugin.cmake | Updates BUILD_TESTS option handling to allow command-line override |
| CMakeLists.txt | Adds BUILD_TESTS option and test subdirectory integration |
| .pre-commit-config.yaml | Adds pre-commit hook to run unit tests before commits |
| .github/workflows/test.yaml | GitHub Actions workflow for running tests and generating coverage reports |
| .codecov.yml | Codecov configuration for coverage tracking and PR comments |
tests/test_simple_utilities.cpp
Outdated
| #include <cmath> | ||
| #include <filesystem> | ||
| #include <sstream> | ||
| #include <string> |
There was a problem hiding this comment.
Missing required header #include <iomanip>. The FormatMilliseconds function on lines 86-89 uses std::setprecision which requires this header. Add #include <iomanip> to fix the compilation error.
| #include <string> | |
| #include <string> | |
| #include <iomanip> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
cmake/XSEPlugin.cmake (1)
49-52: LGTM! Clear conditional logic to preserve user-defined BUILD_TESTS.The logic correctly avoids overriding BUILD_TESTS when set via command line (e.g.,
-DBUILD_TESTS=ON). The comment clearly explains the intent.Optional simplification (nitpick):
TheSTREQUAL ""check is somewhat redundant since CMake'sNOT DEFINEDalready handles undefined variables. You could simplify to:if(NOT DEFINED BUILD_TESTS) set(BUILD_TESTS OFF) endif()However, the current explicit form is perfectly valid and may be preferred for clarity.
tests/run_tests.ps1 (1)
33-40: Consider parameterizing the build configuration.The script hardcodes
Releaseconfiguration (line 35) but searches for bothReleaseandDebugexecutables. Consider adding a parameter to allow developers to choose the build configuration for local testing.Apply this enhancement:
+param( + [ValidateSet('Release', 'Debug')] + [string]$Configuration = 'Release' +) + # Quick Test: Run existing tests # Tests if the test suite works (uses existing MSVC build) Write-Host "========================================" -ForegroundColor Cyan Write-Host "Running Unit Tests" -ForegroundColor Cyan Write-Host "========================================" -ForegroundColor Cyan +Write-Host "Configuration: $Configuration" -ForegroundColor Gray Write-Host ""Then update line 35:
- cmake --build build/ALL --target CommunityShaders_Tests --config Release + cmake --build build/ALL --target CommunityShaders_Tests --config $Configurationtests/CMakeLists.txt (1)
72-97: Consider using configure_file() instead of file(WRITE) for stub header.Using
file(WRITE)to generate the test stub header at configure time has several drawbacks:
- Runs at configure time rather than build time, which can cause issues with incremental builds
- String escaping in multi-line strings can be fragile
- Inline variables in namespaces (lines 93-94) are valid in C++17+ but the approach could lead to ODR violations if included in multiple translation units
Consider one of these approaches:
Option 1: Use configure_file() with a template
Create
tests/test_stubs.h.in:// Test stubs for Skyrim-specific types #include <string> #include <vector> #include <filesystem> #include <d3dcommon.h> namespace REL { struct Version { std::string string(const char*) const { return "1.0.0"; } }; } namespace logger { template<typename... Args> void warn(const char*, Args&&...) {} template<typename... Args> void error(const char*, Args&&...) {} template<typename... Args> void info(const char*, Args&&...) {} } namespace globals { namespace d3d { inline void* device = nullptr; inline void* context = nullptr; } }Then in CMakeLists.txt:
configure_file( "${CMAKE_CURRENT_SOURCE_DIR}/test_stubs.h.in" "${CMAKE_CURRENT_BINARY_DIR}/test_stubs.h" @ONLY )Option 2: Use a real header file
Create
tests/test_stubs.hdirectly in the tests directory and add it to the test sources.tests/test_simple_utilities.cpp (1)
65-76: Consider portability of sprintf_s.The
sprintf_sfunction (lines 69, 73) is Microsoft-specific and may not be available if tests are ever run on non-Windows platforms (e.g., for cross-platform coverage analysis).Since this project targets Windows (Skyrim), this is acceptable, but consider using
std::format(C++20) orstd::snprintffor better portability.Example with std::ostringstream (already used elsewhere in this file):
std::string FormatFileSize(uint64_t bytes) { std::ostringstream oss; oss << std::fixed << std::setprecision(1); if (bytes >= 1024 * 1024) { oss << (static_cast<float>(bytes) / (1024 * 1024)) << " MB"; } else { oss << (static_cast<float>(bytes) / 1024) << " KB"; } return oss.str(); }.github/workflows/test.yaml (1)
103-108: Consider pinning Clang version for stability.Using
version: latestfor Clang (line 106) could cause workflow instability if a new Clang version introduces breaking changes or behaves differently.Consider pinning to a specific version:
- name: Setup Clang uses: egor-tensin/setup-clang@v1 with: - version: latest + version: 17 platform: x64Check the latest stable Clang version and update accordingly. This ensures reproducible builds and prevents unexpected failures.
tests/test_shader_validation.cpp (2)
22-32: RenameIsValidDefineNamand harden character checksThe function name looks like a typo (
IsValidDefineNam), which is slightly confusing in a “specification” test file. Also,std::isupper/std::isdigitoperate onunsigned chardomain; passing a plaincharis technically UB for non‑ASCII input.Consider:
-bool IsValidDefineNam(const std::string& name) +bool IsValidDefineName(const std::string& name) { if (name.empty()) return false; - for (char c : name) { - if (!std::isupper(c) && c != '_' && !std::isdigit(c)) + for (unsigned char c : name) { + if (!std::isupper(c) && c != '_' && !std::isdigit(c)) return false; } return true; }(and update the call sites accordingly).
71-91: Avoid re‑implementing production macro formatting logic inside tests
ShaderMacroandMacrosToStringre‑simulate behavior described as coming fromFormat.h. As soon as the real implementation exists or is exposed, it would be better for the tests to exercise that code directly instead of a duplicated copy here; otherwise this file can silently diverge from the actual behavior.When feasible, consider including the real header and using the production
DefinesToString(or equivalent) API, keeping this test focused on behavior rather than a parallel implementation.tests/test_shader_constants.cpp (1)
15-115: Consider centralizing register indices to avoid drift from HLSLThese tests thoroughly check index ranges, uniqueness, and grouping for flat vs VR builds, but they also duplicate a large set of raw register numbers that “must match HLSL register assignments.”
To reduce the risk of tests drifting from the real definitions over time, consider (when practical):
- Pulling the indices from a shared header (e.g., the struct that owns these constants), or
- Defining a single authoritative table/enum of indices and using it both in production code and these tests.
That keeps the behavioral assertions here while avoiding two independent sources of truth for the actual numeric values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.codecov.yml(1 hunks).github/workflows/test.yaml(1 hunks).pre-commit-config.yaml(1 hunks)CMakeLists.txt(3 hunks)cmake/XSEPlugin.cmake(1 hunks)tests/.gitignore(1 hunks)tests/CMakeLists.txt(1 hunks)tests/CODECOV_SETUP_INSTRUCTIONS.md(1 hunks)tests/QUICKSTART.md(1 hunks)tests/README.md(1 hunks)tests/TROUBLESHOOTING.md(1 hunks)tests/run_tests.ps1(1 hunks)tests/test_configuration.cpp(1 hunks)tests/test_conversion_utilities.cpp(1 hunks)tests/test_enums_and_types.cpp(1 hunks)tests/test_gpu_structures.cpp(1 hunks)tests/test_shader_constants.cpp(1 hunks)tests/test_shader_validation.cpp(1 hunks)tests/test_simple_utilities.cpp(1 hunks)vcpkg.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/CODECOV_SETUP_INSTRUCTIONS.mdtests/QUICKSTART.mdtests/TROUBLESHOOTING.mdtests/run_tests.ps1CMakeLists.txttests/test_enums_and_types.cpptests/README.mdvcpkg.jsoncmake/XSEPlugin.cmaketests/test_gpu_structures.cpptests/CMakeLists.txttests/test_shader_constants.cpptests/test_conversion_utilities.cpptests/test_simple_utilities.cpptests/test_shader_validation.cpptests/test_configuration.cpp
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
tests/test_enums_and_types.cpptests/test_gpu_structures.cpptests/test_shader_constants.cpptests/test_conversion_utilities.cpptests/test_simple_utilities.cpptests/test_shader_validation.cpptests/test_configuration.cpp
🧠 Learnings (12)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
tests/.gitignore
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
tests/test_enums_and_types.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
tests/test_enums_and_types.cpptests/test_gpu_structures.cpptests/test_shader_constants.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Applied to files:
tests/test_enums_and_types.cpptests/test_gpu_structures.cpptests/test_shader_constants.cpp
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
tests/README.mdtests/CMakeLists.txt.codecov.ymltests/test_simple_utilities.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
tests/test_gpu_structures.cpptests/test_shader_constants.cpptests/test_shader_validation.cpp
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
tests/test_shader_constants.cpp
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
tests/test_shader_constants.cpptests/test_shader_validation.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
tests/test_shader_validation.cpp
📚 Learning: 2025-08-05T17:40:44.828Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: package/Shaders/Lighting.hlsl:0-0
Timestamp: 2025-08-05T17:40:44.828Z
Learning: In the skyrim-community-shaders repository, ultra trees (object LOD trees) are detected using a compound shader define condition `defined(DO_ALPHA_TEST) && defined(LOD_BLENDING) && defined(RIM_LIGHTING) && defined(SOFT_LIGHTING)` because "they have no define" according to the comment. The `ExtraFlags::IsTree` flag is used for different tree handling (AO removal in skylighting) and may not apply to ultra trees specifically. Before replacing the compound condition with `IsTree`, verification is needed to ensure the flag covers ultra trees.
Applied to files:
tests/test_shader_validation.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
tests/test_configuration.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Agent
- GitHub Check: coverage
- GitHub Check: test (Release)
- GitHub Check: test (Debug)
🔇 Additional comments (22)
tests/.gitignore (1)
1-13: LGTM! Comprehensive ignore patterns for test artifacts.The
.gitignoreappropriately covers all common test build artifacts, results, and generated files. Well organized with clear comments.tests/CODECOV_SETUP_INSTRUCTIONS.md (1)
1-139: Excellent comprehensive Codecov setup guide!The setup instructions are clear, well-structured, and cover all necessary steps including troubleshooting. The step-by-step format with prerequisites, signup, token configuration, and optional badge addition makes this very maintainer-friendly.
.codecov.yml (1)
1-35: Well-configured Codecov setup for the project's testing strategy.The configuration appropriately:
- Sets informational status to avoid blocking PRs on coverage drops
- Targets 80% coverage for new code (patch)
- Ignores test files, external dependencies, and build artifacts
- Focuses coverage tracking on
src/utilities where unit tests are feasibleThe comment on line 34-35 clearly explains the scope limitation (can't mock Skyrim), which aligns with the testing philosophy described in the README.
tests/QUICKSTART.md (2)
1-207: Excellent beginner-friendly quick start guide!The guide provides a clear, step-by-step path from installation through writing first tests. The inclusion of IDE integration steps and troubleshooting for common issues (Catch2 not found, link errors) makes this particularly valuable for new contributors.
The mock examples (lines 109-112) are especially helpful for understanding how to work around Skyrim dependencies.
199-199: Cannot verify — files not found in repositoryThe files referenced in this review comment (
tests/QUICKSTART.mdand test files) do not exist in the current repository. The repository structure containsfeatures/,package/, andnew-feature-template/directories, but notests/directory orQUICKSTART.mdfile.Without access to the actual file being reviewed, I cannot verify:
- Whether
test_format.cppexists- What line 199 actually contains
- Whether the reference is correct or outdated
Please confirm the correct file path or provide the file content so verification can proceed.
.pre-commit-config.yaml (1)
30-39: Pre-commit hook may fail for users who haven't built tests yet.The hook assumes
build/ALLexists and thatCommunityShaders_Teststarget has been built withBUILD_TESTS=ON. New contributors or users who haven't configured tests will encounter confusing CMake errors when committing.Consider adding a check or clearer error message:
Option 1: Document this requirement prominently in setup docs (ensure QUICKSTART.md mentions pre-commit setup).
Option 2: Wrap the command in a shell script that checks for the test executable first:
- id: run-unit-tests name: Run C++ Unit Tests entry: bash -c 'if [ -f build/ALL/tests/Release/CommunityShaders_Tests.exe ]; then cmake --build build/ALL --target CommunityShaders_Tests --config Release; else echo "⚠️ Tests not built yet. Run: cmake --preset=ALL -DBUILD_TESTS=ON && cmake --build build/ALL --target CommunityShaders_Tests"; exit 1; fi' language: system pass_filenames: false files: '(tests/.*\.(cpp|h|hpp)$|src/Utils/.*\.(cpp|h|hpp)$)'This provides a clearer error message directing users to build tests first.
⛔ Skipped due to learnings
Learnt from: CR Repo: doodlum/skyrim-community-shaders PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-17T18:37:35.839Z Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python toolingCMakeLists.txt (3)
49-54: LGTM!The BUILD_TESTS option is well-integrated with appropriate defaults and clear status messaging. The implementation follows CMake best practices.
837-844: LGTM!The test infrastructure is properly gated and the status message provides clear guidance on the test target name.
855-859: LGTM!The conditional user guidance provides helpful instructions for running tests when BUILD_TESTS is enabled.
tests/run_tests.ps1 (1)
1-84: LGTM!The test runner script is well-structured with clear error handling, colored output for user feedback, and proper exit codes. The logic correctly handles both pre-built tests and building on demand.
tests/CMakeLists.txt (4)
1-21: LGTM!The test project setup correctly finds Catch2, collects test sources, creates the test executable, and sets the C++23 standard to match the main project.
23-50: LGTM!The include directories and link libraries are appropriately configured. The decision to avoid linking the full plugin to prevent Skyrim dependencies is sound, and the UNIT_TEST_BUILD definition allows conditional compilation in source files.
52-69: LGTM!The Clang-based coverage support is properly conditioned with clear warnings for non-Clang compilers. The compile and link options for source-based coverage are correct.
105-117: LGTM!The CTest integration with Catch2 test discovery and the custom run_tests target are properly configured. This enables both
ctestand direct test target execution.tests/test_enums_and_types.cpp (1)
1-476: LGTM!This is a comprehensive and well-structured test suite covering critical enum and type validation. The tests properly verify:
- Sequential and flag-based enum values
- Type safety and size constraints
- Bitwise operations and combinations
- Alignment requirements and padding
- Type traits and compile-time contracts
The test organization with clear sections and descriptive names makes it easy to understand what's being validated. Great work on covering edge cases like power-of-2 checks for flags, overflow prevention, and alignment validation.
tests/test_conversion_utilities.cpp (1)
1-327: LGTM!Excellent test coverage for critical conversion utilities! The test suite demonstrates several strengths:
- Clear documentation explaining why these conversions matter
- Comprehensive coverage of normal cases, edge cases, and real-world validation
- Appropriate use of floating-point matchers (WithinAbs/WithinRel) for numerical accuracy
- Validation of derived constants against base constants
- Bidirectional conversion tests to ensure consistency
The real-world validation section (human height, room dimensions, projectile ranges) is particularly valuable for catching errors that would affect gameplay.
tests/test_simple_utilities.cpp (1)
94-327: LGTM!The test suite provides excellent coverage of utility functions including path normalization, mathematical calculations, formatting, and GPU buffer alignment. The tests are well-organized, use appropriate matchers, and cover both normal and edge cases effectively. The GPU constant buffer alignment tests are particularly valuable for catching subtle bugs.
.github/workflows/test.yaml (3)
1-26: LGTM!The workflow configuration with proper triggers, path filtering, and environment variables follows GitHub Actions best practices. The path filters ensure the workflow only runs when relevant files change, and
workflow_dispatchallows manual execution when needed.
28-86: LGTM!The test job is well-structured with:
- Proper matrix strategy for Release/Debug configurations
- Correct checkout with recursive submodules and full history
- Robust test executable location detection with error handling
- JUnit XML output for test result publishing
- Artifact uploads that run even on failure (if: always())
The test result publishing with composite action provides good CI feedback.
87-214: LGTM!The coverage job is well-implemented with:
- Proper Clang setup for source-based coverage
- Correct LLVM profile data handling with merge and export
- Appropriate ignore patterns for test/build/external files
- HTML report generation for artifacts
- Codecov integration with proper token handling
- Helpful PR comments with coverage links
The coverage workflow follows Clang source-based coverage best practices and provides excellent visibility into test coverage.
vcpkg.json (1)
14-14: File not found in repository.The review comment references
vcpkg.json, but this file does not exist in the repository. Comprehensive searches found no vcpkg-related files. The review cannot be verified against a non-existent file. Confirm whether this comment applies to the current PR or if it was intended for a different change.Likely an incorrect or invalid review comment.
tests/test_gpu_structures.cpp (1)
273-292: Bool packing tests correctly encode the C++↔HLSL contractThese checks (C++
boolvs HLSL 4‑byteboolwithuint32_tsubstitutes) match the project’s convention for GPU‑visible flags and will help catch accidentalboolusage in future structs. Looks good as‑is.
|
✅ A pre-release build is available for this PR: |
Replace test-specific implementations with actual production code: - Tests now compile and link Format.cpp and FileSystem.cpp directly - All 838 assertions test real production functions (Util::FixFilePath, Util::FormatMilliseconds, Util::PathHelpers::*, etc.) Add industry-standard dependency mocking: - Mock ImGui (minimal header stubs) - Mock nlohmann/json (parse, diff, dump, at operations) - Mock Skyrim SDK (REL::Version, logger, globals) - Mock DirectX (D3D types, Windows API) New test coverage: - test_filesystem.cpp: 28 tests for FileSystem.cpp path helpers - test_api_contracts.cpp: 9 tests for compile-time API validation Fixes discovered during testing: - ParseINILine crash on empty lines - Missing #include <iomanip> for std::setprecision - Typo: IsValidDefineNam -> IsValidDefineName Update pre-commit hook to run tests via PowerShell script. 838 assertions, 67 test cases, 100% pass rate. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/README.md (1)
163-163: Inconsistent command format.Line 163 uses shell-style
&&chaining while line 14 shows separate commands. For consistency, use the same format throughout or clarify which shell environment (PowerShell vs bash) each command targets.
🧹 Nitpick comments (1)
tests/README.md (1)
3-3: Hardcoded test metrics will become stale.The specific counts ("812 assertions across 66 test cases") will drift as the test suite evolves, requiring manual updates to the documentation.
Consider one of these approaches:
- Remove specific numbers and use "hundreds of assertions" or similar language
- Add a comment indicating these are approximate/example numbers
- Generate these metrics dynamically in CI and update the README automatically
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.pre-commit-config.yaml(1 hunks)tests/CMakeLists.txt(1 hunks)tests/README.md(1 hunks)tests/test_api_contracts.cpp(1 hunks)tests/test_configuration.cpp(1 hunks)tests/test_filesystem.cpp(1 hunks)tests/test_shader_validation.cpp(1 hunks)tests/test_simple_utilities.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_shader_validation.cpp
- tests/test_filesystem.cpp
- tests/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
tests/test_configuration.cpptests/test_api_contracts.cpptests/test_simple_utilities.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/test_configuration.cpptests/README.mdtests/test_api_contracts.cpptests/test_simple_utilities.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.pre-commit-config.yamltests/README.md
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
tests/test_configuration.cpp
📚 Learning: 2025-06-17T09:27:49.594Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Applied to files:
tests/test_configuration.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Windows, use BuildRelease.bat ALL as the primary build preset; do not cancel builds or shader validation
Applied to files:
tests/README.md
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
tests/test_api_contracts.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: test (Debug)
- GitHub Check: test (Release)
- GitHub Check: coverage
🔇 Additional comments (4)
tests/test_simple_utilities.cpp (1)
1-256: LGTM! Comprehensive test coverage for Format utilities.The test suite thoroughly exercises the production code with:
- Path normalization edge cases (backslashes, consecutive slashes, mixed separators, empty paths)
- Math utility boundary conditions (zero, negative, over 100%)
- Format functions with various input ranges
- GPU buffer alignment validation
- Proper use of Catch2 matchers for floating-point comparisons
The tests follow best practices with clear section names, AAA pattern, and comprehensive edge case coverage.
tests/test_api_contracts.cpp (1)
1-213: Excellent use of compile-time API contract validation!This is a best-practice approach to preventing API breakage:
- Function pointer assignments fail at compile time if signatures change
- Parameter order validation catches subtle bugs (lines 188-213)
- Runtime invariant checks ensure basic behavioral contracts
- Clear documentation explains the purpose and value
This pattern is particularly valuable during refactoring, as it provides immediate feedback when public APIs are inadvertently modified.
tests/test_configuration.cpp (2)
59-104: ParseINILine properly handles edge cases.The trimming logic correctly guards against
nposat lines 66-68, 87-89, and 94-100, preventing the out-of-range errors flagged in previous reviews. The implementation safely handles:
- All-whitespace lines
- Empty strings
- Comments and section headers
- Missing equals signs
115-461: Comprehensive configuration test coverage.The test suite thoroughly validates:
- JSON structure and type checking with edge cases
- Version validation and comparison logic
- INI parsing with various formats (spaces, comments, booleans, numerics)
- File extension validation for multiple types
- Settings path construction patterns
- Error handling for malformed JSON and missing keys
- Multi-level configuration merging
The tests follow Catch2 best practices and provide excellent coverage for configuration-related code.
- Add missing <iomanip> include to test_simple_utilities.cpp (fixes compilation error) - Fix ParseINILine trimming to handle npos from find_last_not_of (fixes runtime crash) - Remove hardcoded personal path from TROUBLESHOOTING.md - Fix inconsistent test count documentation in README.md - Fix command chaining syntax for cross-platform compatibility Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The coverage flags were only applied to the test executable, not to the UtilsForTesting library containing the production code being tested (src/Utils/Format.cpp, src/Utils/FileSystem.cpp). Now applies -fprofile-instr-generate and -fcoverage-mapping to both: - UtilsForTesting library (production code) - CommunityShaders_Tests executable This enables proper coverage measurement of production utility code. Addresses Copilot feedback: captures test exit code immediately to prevent loss between commands, and documents that run_tests.ps1 auto-builds tests so pre-commit hook is safe. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/CMakeLists.txt (3)
17-23: Good start with conservative scope.Starting with pure utility functions (Format.cpp, FileSystem.cpp) is a sensible approach. The comments clearly explain why other files like Game.cpp are excluded due to heavy Skyrim dependencies.
As the test infrastructure matures, consider expanding coverage to additional utility modules. Would you like me to help identify other good candidates for unit testing in
src/Utils/?
142-227: Consider moving generated mocks to dedicated header files.While the
file(WRITE)approach works, maintaining extensive mocks as CMake string literals can become unwieldy as the test suite grows. Consider refactoring these mocks into separate files undertests/mocks/for better maintainability and IDE support.Suggested structure:
tests/ mocks/ test_stubs.h directx_mocks.h skyrim_mocks.h imgui_mocks.hThis would:
- Improve syntax highlighting and IDE navigation
- Make it easier to update mocks when APIs change
- Allow better organization as test coverage expands
- Enable reuse across multiple test targets
You could still use
target_include_directoriesto prioritizetests/mocks/over production headers.
176-181: Consider parameterizing the mock version for future flexibility.The
REL::Versionmock currently returns a hardcoded version string"1.6.640". While this is fine for initial testing, consider making it parameterizable if tests need to verify behavior across different game versions.Optional improvement for future:
namespace REL { struct Version { std::string version_str = "1.6.640"; // Default, but can be set per-test std::string string([[maybe_unused]] const char* separator = ".") const { return version_str; } // ... rest of mock }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-config.yaml(1 hunks)tests/CMakeLists.txt(1 hunks)tests/run_tests.ps1(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
tests/CMakeLists.txttests/run_tests.ps1
🧠 Learnings (3)
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to src/**/*.{cpp,cxx,cc,h,hpp,hxx} : Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: On Linux/WSL, do not attempt to build or validate shaders; limit work to code review, docs, and Python tooling
Applied to files:
.pre-commit-config.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: test (Debug)
- GitHub Check: coverage
- GitHub Check: test (Release)
🔇 Additional comments (8)
tests/run_tests.ps1 (3)
67-68: LGTM! Exit code captured correctly.The test exit code is now captured immediately after test execution, which correctly addresses the concern raised in previous reviews.
70-81: LGTM! Clear result reporting.The exit code checking and colored output provide clear feedback. Proper error propagation with
exit 1ensures CI/CD pipelines will correctly detect test failures.
14-18: Verify build directory structure matches your project's build configuration.The hardcoded
build/ALLpath cannot be verified from the source repository alone, as build directories are generated during compilation. Confirm this path aligns with your project's CMake or build system configuration and that developers following the standard build process will generate test executables in these locations..pre-commit-config.yaml (1)
38-40: LGTM! Appropriate file pattern and helpful comments.The file pattern correctly targets test files and utility code under
src/Utils/, and the comments clearly explain the auto-build behavior and CI handling.tests/CMakeLists.txt (4)
10-13: LGTM! Proper use of CONFIGURE_DEPENDS with globbing.Using
CONFIGURE_DEPENDSensures CMake reconfigures when test files are added or removed, which is the correct approach for test discovery.
34-39: LGTM! Critical include directory ordering.The include directory ordering is correct and the comment appropriately emphasizes its importance. Placing
CMAKE_CURRENT_BINARY_DIRfirst ensures mock headers (likeGlobals.h) intercept includes before production headers, which is essential for the mocking strategy.
108-137: LGTM! Comprehensive coverage configuration.The coverage setup correctly applies instrumentation flags to both the library under test (
UtilsForTesting) and the test executable, which is essential for accurate coverage measurement. The Clang-specific approach and warning message for other compilers are appropriate.
272-282: LGTM! Standard Catch2 integration.The test discovery setup using
catch_discover_testsand the customrun_teststarget provide both CTest integration and a convenient manual run option. The informational message is helpful for developers.
1 participant
Don't merge. Trying something.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.