Repository navigation
refactor(tests): CMake-based CDB, workspace fixture, test cleanup - #378
Merged
Merged
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates test LSP tooling: adds typed CliceClient APIs (initialize/open/wait), workspace fixture and CDB generator (CMake-backed), migrates tests from TempFile→TempDir with shared write_cdb, adds many CMake module test projects and .cppm formatting, and moves pytest asyncio config to pytest.ini. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Tester
participant FS as FileSystem
participant Client as CliceClient
participant Server as LSP Server
Tester->>Client: initialize(workspace: Path)
Client->>Server: initialize(request with rootUri/workspaceFolders)
Server-->>Client: InitializeResult
Client->>Server: initialized notification
Tester->>FS: read(filepath)
FS-->>Client: content, uri
Client->>Server: textDocument/didOpen (uri, content)
Server->>Server: analyze / compile (may invoke CMake/Ninja for CDB)
Server-->>Client: textDocument/publishDiagnostics (uri)
Client-->>Tester: wait_diagnostics returns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
- Remove TempFile from worker_test_helpers.h, use TempDir from temp_dir.h for isolated temp directories per test case - Move write_cdb() into cdb_helper.h to deduplicate across dependency_graph_tests and compile_graph_integration_tests - Extract shared LSP helpers (lsp_initialize, lsp_open, lsp_open_and_wait, lsp_wait_diagnostics) into conftest.py, removing duplicate _init/_open/ _open_and_wait from test_server.py, test_file_operation.py, test_modules.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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 (5)
tests/conftest.py (1)
199-210: Consider parameterizinglanguage_idfor broader file type support.The helper hardcodes
language_id="cpp"for all files. While this works for.cppfiles, the module tests intest_modules.pyopen.cppmfiles through this helper. If the server relies onlanguage_idfor module-specific processing, this could cause issues.Consider allowing callers to override the language ID when needed:
♻️ Optional: Accept language_id parameter
-def lsp_open(client, filepath: Path, version: int = 0): +def lsp_open(client, filepath: Path, version: int = 0, language_id: str = "cpp"): """Open a text document and return (uri, content).""" content = filepath.read_text(encoding="utf-8") uri = filepath.as_uri() client.text_document_did_open( DidOpenTextDocumentParams( text_document=TextDocumentItem( - uri=uri, language_id="cpp", version=version, text=content + uri=uri, language_id=language_id, version=version, text=content ) ) ) return uri, content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 199 - 210, The helper lsp_open currently hardcodes language_id="cpp"; change its signature to accept an optional language_id parameter (default "cpp") and pass that value into TextDocumentItem when calling client.text_document_did_open so callers can override for .cppm or other files; update any tests that call lsp_open to pass the appropriate language_id where needed (references: lsp_open function and TextDocumentItem in tests/conftest.py).tests/unit/feature/code_completion_tests.cpp (1)
17-31: Prefer function-localvfs/main_pathto avoid shared mutable test state.Keeping these at suite scope is unnecessary and can make parallel runs flaky. Localize both inside
code_complete().Refactor sketch
-llvm::IntrusiveRefCntPtr<TestVFS> vfs; -std::string main_path; @@ void code_complete(llvm::StringRef code) { - vfs = llvm::makeIntrusiveRefCnt<TestVFS>(); + auto vfs = llvm::makeIntrusiveRefCnt<TestVFS>(); @@ - main_path = TestVFS::path("main.cpp"); + auto main_path = TestVFS::path("main.cpp");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/feature/code_completion_tests.cpp` around lines 17 - 31, The global mutable variables vfs and main_path should be made local to code_complete() to avoid shared test state; remove the suite-scope declarations of llvm::IntrusiveRefCntPtr<TestVFS> vfs and std::string main_path and instead create local variables inside code_complete() (e.g., llvm::IntrusiveRefCntPtr<TestVFS> vfs = llvm::makeIntrusiveRefCnt<TestVFS>(); and std::string main_path = TestVFS::path("main.cpp")), update uses in the function where CompilationParams params, AnnotatedSource::from(code), vfs->add(...), params.vfs = vfs, params.arguments and params.completion reference them, and ensure no other tests rely on the removed globals.tests/unit/compile/compilation_tests.cpp (2)
242-273: Test does not verify PCH reuse with the second content version.The test computes bounds for both
content_v1andcontent_v2and verifies they're equal, but only compilesv1. To fully test that "modifying code after the preamble should not require PCH rebuild," the test should also compilev2using the existing PCH and verify it succeeds.♻️ Suggested enhancement to complete the test
// Build PCH with v1. add_main("main.cpp", content_v1); ASSERT_TRUE(compile_with_pch()); ASSERT_TRUE(unit.has_value()); ASSERT_TRUE(unit->top_level_decls().size() >= 1U); + + // Verify v2 can reuse the same PCH (bound is identical). + // Note: This would require exposing the PCH or modifying compile_with_pch + // to accept pre-built PCH info, which may be out of scope for this PR. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/compile/compilation_tests.cpp` around lines 242 - 273, The test PCHContentDifference computes preamble bounds for content_v1 and content_v2 but never verifies PCH reuse for content_v2; after building the PCH with content_v1 (using add_main("main.cpp", content_v1) and ASSERT_TRUE(compile_with_pch())), update the test to replace the main with content_v2 (e.g., call add_main("main.cpp", content_v2)), invoke compile_with_pch() again and ASSERT_TRUE on its result, then assert unit.has_value() and that unit->top_level_decls().size() is >= 1U to confirm compilation succeeded without rebuilding PCH.
175-240: Test cleanup may leave orphaned files on early assertion failure.The temp PCM files created via
fs::createTemporaryFileare only cleaned up at the end of the test. If anyASSERT_*fails before lines 238-239, the files will remain. Consider using RAII-based cleanup or moving cleanup to a scope guard.♻️ Optional: RAII-based cleanup pattern
+ // Helper for automatic cleanup + auto cleanup_file = [](const std::string& path) { + llvm::sys::fs::remove(path); + }; + std::unique_ptr<std::string, decltype(cleanup_file)> pcm_b_guard; + std::unique_ptr<std::string, decltype(cleanup_file)> pcm_a_guard; + auto pcm_b_path = fs::createTemporaryFile("mod_b", "pcm"); ASSERT_TRUE(pcm_b_path.operator bool()); + pcm_b_guard = std::unique_ptr<std::string, decltype(cleanup_file)>( + new std::string(*pcm_b_path), cleanup_file); params_b.output_file = *pcm_b_path; // ... similar for pcm_a_path ... - - // Clean up temp PCM files. - llvm::sys::fs::remove(*pcm_b_path); - llvm::sys::fs::remove(*pcm_a_path);Alternatively, if the test framework supports it, use a test fixture with teardown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/compile/compilation_tests.cpp` around lines 175 - 240, The test currently creates temporary PCM files via fs::createTemporaryFile (pcm_b_path, pcm_a_path) and only calls llvm::sys::fs::remove at the end, which leaks files on early ASSERT_* failures; wrap the temporary-paths in an RAII cleanup (e.g., a small ScopedTempFile or scope guard) that stores the Optional<Path> returned by fs::createTemporaryFile and removes the file in its destructor, or register a teardown/scope-exit that always calls llvm::sys::fs::remove for pcm_b_path and pcm_a_path so cleanup runs even if unit_b or unit_a assertions fail.tests/unit/test/tester.cpp (1)
201-281: Consider extracting common setup logic to reduce duplication.The initial setup (lines 202-217) duplicates
prepare_driver()'s argument resolution logic. Similarly, the file remapping loops (lines 235-243 and 262-269) share identical path-handling logic. While the current implementation is correct, extracting a helper could improve maintainability.♻️ Optional: Extract shared logic
// Private helper for driver argument setup void Tester::setup_driver_args(llvm::StringRef standard) { params = CompilationParams(); unit.reset(); vfs = llvm::makeIntrusiveRefCnt<TestVFS>(); for(auto& [file, source]: sources.all_files) { vfs->add(file, source.content); } auto command = std::format("clang++ {} {} -fms-extensions", standard, src_path); database.add_command("fake", src_path, command); CommandOptions options; options.query_toolchain = true; options.suppress_logging = true; auto commands = database.lookup(src_path, options); assert(!commands.empty() && "lookup failed after add_command"); params.arguments = commands.front().arguments; } // Private helper for remapping files void Tester::remap_sources(std::optional<std::size_t> main_bound = std::nullopt) { for(auto& [file, source]: sources.all_files) { if(file == src_path) { if(main_bound) { params.add_remapped_file(file, source.content, *main_bound); } else { params.add_remapped_file(file, source.content); } } else { std::string path = path::is_absolute(file) ? file.str() : path::join(".", file); params.add_remapped_file(path, source.content); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/tester.cpp` around lines 201 - 281, The method Tester::compile_driver_with_pch duplicates driver argument resolution and file-remapping logic; extract the initial setup block (params reset, unit.reset, vfs creation and population, command creation/database.add_command, CommandOptions lookup and params.arguments assignment) into a new private helper Tester::setup_driver_args(llvm::StringRef standard) and replace the duplicated block with a call to it, and extract the two remapping loops into a private Tester::remap_sources(std::optional<std::size_t> main_bound = std::nullopt) that adds remapped files (using path::is_absolute/file.str or path::join for non-src_path entries and the optional bound for the main file); then call remap_sources(bound) for the preamble phase and remap_sources() for the content phase, and update any other callers like prepare_driver() to use setup_driver_args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/semantic/ast_utility.cpp`:
- Around line 892-895: The detection logic that treats any declaration under a
top-level std ancestor as std::forward is too broad; change the loop that walks
DeclContext (used by the std::forward check and by unwrapForward) to ensure
every namespace between the callee and the top-level "std" is an inline
namespace. Concretely, when iterating DC = Callee->getDeclContext() upwards and
llvm::dyn_casting to clang::NamespaceDecl, require NS->isInline() for every
intermediate namespace (allowing the final match where identifier_of(*NS) ==
"std" and NS->getParent()->isTranslationUnit()); only return true if that
inline-namespace chain condition holds. Update the same check used by
unwrapForward accordingly.
In `@tests/integration/test_modules.py`:
- Line 22: Remove the unused import lsp_wait_diagnostics from the import list in
tests/integration/test_modules.py: keep only the actually used helpers
(lsp_initialize, lsp_open, lsp_open_and_wait) since diagnostics are already
awaited via lsp_open_and_wait; update the import statement to eliminate
lsp_wait_diagnostics to resolve the unused-import warning.
In `@tests/integration/test_server.py`:
- Line 30: Remove the unused import lsp_wait_diagnostics from the import list in
tests/integration/test_server.py; update the line that currently imports
lsp_initialize, lsp_open, lsp_open_and_wait, lsp_wait_diagnostics to only import
lsp_initialize, lsp_open, lsp_open_and_wait (since lsp_open_and_wait already
calls lsp_wait_diagnostics), then run the test suite to ensure no regressions.
In `@tests/unit/feature/signature_help_tests.cpp`:
- Around line 19-21: The test indexes nameless_points()[0] without checking that
nameless_points() contains any markers, which can crash on malformed input; add
a guard before using nameless_points()[0] (e.g., assert or conditional) in the
test setup where params.completion is assigned so that if
nameless_points().empty() you fail the test with a clear message or skip setting
params.completion, and ensure the rest of the setup lines
(TestVFS::path("main.cpp"), params.add_remapped_file(...),
sources.all_files["main.cpp"].content) are only executed when a valid marker
exists.
In `@tests/unit/test/cdb_helper.h`:
- Around line 22-31: The json_escape function only handles backslash and
double-quote and must also escape JSON control characters; update inline
std::string json_escape(llvm::StringRef s) to map control chars (0x00–0x1F) to
their JSON-escaped forms (use "\b", "\f", "\n", "\r", "\t" for those names and
otherwise "\u00XX" hex escapes for other control bytes) while still escaping
'\\' and '"' so generated compile_commands.json contains valid JSON string
values.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 199-210: The helper lsp_open currently hardcodes
language_id="cpp"; change its signature to accept an optional language_id
parameter (default "cpp") and pass that value into TextDocumentItem when calling
client.text_document_did_open so callers can override for .cppm or other files;
update any tests that call lsp_open to pass the appropriate language_id where
needed (references: lsp_open function and TextDocumentItem in
tests/conftest.py).
In `@tests/unit/compile/compilation_tests.cpp`:
- Around line 242-273: The test PCHContentDifference computes preamble bounds
for content_v1 and content_v2 but never verifies PCH reuse for content_v2; after
building the PCH with content_v1 (using add_main("main.cpp", content_v1) and
ASSERT_TRUE(compile_with_pch())), update the test to replace the main with
content_v2 (e.g., call add_main("main.cpp", content_v2)), invoke
compile_with_pch() again and ASSERT_TRUE on its result, then assert
unit.has_value() and that unit->top_level_decls().size() is >= 1U to confirm
compilation succeeded without rebuilding PCH.
- Around line 175-240: The test currently creates temporary PCM files via
fs::createTemporaryFile (pcm_b_path, pcm_a_path) and only calls
llvm::sys::fs::remove at the end, which leaks files on early ASSERT_* failures;
wrap the temporary-paths in an RAII cleanup (e.g., a small ScopedTempFile or
scope guard) that stores the Optional<Path> returned by fs::createTemporaryFile
and removes the file in its destructor, or register a teardown/scope-exit that
always calls llvm::sys::fs::remove for pcm_b_path and pcm_a_path so cleanup runs
even if unit_b or unit_a assertions fail.
In `@tests/unit/feature/code_completion_tests.cpp`:
- Around line 17-31: The global mutable variables vfs and main_path should be
made local to code_complete() to avoid shared test state; remove the suite-scope
declarations of llvm::IntrusiveRefCntPtr<TestVFS> vfs and std::string main_path
and instead create local variables inside code_complete() (e.g.,
llvm::IntrusiveRefCntPtr<TestVFS> vfs = llvm::makeIntrusiveRefCnt<TestVFS>();
and std::string main_path = TestVFS::path("main.cpp")), update uses in the
function where CompilationParams params, AnnotatedSource::from(code),
vfs->add(...), params.vfs = vfs, params.arguments and params.completion
reference them, and ensure no other tests rely on the removed globals.
In `@tests/unit/test/tester.cpp`:
- Around line 201-281: The method Tester::compile_driver_with_pch duplicates
driver argument resolution and file-remapping logic; extract the initial setup
block (params reset, unit.reset, vfs creation and population, command
creation/database.add_command, CommandOptions lookup and params.arguments
assignment) into a new private helper Tester::setup_driver_args(llvm::StringRef
standard) and replace the duplicated block with a call to it, and extract the
two remapping loops into a private
Tester::remap_sources(std::optional<std::size_t> main_bound = std::nullopt) that
adds remapped files (using path::is_absolute/file.str or path::join for
non-src_path entries and the optional bound for the main file); then call
remap_sources(bound) for the preamble phase and remap_sources() for the content
phase, and update any other callers like prepare_driver() to use
setup_driver_args.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db36cd05-db0e-4fae-8488-e0710a99d19e
📒 Files selected for processing (36)
src/semantic/ast_utility.cpptests/conftest.pytests/integration/test_file_operation.pytests/integration/test_lifecycle.pytests/integration/test_modules.pytests/integration/test_server.pytests/unit/command/argument_parser_tests.cpptests/unit/command/command_tests.cpptests/unit/command/toolchain_tests.cpptests/unit/compile/compilation_tests.cpptests/unit/compile/diagnostic_tests.cpptests/unit/compile/directive_tests.cpptests/unit/compile/tidy_tests.cpptests/unit/feature/code_completion_tests.cpptests/unit/feature/document_link_tests.cpptests/unit/feature/document_symbol_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/feature/hover_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/feature/signature_help_tests.cpptests/unit/index/merged_index_tests.cpptests/unit/index/project_index_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/semantic/selection_tests.cpptests/unit/semantic/template_resolver_tests.cpptests/unit/server/compile_graph_integration_tests.cpptests/unit/server/module_worker_tests.cpptests/unit/server/stateful_worker_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/server/worker_test_helpers.htests/unit/syntax/dependency_graph_tests.cpptests/unit/test/annotation.htests/unit/test/cdb_helper.htests/unit/test/tester.cpptests/unit/test/tester.h
16bit-ykiko
force-pushed
the
feat/compile-index-tests
branch
from
March 31, 2026 03:08
b64ee8d to
5fdd857
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test/cdb_helper.h (1)
62-65: Exposeload()result to avoid silent test setup failures.On Line 64, the loaded-entry count is discarded. Returning it from
write_cdb(...)makes failures easier to catch at call sites (especially when JSON is malformed or file creation fails).Suggested diff
-inline void write_cdb(TempDir& tmp, CompilationDatabase& cdb, llvm::StringRef json_content) { +inline std::size_t write_cdb(TempDir& tmp, CompilationDatabase& cdb, llvm::StringRef json_content) { tmp.touch("compile_commands.json", json_content); - cdb.load(tmp.path("compile_commands.json")); + return cdb.load(tmp.path("compile_commands.json")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/cdb_helper.h` around lines 62 - 65, The helper write_cdb currently discards the return value of CompilationDatabase::load which can hide test setup failures; change write_cdb(TempDir& tmp, CompilationDatabase& cdb, llvm::StringRef json_content) to return the load result (e.g., an int or size_t) and propagate the value from cdb.load(tmp.path("compile_commands.json")) so callers can assert the loaded-entry count after creating the compile_commands.json file via TempDir::touch; ensure callers in tests are updated to check the returned count where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test/cdb_helper.h`:
- Around line 62-65: The helper write_cdb currently discards the return value of
CompilationDatabase::load which can hide test setup failures; change
write_cdb(TempDir& tmp, CompilationDatabase& cdb, llvm::StringRef json_content)
to return the load result (e.g., an int or size_t) and propagate the value from
cdb.load(tmp.path("compile_commands.json")) so callers can assert the
loaded-entry count after creating the compile_commands.json file via
TempDir::touch; ensure callers in tests are updated to check the returned count
where appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e19c34fb-8b8e-4eda-8d80-736227b52429
📒 Files selected for processing (12)
tests/conftest.pytests/integration/test_file_operation.pytests/integration/test_lifecycle.pytests/integration/test_modules.pytests/integration/test_server.pytests/unit/server/compile_graph_integration_tests.cpptests/unit/server/module_worker_tests.cpptests/unit/server/stateful_worker_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/server/worker_test_helpers.htests/unit/syntax/dependency_graph_tests.cpptests/unit/test/cdb_helper.h
💤 Files with no reviewable changes (2)
- tests/unit/server/compile_graph_integration_tests.cpp
- tests/unit/syntax/dependency_graph_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/unit/server/module_worker_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/integration/test_lifecycle.py
- tests/unit/server/worker_test_helpers.h
- tests/integration/test_modules.py
- tests/conftest.py
- tests/unit/server/stateful_worker_tests.cpp
Move lsp_initialize/lsp_open/lsp_open_and_wait/lsp_wait_diagnostics from free functions into CliceClient as initialize/open/open_and_wait/ wait_diagnostics methods for cleaner API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 122-126: The method open_and_wait calls self.open(filepath) before
setting up the diagnostics wait, causing races if diagnostics arrive
immediately; change the flow in open_and_wait so you prepare/subscribe the
diagnostics wait (i.e., call or create the wait event via self.wait_diagnostics
or equivalent subscription) before invoking self.open(filepath), then open the
file and await the previously prepared diagnostics future; update references in
tests/conftest.py to ensure open_and_wait uses the pre-created diagnostics
waiter (and that on_diagnostics sets that event) to avoid intermittent timeouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a357c11-3df7-4cb8-8a8e-7feb0b63b1f9
📒 Files selected for processing (5)
tests/conftest.pytests/integration/test_file_operation.pytests/integration/test_lifecycle.pytests/integration/test_modules.pytests/integration/test_server.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/test_lifecycle.py
- tests/integration/test_server.py
- tests/integration/test_file_operation.py
Replace hand-written compile_commands.json with per-directory CMakeLists.txt and cmake -G Ninja generation for more realistic integration test coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/data/modules/independent_modules/CMakeLists.txt (1)
4-5: Make the C++ standard non-negotiable in this fixture.Please add
CMAKE_CXX_STANDARD_REQUIRED ONto avoid silent downgrade from C++20.Suggested patch
set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/modules/independent_modules/CMakeLists.txt` around lines 4 - 5, Add an explicit requirement that the C++ standard cannot be downgraded by setting CMAKE_CXX_STANDARD_REQUIRED ON in the CMakeLists fixture: locate the existing set(CMAKE_CXX_STANDARD 20) (and the related set(CMAKE_CXX_EXTENSIONS OFF)) and insert a set(CMAKE_CXX_STANDARD_REQUIRED ON) immediately after them so CMake enforces C++20.tests/data/modules/deep_chain/CMakeLists.txt (1)
4-5: Require C++20 explicitly to keep fixture behavior deterministic.Add
CMAKE_CXX_STANDARD_REQUIRED ONso CI/toolchain differences can’t downgrade the language mode.Suggested patch
set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/modules/deep_chain/CMakeLists.txt` around lines 4 - 5, Add an explicit requirement that the C++ standard not be downgraded by setting CMAKE_CXX_STANDARD_REQUIRED ON in the same CMakeLists context that defines the standard (near set(CMAKE_CXX_STANDARD 20) and set(CMAKE_CXX_EXTENSIONS OFF)); this ensures the C++20 mode is enforced across CI/toolchains by making CMAKE_CXX_STANDARD required rather than optional.tests/data/modules/module_partitions/CMakeLists.txt (1)
4-5: Enforce C++20 as a hard requirement for this fixture.Set
CMAKE_CXX_STANDARD_REQUIRED ONso this test fixture never silently falls back to an older standard on unsupported toolchains.Suggested patch
set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/modules/module_partitions/CMakeLists.txt` around lines 4 - 5, The CMakeLists currently sets CMAKE_CXX_STANDARD to 20 but doesn't enforce it; update the fixture to require C++20 by adding a directive to enable strict standard enforcement—specifically add CMAKE_CXX_STANDARD_REQUIRED set to ON (in the same scope where set(CMAKE_CXX_STANDARD 20) and set(CMAKE_CXX_EXTENSIONS OFF) are defined) so the build will fail rather than silently falling back on older standards.tests/data/modules/dotted_module_name/CMakeLists.txt (1)
4-5: Apply strict C++20 enforcement here as well.Recommend adding
CMAKE_CXX_STANDARD_REQUIRED ONfor consistent fixture behavior across environments.Suggested patch
set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/modules/dotted_module_name/CMakeLists.txt` around lines 4 - 5, The CMakeLists currently sets CMAKE_CXX_STANDARD and CMAKE_CXX_EXTENSIONS but doesn't enforce the standard; update the file to explicitly require C++20 by adding set(CMAKE_CXX_STANDARD_REQUIRED ON) so that the CMake target honors CMAKE_CXX_STANDARD (refer to the existing set(CMAKE_CXX_STANDARD 20) and set(CMAKE_CXX_EXTENSIONS OFF) lines) and ensures consistent fixture behavior across environments.tests/integration/test_modules.py (1)
53-54: Replace fixed sleep with readiness-based synchronization.A hardcoded
sleep(2.0)can be flaky and adds cumulative test latency. Prefer waiting on a concrete readiness signal/event from the client/server handshake path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` around lines 53 - 54, Replace the brittle hardcoded await asyncio.sleep(2.0) with a readiness-based wait: remove the sleep and await a concrete readiness signal such as a client/server handshake completion API or event (for example await client.wait_until_ready() or await server_ready_event.wait()), or add a small helper like wait_for_ready() that polls/awaits the connection/handshake state (e.g., client.handshake_complete or server.signaled_ready) so the test proceeds only when the server is truly ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_modules.py`:
- Around line 35-46: Replace the literal "cmake" in the subprocess.run call with
an absolute path resolved via shutil.which("cmake") and fail-fast if it returns
None; update the argument list passed to subprocess.run (the call in
tests/integration/test_modules.py where subprocess.run(...) builds the CMake
command) to use that resolved path instead of the string, and add a timeout
parameter (e.g., timeout=60) to the subprocess.run invocation to avoid hanging
in CI; ensure you do not enable shell=True and keep check=True and
capture_output/Text as before.
---
Nitpick comments:
In `@tests/data/modules/deep_chain/CMakeLists.txt`:
- Around line 4-5: Add an explicit requirement that the C++ standard not be
downgraded by setting CMAKE_CXX_STANDARD_REQUIRED ON in the same CMakeLists
context that defines the standard (near set(CMAKE_CXX_STANDARD 20) and
set(CMAKE_CXX_EXTENSIONS OFF)); this ensures the C++20 mode is enforced across
CI/toolchains by making CMAKE_CXX_STANDARD required rather than optional.
In `@tests/data/modules/dotted_module_name/CMakeLists.txt`:
- Around line 4-5: The CMakeLists currently sets CMAKE_CXX_STANDARD and
CMAKE_CXX_EXTENSIONS but doesn't enforce the standard; update the file to
explicitly require C++20 by adding set(CMAKE_CXX_STANDARD_REQUIRED ON) so that
the CMake target honors CMAKE_CXX_STANDARD (refer to the existing
set(CMAKE_CXX_STANDARD 20) and set(CMAKE_CXX_EXTENSIONS OFF) lines) and ensures
consistent fixture behavior across environments.
In `@tests/data/modules/independent_modules/CMakeLists.txt`:
- Around line 4-5: Add an explicit requirement that the C++ standard cannot be
downgraded by setting CMAKE_CXX_STANDARD_REQUIRED ON in the CMakeLists fixture:
locate the existing set(CMAKE_CXX_STANDARD 20) (and the related
set(CMAKE_CXX_EXTENSIONS OFF)) and insert a set(CMAKE_CXX_STANDARD_REQUIRED ON)
immediately after them so CMake enforces C++20.
In `@tests/data/modules/module_partitions/CMakeLists.txt`:
- Around line 4-5: The CMakeLists currently sets CMAKE_CXX_STANDARD to 20 but
doesn't enforce it; update the fixture to require C++20 by adding a directive to
enable strict standard enforcement—specifically add CMAKE_CXX_STANDARD_REQUIRED
set to ON (in the same scope where set(CMAKE_CXX_STANDARD 20) and
set(CMAKE_CXX_EXTENSIONS OFF) are defined) so the build will fail rather than
silently falling back on older standards.
In `@tests/integration/test_modules.py`:
- Around line 53-54: Replace the brittle hardcoded await asyncio.sleep(2.0) with
a readiness-based wait: remove the sleep and await a concrete readiness signal
such as a client/server handshake completion API or event (for example await
client.wait_until_ready() or await server_ready_event.wait()), or add a small
helper like wait_for_ready() that polls/awaits the connection/handshake state
(e.g., client.handshake_complete or server.signaled_ready) so the test proceeds
only when the server is truly ready.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6285a383-aa28-4da8-86e8-fcf51c3e1282
📒 Files selected for processing (27)
tests/data/modules/chained_modules/CMakeLists.txttests/data/modules/circular_module_dependency/CMakeLists.txttests/data/modules/class_export_and_inheritance/CMakeLists.txttests/data/modules/consumer_imports_module/CMakeLists.txttests/data/modules/deep_chain/CMakeLists.txttests/data/modules/diamond_modules/CMakeLists.txttests/data/modules/dotted_module_name/CMakeLists.txttests/data/modules/export_block/CMakeLists.txttests/data/modules/export_namespace/CMakeLists.txttests/data/modules/global_module_fragment/CMakeLists.txttests/data/modules/gmf_with_import/CMakeLists.txttests/data/modules/hover_on_imported_symbol/CMakeLists.txttests/data/modules/independent_modules/CMakeLists.txttests/data/modules/module_compile_error/CMakeLists.txttests/data/modules/module_implementation_unit/CMakeLists.txttests/data/modules/module_partitions/CMakeLists.txttests/data/modules/no_modules_plain_cpp/CMakeLists.txttests/data/modules/partition_chain/CMakeLists.txttests/data/modules/partition_interface/CMakeLists.txttests/data/modules/partition_with_external_import/CMakeLists.txttests/data/modules/partition_with_gmf/CMakeLists.txttests/data/modules/private_module_fragment/CMakeLists.txttests/data/modules/re_export/CMakeLists.txttests/data/modules/save_recompile/CMakeLists.txttests/data/modules/single_module_no_deps/CMakeLists.txttests/data/modules/template_export/CMakeLists.txttests/integration/test_modules.py
✅ Files skipped from review due to trivial changes (22)
- tests/data/modules/partition_with_external_import/CMakeLists.txt
- tests/data/modules/global_module_fragment/CMakeLists.txt
- tests/data/modules/consumer_imports_module/CMakeLists.txt
- tests/data/modules/circular_module_dependency/CMakeLists.txt
- tests/data/modules/hover_on_imported_symbol/CMakeLists.txt
- tests/data/modules/template_export/CMakeLists.txt
- tests/data/modules/partition_chain/CMakeLists.txt
- tests/data/modules/module_implementation_unit/CMakeLists.txt
- tests/data/modules/no_modules_plain_cpp/CMakeLists.txt
- tests/data/modules/export_namespace/CMakeLists.txt
- tests/data/modules/save_recompile/CMakeLists.txt
- tests/data/modules/chained_modules/CMakeLists.txt
- tests/data/modules/export_block/CMakeLists.txt
- tests/data/modules/single_module_no_deps/CMakeLists.txt
- tests/data/modules/partition_with_gmf/CMakeLists.txt
- tests/data/modules/diamond_modules/CMakeLists.txt
- tests/data/modules/gmf_with_import/CMakeLists.txt
- tests/data/modules/private_module_fragment/CMakeLists.txt
- tests/data/modules/re_export/CMakeLists.txt
- tests/data/modules/class_export_and_inheritance/CMakeLists.txt
- tests/data/modules/partition_interface/CMakeLists.txt
- tests/data/modules/module_compile_error/CMakeLists.txt
- Resolve cmake path via shutil.which() and add subprocess timeout - Fix race in open_and_wait: arm diagnostics event before opening doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/test_modules.py (1)
400-402:⚠️ Potential issue | 🟠 MajorLine 400 reintroduces the open/diagnostics race.
Line 400 sends
didOpenbefore the waiter exists, so a fast publish can be missed and Line 402 will intermittently time out. Reuseclient.open_and_wait()here, or armwait_for_diagnostics()beforeclient.open().🔧 Suggested patch
- leaf_uri, _ = client.open(tmp_path / "leaf.cppm") - event = client.wait_for_diagnostics(leaf_uri) - await asyncio.wait_for(event.wait(), timeout=60.0) + leaf_uri, _ = await client.open_and_wait(tmp_path / "leaf.cppm")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` around lines 400 - 402, The test reintroduces an open/diagnostics race by calling client.open(...) before arming the diagnostics waiter; change the sequence so the diagnostic waiter is created before sending didOpen or simply replace the pair with client.open_and_wait(tmp_path / "leaf.cppm") to atomically open and wait for diagnostics; locate the calls to client.open, client.wait_for_diagnostics and event.wait in tests/integration/test_modules.py and either call client.wait_for_diagnostics(...) first and then client.open(...), or swap both lines for a single client.open_and_wait(...) call to reliably avoid the race.
🧹 Nitpick comments (1)
tests/integration/test_modules.py (1)
32-56: Avoid generating CMake output inside the checked-in fixtures.Lines 37-56 create
build/and copycompile_commands.jsonback into the source tree undertests/data/modules/*. That leaves these fixtures stateful across runs and dirties the worktree after local test execution. Thetmp_pathpattern already used intest_save_recompileis safer here too—copy each fixture into a per-test temp dir before invoking CMake.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` around lines 32 - 56, The helper _generate_cdb is creating build/ and copying compile_commands.json back into the original fixture workspace, which dirties checked-in fixtures; instead, update the tests to copy the fixture directory into a per-test temporary directory (follow the tmp_path usage in test_save_recompile) and call _generate_cdb on that temp copy so CMake runs and output stay isolated, and modify _generate_cdb so it only writes compile_commands.json inside the provided workspace (or returns the path to the generated compile_commands.json) without mutating the original fixture; ensure the tests invoke the temp-copy pattern before calling _generate_cdb and remove the shutil.copy2 back into source-tree behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_modules.py`:
- Around line 400-402: The test reintroduces an open/diagnostics race by calling
client.open(...) before arming the diagnostics waiter; change the sequence so
the diagnostic waiter is created before sending didOpen or simply replace the
pair with client.open_and_wait(tmp_path / "leaf.cppm") to atomically open and
wait for diagnostics; locate the calls to client.open,
client.wait_for_diagnostics and event.wait in tests/integration/test_modules.py
and either call client.wait_for_diagnostics(...) first and then
client.open(...), or swap both lines for a single client.open_and_wait(...) call
to reliably avoid the race.
---
Nitpick comments:
In `@tests/integration/test_modules.py`:
- Around line 32-56: The helper _generate_cdb is creating build/ and copying
compile_commands.json back into the original fixture workspace, which dirties
checked-in fixtures; instead, update the tests to copy the fixture directory
into a per-test temporary directory (follow the tmp_path usage in
test_save_recompile) and call _generate_cdb on that temp copy so CMake runs and
output stay isolated, and modify _generate_cdb so it only writes
compile_commands.json inside the provided workspace (or returns the path to the
generated compile_commands.json) without mutating the original fixture; ensure
the tests invoke the temp-copy pattern before calling _generate_cdb and remove
the shutil.copy2 back into source-tree behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6963b6ed-f1a5-4315-9349-328c052065b5
📒 Files selected for processing (2)
tests/conftest.pytests/integration/test_modules.py
Member
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/test_modules.py (1)
399-402:⚠️ Potential issue | 🟠 MajorArm the diagnostics wait before opening
leaf.cppm.Line 400 reintroduces the same missed-notification race: if diagnostics arrive immediately after
client.open(...), the event created on Line 401 never gets set and the test can hang intermittently. Reuseopen_and_wait()here, or create the event before callingopen().🔧 Suggested patch
- leaf_uri, _ = client.open(tmp_path / "leaf.cppm") - event = client.wait_for_diagnostics(leaf_uri) - await asyncio.wait_for(event.wait(), timeout=60.0) + leaf_uri, _ = await client.open_and_wait(tmp_path / "leaf.cppm")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` around lines 399 - 402, The test risks a missed-notification race because it calls client.open(...) before arming the diagnostics wait; change the sequence to arm the diagnostics listener first (call client.wait_for_diagnostics(...) or reuse the helper open_and_wait() that both opens and waits) so the Event is created before invoking client.open for "leaf.cppm" (refer to client.open, client.wait_for_diagnostics, and open_and_wait) ensuring the event can be set even if diagnostics arrive immediately.
🧹 Nitpick comments (1)
tests/integration/test_modules.py (1)
59-64: Avoid the fixed post-initialize sleep.Line 63 makes every module test pay a hard-coded 2s and can still race on slower runners. Since
CliceClientalready records work-done progress, prefer waiting on a concrete “CDB scan finished” signal instead of sleeping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` around lines 59 - 64, Replace the fixed asyncio.sleep(2.0) in the _init function with a deterministic wait for the CDB scan work-done progress recorded by CliceClient: after calling client.initialize(workspace) poll or await the client's work-done/progress API (e.g., client.wait_for_work_done, client.wait_for_progress_completion, or loop until client.work_done_records shows the "CDB scan finished" entry) and return only once that specific "CDB scan finished" progress is observed, ensuring tests don't rely on a hard-coded sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_modules.py`:
- Around line 399-402: The test risks a missed-notification race because it
calls client.open(...) before arming the diagnostics wait; change the sequence
to arm the diagnostics listener first (call client.wait_for_diagnostics(...) or
reuse the helper open_and_wait() that both opens and waits) so the Event is
created before invoking client.open for "leaf.cppm" (refer to client.open,
client.wait_for_diagnostics, and open_and_wait) ensuring the event can be set
even if diagnostics arrive immediately.
---
Nitpick comments:
In `@tests/integration/test_modules.py`:
- Around line 59-64: Replace the fixed asyncio.sleep(2.0) in the _init function
with a deterministic wait for the CDB scan work-done progress recorded by
CliceClient: after calling client.initialize(workspace) poll or await the
client's work-done/progress API (e.g., client.wait_for_work_done,
client.wait_for_progress_completion, or loop until client.work_done_records
shows the "CDB scan finished" entry) and return only once that specific "CDB
scan finished" progress is observed, ensuring tests don't rely on a hard-coded
sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5d0a091-31f0-4dd9-942e-f54c91c336ce
📒 Files selected for processing (2)
tests/conftest.pytests/integration/test_modules.py
…port The previous CMakeLists.txt used a simple add_library(OBJECT) which doesn't enable C++20 module compilation. Use cmake 3.28+ FILE_SET CXX_MODULES so the generated CDB includes correct module flags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The server finds compile_commands.json in the build/ subdirectory automatically, no need to copy it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…port Use FILE_SET CXX_MODULES in all test CMakeLists.txt so cmake generates module-aware compile commands (-fmodules-ts, -fmodule-mapper, etc.). Also add .cppm to format-cpp task and apply clang-format to test data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t setup
Replace manual `test_data_dir / "subdir"` paths with a `@pytest.mark.workspace("subdir")`
marker and `ws` fixture that auto-resolves the workspace path and generates CDB via CMake.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/test_modules.py (1)
351-353:⚠️ Potential issue | 🟠 MajorArm diagnostics wait before opening
leaf.cppmto avoid flakiness.On Line 351,
client.open(...)happens before the waiter is created on Line 352. If diagnostics arrive immediately, the event is missed and this can intermittently timeout.🔧 Proposed fix
- leaf_uri, _ = client.open(tmp_path / "leaf.cppm") - event = client.wait_for_diagnostics(leaf_uri) - await asyncio.wait_for(event.wait(), timeout=60.0) + leaf_uri, _ = await client.open_and_wait(tmp_path / "leaf.cppm")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` around lines 351 - 353, The test creates the diagnostics waiter after opening the file which can miss immediately arriving diagnostics; swap the operations so you call client.wait_for_diagnostics(...) to get the event before calling client.open(tmp_path / "leaf.cppm") and then await the event (i.e., use the event returned by client.wait_for_diagnostics for leaf_uri/filename prior to invoking client.open) to ensure the test reliably observes diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_modules.py`:
- Around line 351-353: The test creates the diagnostics waiter after opening the
file which can miss immediately arriving diagnostics; swap the operations so you
call client.wait_for_diagnostics(...) to get the event before calling
client.open(tmp_path / "leaf.cppm") and then await the event (i.e., use the
event returned by client.wait_for_diagnostics for leaf_uri/filename prior to
invoking client.open) to ensure the test reliably observes diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b4b287d-4f16-405d-be3b-d7e2619770c2
📒 Files selected for processing (5)
tests/conftest.pytests/integration/test_file_operation.pytests/integration/test_lifecycle.pytests/integration/test_modules.pytests/integration/test_server.py
✅ Files skipped from review due to trivial changes (1)
- tests/integration/test_file_operation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/test_lifecycle.py
- tests/integration/test_server.py
Merge workspace resolution and LSP initialization into the client fixture so tests no longer need manual `await client.initialize()` calls. Rename `ws` to `workspace` and add type annotations throughout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixture types are inferred by pytest; annotations in test cases are redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes Pylance type inference for the yielding client fixture. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/conftest.py (2)
209-222: Consider caching the CDB generation to avoid redundant CMake runs.The
workspacefixture runsgenerate_cdb()on every test that uses the same workspace directory. For workspaces withCMakeLists.txt, this could redundantly regeneratecompile_commands.jsonmultiple times during a test session. Consider checking ifbuild/compile_commands.jsonalready exists before regenerating.♻️ Proposed optimization
`@pytest.fixture` def workspace(request: pytest.FixtureRequest, test_data_dir: Path) -> Path | None: """Resolve workspace path from `@pytest.mark.workspace`("subdir") marker. If the workspace contains a CMakeLists.txt, automatically runs cmake to generate compile_commands.json. Returns None if no marker is present. """ marker = request.node.get_closest_marker("workspace") if marker is None: return None path = test_data_dir / marker.args[0] - if (path / "CMakeLists.txt").exists(): + cdb_path = path / "build" / "compile_commands.json" + if (path / "CMakeLists.txt").exists() and not cdb_path.exists(): generate_cdb(path) return path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 209 - 222, The workspace fixture currently calls generate_cdb(path) whenever a CMakeLists.txt exists, causing redundant CMake runs; modify the logic in the workspace fixture to first check for an existing compile_commands.json (e.g. path / "build" / "compile_commands.json") and only call generate_cdb(path) if that file is missing or stale, keeping the existing checks around (path / "CMakeLists.txt"). Ensure you reference the workspace fixture and the generate_cdb function when implementing the presence check so repeated tests sharing the same workspace skip unnecessary regeneration.
186-206: Consider checking for Ninja availability alongside CMake.
generate_cdbrequires bothcmakeandninjato be available (since it uses-G Ninja). If Ninja is missing, CMake will fail with a confusing error. Consider validating Ninja availability upfront for a clearer error message.💡 Proposed improvement
def generate_cdb(workspace: Path) -> None: """Generate compile_commands.json using CMake with Ninja backend.""" cmake = shutil.which("cmake") if cmake is None: raise RuntimeError("cmake executable not found in PATH") + ninja = shutil.which("ninja") + if ninja is None: + raise RuntimeError("ninja executable not found in PATH (required for -G Ninja)") subprocess.run(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 186 - 206, The generate_cdb function currently only verifies CMake; add a check for Ninja availability by calling shutil.which("ninja") before invoking cmake and raise a clear RuntimeError if it's missing so users get an immediate, helpful message (update generate_cdb to validate both cmake and ninja and include the tool names in the error text).tests/integration/test_server.py (1)
284-293: Unusedworkspaceparameter.The
workspaceparameter is declared but never used intest_hover_on_unknown_file. While the marker is required for the fixture to work, consider if this test actually needs a workspace or if it could be restructured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_server.py` around lines 284 - 293, The test_hover_on_unknown_file function declares an unused workspace parameter; either remove the workspace parameter from the test signature (keep the `@pytest.mark.workspace`("hello_world") decorator if that marker must remain) or rename the parameter to _workspace to signal it is intentionally unused; update the function signature for test_hover_on_unknown_file accordingly to eliminate the unused-parameter warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/conftest.py`:
- Around line 209-222: The workspace fixture currently calls generate_cdb(path)
whenever a CMakeLists.txt exists, causing redundant CMake runs; modify the logic
in the workspace fixture to first check for an existing compile_commands.json
(e.g. path / "build" / "compile_commands.json") and only call generate_cdb(path)
if that file is missing or stale, keeping the existing checks around (path /
"CMakeLists.txt"). Ensure you reference the workspace fixture and the
generate_cdb function when implementing the presence check so repeated tests
sharing the same workspace skip unnecessary regeneration.
- Around line 186-206: The generate_cdb function currently only verifies CMake;
add a check for Ninja availability by calling shutil.which("ninja") before
invoking cmake and raise a clear RuntimeError if it's missing so users get an
immediate, helpful message (update generate_cdb to validate both cmake and ninja
and include the tool names in the error text).
In `@tests/integration/test_server.py`:
- Around line 284-293: The test_hover_on_unknown_file function declares an
unused workspace parameter; either remove the workspace parameter from the test
signature (keep the `@pytest.mark.workspace`("hello_world") decorator if that
marker must remain) or rename the parameter to _workspace to signal it is
intentionally unused; update the function signature for
test_hover_on_unknown_file accordingly to eliminate the unused-parameter
warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc135eae-6df9-40be-83c3-2b24ffee27d7
📒 Files selected for processing (5)
tests/conftest.pytests/integration/test_file_operation.pytests/integration/test_lifecycle.pytests/integration/test_modules.pytests/integration/test_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_modules.py
…remove pyproject.toml - Switch to pytest-asyncio auto mode with @pytest.fixture instead of @pytest_asyncio.fixture, enabling Pylance type inference for fixtures - Remove pyproject.toml in favor of pytest.ini (managed by pixi.toml) - Remove redundant section separators and docstrings that duplicate test names, keep meaningful ones for module-related tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
16bit-ykiko
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/conftest.py (1)
225-244: Add return type annotation to theclientfixture.The async generator fixture is missing a return type annotation. Adding it improves IDE support and documentation.
Suggested fix
`@pytest.fixture` async def client( request: pytest.FixtureRequest, executable: Path, workspace: Path | None -): +) -> AsyncGenerator[CliceClient, None]: """Spawn clice server, auto-initialize if `@pytest.mark.workspace` is present."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 225 - 244, The async pytest fixture function client is missing a return type annotation; update the fixture signature for client to include an appropriate return type (e.g., -> AsyncGenerator[CliceClient, None] or -> CliceClient depending on whether it yields) to improve IDE typing and docs, and ensure the annotation matches how client is used (if using yield-style fixture use AsyncGenerator with proper typing, otherwise annotate as CliceClient); reference the client fixture and the CliceClient class and keep the start_io and initialize calls unchanged.tests/integration/test_modules.py (1)
7-7: Consider using relative import for consistency.Using
from tests.conftest import generate_cdbworks butfrom conftest import generate_cdb(relative) is more conventional for imports within the same test package and matches how pytest resolves conftest.Suggested change
-from tests.conftest import generate_cdb +from conftest import generate_cdb🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_modules.py` at line 7, Replace the absolute import in the test with a relative import for consistency: change the import that brings in generate_cdb (currently written as from tests.conftest import generate_cdb) to use the package-local conftest module (from conftest import generate_cdb) so pytest's usual conftest resolution is used and imports are consistent across tests; update the import statement where generate_cdb is referenced in test_modules.py accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/conftest.py`:
- Around line 225-244: The async pytest fixture function client is missing a
return type annotation; update the fixture signature for client to include an
appropriate return type (e.g., -> AsyncGenerator[CliceClient, None] or ->
CliceClient depending on whether it yields) to improve IDE typing and docs, and
ensure the annotation matches how client is used (if using yield-style fixture
use AsyncGenerator with proper typing, otherwise annotate as CliceClient);
reference the client fixture and the CliceClient class and keep the start_io and
initialize calls unchanged.
In `@tests/integration/test_modules.py`:
- Line 7: Replace the absolute import in the test with a relative import for
consistency: change the import that brings in generate_cdb (currently written as
from tests.conftest import generate_cdb) to use the package-local conftest
module (from conftest import generate_cdb) so pytest's usual conftest resolution
is used and imports are consistent across tests; update the import statement
where generate_cdb is referenced in test_modules.py accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2105941f-1ecf-426c-96ff-ed6b9e70f51d
📒 Files selected for processing (7)
tests/conftest.pytests/integration/test_file_operation.pytests/integration/test_lifecycle.pytests/integration/test_modules.pytests/integration/test_server.pytests/pyproject.tomltests/pytest.ini
💤 Files with no reviewable changes (1)
- tests/pyproject.toml
✅ Files skipped from review due to trivial changes (1)
- tests/pytest.ini
- Disable CMAKE_CXX_SCAN_FOR_MODULES to avoid "compiler does not support scanning" error on macOS - Prefer clang++ as CXX compiler when available (pixi environment) - Show cmake stderr on failure for easier debugging Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UNDEFINED_SYMBOL is on line 5 (0-indexed line 4), not line 3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/conftest.py (2)
217-222: Consider validating marker arguments for a clearer error message.If a test uses
@pytest.mark.workspace()without an argument,marker.args[0]will raise an unhelpfulIndexError. A guard would provide a clearer failure message for test developers.💡 Proposed validation
marker = request.node.get_closest_marker("workspace") if marker is None: return None + if not marker.args: + raise ValueError("@pytest.mark.workspace requires a subdirectory argument") path = test_data_dir / marker.args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 217 - 222, The workspace marker handling should validate that the marker includes an argument before accessing marker.args[0]; update the block that calls request.node.get_closest_marker("workspace") to check marker is not None and that len(marker.args) > 0, and if not raise a clear error (e.g., pytest.UsageError or ValueError) explaining that `@pytest.mark.workspace` requires a path argument; keep the existing behavior of constructing path = test_data_dir / marker.args[0] and calling generate_cdb(path) when a CMakeLists.txt exists.
226-229: Consider adding return type annotation for the async fixture.The fixture is well-designed—auto-initializing when
@pytest.mark.workspaceis present while allowing manual initialization for custom scenarios (liketest_save_recompile). Adding a return type would improve IDE support.💡 Proposed type annotation
`@pytest.fixture` async def client( request: pytest.FixtureRequest, executable: Path, workspace: Path | None -): +) -> AsyncGenerator[CliceClient, None]: """Spawn clice server, auto-initialize if `@pytest.mark.workspace` is present."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 226 - 229, The async pytest fixture named client should include an explicit return type to improve IDE/type-checker support; add a return annotation such as -> AsyncGenerator[ClientType, None] (or the concrete client class used in your tests) on the client fixture, import AsyncGenerator (from typing or collections.abc depending on Python version) and the concrete ClientType, and ensure the annotation matches how the fixture yields/returns the client instance in client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/conftest.py`:
- Around line 217-222: The workspace marker handling should validate that the
marker includes an argument before accessing marker.args[0]; update the block
that calls request.node.get_closest_marker("workspace") to check marker is not
None and that len(marker.args) > 0, and if not raise a clear error (e.g.,
pytest.UsageError or ValueError) explaining that `@pytest.mark.workspace` requires
a path argument; keep the existing behavior of constructing path = test_data_dir
/ marker.args[0] and calling generate_cdb(path) when a CMakeLists.txt exists.
- Around line 226-229: The async pytest fixture named client should include an
explicit return type to improve IDE/type-checker support; add a return
annotation such as -> AsyncGenerator[ClientType, None] (or the concrete client
class used in your tests) on the client fixture, import AsyncGenerator (from
typing or collections.abc depending on Python version) and the concrete
ClientType, and ensure the annotation matches how the fixture yields/returns the
client instance in client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3379d5c0-cd91-446a-9233-2d76c178e72d
📒 Files selected for processing (1)
tests/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_modules.py`:
- Around line 263-267: Replace the flaky fixed sleep by waiting for a
deterministic server response: remove the await asyncio.sleep(5.0) after
client.open(workspace / "cycle_a.cppm") and instead wait for a concrete signal
such as calling client.open_and_wait(workspace / "cycle_a.cppm") or polling
client.diagnostics for the corresponding URI with a bounded timeout; ensure you
use the same workspace / "cycle_a.cppm" URI and propagate a clear timeout so the
test fails fast if the server doesn't respond.
- Around line 200-203: The test currently assumes the first diagnostic
(diags[0]) is the error; instead iterate/search through diags to find any
diagnostic whose range.start.line equals the expected line (and optionally
matches the expected severity) and assert that such a diagnostic exists; replace
the direct use of diags[0] and the equality assertion with a check like using
any(...) or next(filter(...), None) over diags and assert the result is not None
(and then assert its properties if needed) so the test no longer depends on
diagnostic ordering (refer to the diags variable and the error_diag usage in
this block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a853c941-262b-4867-9303-a9862ab63e66
📒 Files selected for processing (1)
tests/integration/test_modules.py
The pixi clang++ needs lld as linker but doesn't use it by default. Reuse cmake/toolchain.cmake to get correct compiler and linker setup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 215-221: The code directly indexes marker.args[0] which can raise
IndexError or produce unclear failures; before using marker.args in the fixture
containing request.node.get_closest_marker("workspace"), validate that
marker.args exists, has at least one positional argument, and that the first
argument is a valid string (or path-like), and if not raise a clear test-setup
error (e.g., pytest.fail or raise a ValueError with an explanatory message).
Move the assignment path = test_data_dir / marker.args[0] and the subsequent
CMakeLists check/generate_cdb call to after this validation so you never index
marker.args without checking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fea3687f-4d38-4880-8bdf-f9df36e05375
📒 Files selected for processing (1)
tests/conftest.py
clang-cl does not support CMake's module dependency scanning. Disabling it still generates correct CDB entries on all platforms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Unify all platforms to use clang++/lld. This enables CMake's C++20 module dependency scanning on Windows, which clang-cl did not support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/conftest.py (1)
216-222:⚠️ Potential issue | 🟡 MinorValidate
@pytest.mark.workspacearguments before indexing.Line 219 assumes one positional string argument.
@pytest.mark.workspacewith no args (or wrong type) will throw unclear setup errors. Please guard args and raisepytest.UsageErrorwith a clear message; also validate that the resolved path exists.Suggested hardening
marker = request.node.get_closest_marker("workspace") if marker is None: return None -path = test_data_dir / marker.args[0] +if len(marker.args) != 1 or not isinstance(marker.args[0], str): + raise pytest.UsageError( + '@pytest.mark.workspace requires one string argument, e.g. ' + '@pytest.mark.workspace("modules/hello_world")' + ) +path = (test_data_dir / marker.args[0]).resolve() +if not path.exists(): + raise pytest.UsageError(f"Workspace path does not exist: {path}") if (path / "CMakeLists.txt").exists(): generate_cdb(path) return path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 216 - 222, The workspace marker handling should validate marker arguments and the target path before using them: in the block that calls request.node.get_closest_marker("workspace") check that marker.args exists, has exactly one positional argument, and that the argument is a string; if not, raise pytest.UsageError with a clear message about the expected `@pytest.mark.workspace`("rel/path") usage. After resolving path = test_data_dir / marker.args[0], verify path.exists() (and is a directory if appropriate) and raise pytest.UsageError if the path does not exist; only then call generate_cdb(path) if (path / "CMakeLists.txt").exists().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 122-125: wait_diagnostics can miss diagnostics that arrived just
before calling wait_for_diagnostics because wait_for_diagnostics re-arms state;
modify wait_diagnostics to first check self.diagnostics.get(uri) (or equivalent
current diagnostics storage) and return immediately if diagnostics are already
present for that URI, otherwise call wait_for_diagnostics(uri) and await
event.wait(); ensure you reference the existing symbols wait_diagnostics,
wait_for_diagnostics, self.diagnostics, uri, and event.wait() so the guard check
is added before re-arming/waiting.
---
Duplicate comments:
In `@tests/conftest.py`:
- Around line 216-222: The workspace marker handling should validate marker
arguments and the target path before using them: in the block that calls
request.node.get_closest_marker("workspace") check that marker.args exists, has
exactly one positional argument, and that the argument is a string; if not,
raise pytest.UsageError with a clear message about the expected
`@pytest.mark.workspace`("rel/path") usage. After resolving path = test_data_dir /
marker.args[0], verify path.exists() (and is a directory if appropriate) and
raise pytest.UsageError if the path does not exist; only then call
generate_cdb(path) if (path / "CMakeLists.txt").exists().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfd25d0f-aa87-4e49-84ad-365be1d0355f
📒 Files selected for processing (1)
tests/conftest.py
- Guard wait_diagnostics against already-arrived diagnostics - Validate @pytest.mark.workspace arguments - Use any() for diagnostic assertion to avoid ordering dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch Windows toolchain from clang-cl to clang++ (GNU driver) with --target=x86_64-pc-windows-msvc. This enables CMake C++20 module scanning support on Windows (clang-cl not supported until CMake 4.4). Simplify ASan setup: clang++ handles -fsanitize=address linking automatically on all platforms, removing the need for manual clang_rt.asan_dynamic lib specification on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep the manual ASan runtime lib linking for clang-cl (MSVC driver), use -fsanitize=address linker flag for clang++/gcc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmake/toolchain.cmake (2)
7-8: Hardcoded x86_64 target limits to 64-bit Windows builds.The
--target=x86_64-pc-windows-msvcflag hardcodes the architecture. If ARM64 Windows support is needed in the future, this would need parameterization. If 64-bit only is intentional, this is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/toolchain.cmake` around lines 7 - 8, The flags set for C and C++ are hardcoded to "--target=x86_64-pc-windows-msvc" which forces 64-bit Windows only; change the set(CMAKE_C_FLAGS ...) and set(CMAKE_CXX_FLAGS ...) usage to use a configurable variable (e.g., TARGET_TRIPLE or WINDOWS_TARGET_TRIPLE) or derive the triple from CMake variables (like CMAKE_SYSTEM_PROCESSOR or an option) and default to the current value, then use that variable in the set() calls and expose it as a CACHE STRING so ARM64 (or other) targets can be selected without editing the file.
9-11: Linker flag inconsistency with CMakeLists.txt.The toolchain sets
-fuse-ld=lldfor Windows, butCMakeLists.txt(lines 85-88) adds-fuse-ld=lld-linkviatarget_link_optionsfor the WIN32 case. When targeting the MSVC ABI (--target=x86_64-pc-windows-msvc),lld-linkis the appropriate linker driver.While the last
-fuse-ld=flag typically wins, having both is inconsistent and potentially confusing. Consider using-fuse-ld=lld-linkhere for clarity, or removing these from the toolchain and relying solely onCMakeLists.txt.Suggested change for consistency
- set(CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "Executable linker flags") - set(CMAKE_SHARED_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "Shared library linker flags") - set(CMAKE_MODULE_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "Module linker flags") + set(CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld-link" CACHE STRING "Executable linker flags") + set(CMAKE_SHARED_LINKER_FLAGS "-fuse-ld=lld-link" CACHE STRING "Shared library linker flags") + set(CMAKE_MODULE_LINKER_FLAGS "-fuse-ld=lld-link" CACHE STRING "Module linker flags")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/toolchain.cmake` around lines 9 - 11, Toolchain sets -fuse-ld=lld which conflicts with CMakeLists.txt adding -fuse-ld=lld-link for WIN32; update consistency by using lld-link for MSVC targets or remove duplication. Modify the three linker flag settings in toolchain.cmake (the set(CMAKE_EXE_LINKER_FLAGS ...), set(CMAKE_SHARED_LINKER_FLAGS ...), set(CMAKE_MODULE_LINKER_FLAGS ...)) to use "-fuse-ld=lld-link" when targeting the MSVC ABI (--target=x86_64-pc-windows-msvc) or alternatively remove these sets and rely on the WIN32-specific target_link_options in CMakeLists.txt that adds "-fuse-ld=lld-link"; ensure the chosen approach keeps CMakeLists.txt's target_link_options(WIN32 ... "-fuse-ld=lld-link") behavior unchanged.tests/conftest.py (1)
190-209: Consider validating workspace path stays within test data directory.While the workspace path originates from test markers (controlled by developers), there's no validation that the resolved path stays within
test_data_dir. A marker like@pytest.mark.workspace("../../etc")would resolve outside the expected directory.The static analysis hint (S603) about subprocess is a false positive here since
shell=Falseis used and the path comes from test fixtures, not external input.Optional path containment check
path = test_data_dir / marker.args[0] + try: + path = path.resolve() + path.relative_to(test_data_dir.resolve()) + except ValueError: + raise pytest.UsageError( + f"Workspace path must be within test data directory: {path}" + ) if (path / "CMakeLists.txt").exists(): generate_cdb(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 190 - 209, The generate_cdb function should validate that the provided workspace path stays inside the repository test data directory before invoking subprocess: resolve workspace (workspace_resolved = workspace.resolve()) and resolve the test data root (e.g. test_data_dir = Path(__file__).resolve().parent / "test_data" or the existing test_data_dir fixture/constant), then check containment using workspace_resolved.is_relative_to(test_data_dir.resolve()) (or compare parts for older Python) and raise a RuntimeError if not contained; perform this check at the start of generate_cdb (before calling cmake/subprocess) so you reject markers like "@pytest.mark.workspace('../../etc')" early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 55-58: The else() branch appends ASan to CMAKE_EXE_LINKER_FLAGS
and CMAKE_SHARED_LINKER_FLAGS but omits CMAKE_MODULE_LINKER_FLAGS; update the
else() block to also append " -fsanitize=address" to CMAKE_MODULE_LINKER_FLAGS
so module libraries receive the same ASan linker flag as executables and shared
libs (mirror the clang-cl branch's handling of CMAKE_MODULE_LINKER_FLAGS).
---
Nitpick comments:
In `@cmake/toolchain.cmake`:
- Around line 7-8: The flags set for C and C++ are hardcoded to
"--target=x86_64-pc-windows-msvc" which forces 64-bit Windows only; change the
set(CMAKE_C_FLAGS ...) and set(CMAKE_CXX_FLAGS ...) usage to use a configurable
variable (e.g., TARGET_TRIPLE or WINDOWS_TARGET_TRIPLE) or derive the triple
from CMake variables (like CMAKE_SYSTEM_PROCESSOR or an option) and default to
the current value, then use that variable in the set() calls and expose it as a
CACHE STRING so ARM64 (or other) targets can be selected without editing the
file.
- Around line 9-11: Toolchain sets -fuse-ld=lld which conflicts with
CMakeLists.txt adding -fuse-ld=lld-link for WIN32; update consistency by using
lld-link for MSVC targets or remove duplication. Modify the three linker flag
settings in toolchain.cmake (the set(CMAKE_EXE_LINKER_FLAGS ...),
set(CMAKE_SHARED_LINKER_FLAGS ...), set(CMAKE_MODULE_LINKER_FLAGS ...)) to use
"-fuse-ld=lld-link" when targeting the MSVC ABI
(--target=x86_64-pc-windows-msvc) or alternatively remove these sets and rely on
the WIN32-specific target_link_options in CMakeLists.txt that adds
"-fuse-ld=lld-link"; ensure the chosen approach keeps CMakeLists.txt's
target_link_options(WIN32 ... "-fuse-ld=lld-link") behavior unchanged.
In `@tests/conftest.py`:
- Around line 190-209: The generate_cdb function should validate that the
provided workspace path stays inside the repository test data directory before
invoking subprocess: resolve workspace (workspace_resolved =
workspace.resolve()) and resolve the test data root (e.g. test_data_dir =
Path(__file__).resolve().parent / "test_data" or the existing test_data_dir
fixture/constant), then check containment using
workspace_resolved.is_relative_to(test_data_dir.resolve()) (or compare parts for
older Python) and raise a RuntimeError if not contained; perform this check at
the start of generate_cdb (before calling cmake/subprocess) so you reject
markers like "@pytest.mark.workspace('../../etc')" early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fa6fb40-b529-47de-9914-6b6c0041002a
📒 Files selected for processing (5)
CMakeLists.txtcmake/toolchain.cmakescripts/activate_asan.battests/conftest.pytests/integration/test_modules.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_modules.py
…ndows - Toolchain: remove if/else duplication, Windows only needs extra CMAKE_MSVC_RUNTIME_LIBRARY (pixi clang already targets MSVC implicitly) - ASan: use CMAKE_CXX_COMPILER_FRONTEND_VARIANT to distinguish clang-cl (MSVC frontend, needs manual ASan lib linking) from clang++ (GNU frontend, -fsanitize=address handles linking automatically) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lld-link enables Identical COMDAT Folding by default, which merges string literals like "}" and L"}" causing ASan to report ODR violations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FILE_SET CXX_MODULES) in all 26tests/data/modules/*/directories. CDB is generated on-the-fly viacmake -G Ninjaduring test setup.@pytest.mark.workspace()decorator: Introduce a marker + fixture pattern so tests declare their workspace via decorator and receive a resolvedworkspacepath. The fixture auto-generates CDB when a CMakeLists.txt is present.CliceClienthelper methods: Addinitialize(),open(),wait_diagnostics(), andopen_and_wait()to reduce boilerplate across all test files.asyncio_mode = "auto": Switch from@pytest_asyncio.fixture+@pytest.mark.asyncioto@pytest.fixture+ auto mode for proper Pylance type inference on fixtures.tests/pyproject.toml(config moved topytest.ini)..cppmtoformat-cppglob pattern.CMAKE_CXX_SCAN_FOR_MODULESand prefer pixi clang++ to fix macOS CI where CMake rejects module scanning.Test plan
Summary by CodeRabbit