Skip to content

test: add MSCV case of compiler toolchain test#351

Open
tsurumi-yizhou wants to merge 2 commits intoclice-io:mainfrom
tsurumi-yizhou:mscv-tests
Open

test: add MSCV case of compiler toolchain test#351
tsurumi-yizhou wants to merge 2 commits intoclice-io:mainfrom
tsurumi-yizhou:mscv-tests

Conversation

@tsurumi-yizhou
Copy link
Copy Markdown
Member

@tsurumi-yizhou tsurumi-yizhou commented Jan 22, 2026

Summary by CodeRabbit

  • Tests

    • Improved MSVC toolchain test to run only in appropriate Windows CI environments and to more thoroughly validate compilation behavior.
  • Chores

    • CI workflow updated to ensure MSVC developer tools are prepared on Windows runners before build and test steps.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 22, 2026

📝 Walkthrough

Walkthrough

Adds a full MSVC toolchain unit test and a Windows-specific CI step to set up the MSVC developer environment; the test runs only on CI Windows, queries cl.exe arguments for C++23/EH, compiles a temp file, and asserts zero diagnostics.

Changes

Cohort / File(s) Summary
MSVC Toolchain Test Implementation
tests/unit/Compiler/ToolchainTests.cpp
Replaced placeholder test with a full MSVC test: updated skip condition to require CI and Windows, creates a temporary C++ file, queries the toolchain for cl.exe with C++23 and EH options, captures args, configures CompilationParams to use database-derived args and a file remap, compiles, and asserts success with no diagnostics.
Windows CI: MSVC Setup Step
.github/workflows/test-cmake.yml
Added default pwsh shell and introduced a Windows-only "Setup MSVC Developer Environment (Windows)" step (condition: matrix.os == 'windows-2025') placed before MSVC-dependent setup steps to ensure MSVC tools are available in the Windows workflow.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nudged a temp file into light,

cl.exe hummed C++23 bright,
CI on Windows woke the tools,
Tests now hop and follow rules,
A rabbit cheers for builds done right.

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title contains a typo: 'MSCV' should be 'MSVC'. While the intent is clear and the title accurately describes the main change (adding an MSVC compiler toolchain test), the spelling error makes it technically incorrect. Correct the typo in the title from 'MSCV' to 'MSVC': 'test: add MSVC case of compiler toolchain test'
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@tests/unit/Compiler/ToolchainTests.cpp`:
- Around line 80-112: The test MSVC currently uses only C++11-era code so
dropping "-std:c++23" could go unnoticed; update the remapped file passed to
params.add_remapped_file in the MSVC test to include a compile-time check that
requires C++23 (e.g. a preprocessor guard like `#if` __cplusplus < 202300L /
`#error` "C++23 required" / `#endif` or a static_assert that fails when the language
standard is older), so the test will fail if the -std:c++23 argument (in the
arguments vector passed to toolchain::query_toolchain and used in
params.arguments) is not propagated.

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.

1 participant