Repository navigation
refactor: tests and format the world - #314
Conversation
WalkthroughReplaces the legacy test runner and DSL with a new Runner2-based test runner and TEST_SUITE/TEST_CASE macro framework, adds cpptrace as a dependency, migrates ~80+ unit tests to the new macros, removes several old test utilities, and applies various small header and formatting fixes. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~100–120 minutes Areas needing focused review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
xmake.lua (1)
44-44: Consider pinning the cpptrace version.While cpptrace is successfully added, the CMake configuration pins it to v1.0.4 whereas this xmake configuration doesn't specify a version. For consistency and reproducibility across build systems, consider pinning the version here as well.
Apply this diff to pin the version:
-add_requires(libuv_require, "toml++", "croaring", "flatbuffers", "cpptrace") +add_requires(libuv_require, "toml++", "croaring", "flatbuffers", "cpptrace 1.0.4")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txt(1 hunks)cmake/package.cmake(2 hunks)xmake.lua(2 hunks)
⏰ 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). (8)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, releasedbg)
🔇 Additional comments (4)
cmake/package.cmake (2)
120-120: LGTM!The setting follows the existing pattern for Roaring configuration. While tangential to the cpptrace addition, disabling microbenchmarks is a reasonable test configuration choice.
132-139: cpptrace v1.0.4 verified as valid and secure.The version tag exists and has no known security advisories. The integration is correct.
CMakeLists.txt (1)
152-152: LGTM!The private linkage of cpptrace to unit_tests is correct and consistent with the FetchContent declaration in cmake/package.cmake.
xmake.lua (1)
148-148: LGTM!The cpptrace package is correctly added to the unit_tests target, consistent with the CMake configuration.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/unit/Test/Test.cpp (1)
18-18: Remove empty comment.Line 18 contains an empty
///comment that appears to be leftover from development.TEST_CASE(World, skip_test) { x = 3; - /// return TestState::Skipped; };tests/unit/Test/Runner.cpp (1)
34-34: Iterate by reference to avoid copying.The loop iterates
suitesby value, creating unnecessary copies ofTestSuiteobjects.- for(auto suite: suites) { + for(auto& suite: suites) {include/Test/Runner.h (1)
25-25: Memberfocusshadows the struct typefocus.The member name
focus(line 25) shadows the marker typestruct focus(line 18). Consider renaming one for clarity, e.g.,is_focusedorhas_focus.struct TestAttrs { - bool skiped = false; - bool focus = false; + bool skipped = false; + bool is_focused = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/unit_tests.cc(2 hunks)include/Test/Runner.h(1 hunks)include/Test/Test2.h(1 hunks)tests/unit/Test/Runner.cpp(1 hunks)tests/unit/Test/Test.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
bin/unit_tests.cc (1)
tests/unit/Test/Runner.cpp (2)
instance(7-10)instance(7-7)
include/Test/Test2.h (4)
include/Test/Runner.h (2)
clice(7-39)TestState(9-38)bin/unit_tests.cc (2)
instance(50-53)instance(50-50)tests/unit/Test/Runner.cpp (2)
instance(7-10)instance(7-7)tests/unit/Test/Test.cpp (3)
TEST_SUITE(5-26)TEST_CASE(16-20)TEST_CASE(22-25)
tests/unit/Test/Runner.cpp (2)
bin/unit_tests.cc (6)
instance(50-53)instance(50-50)add_suite(55-57)add_suite(55-55)run_tests(131-179)run_tests(131-131)include/Test/Test2.h (1)
suites(28-36)
⏰ 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). (8)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
🔇 Additional comments (5)
bin/unit_tests.cc (2)
2-2: LGTM on the include addition.The include for
Test/Test2.his appropriate for accessing the new test framework.
202-202: I'll help you verify whether the oldRunnerclass is still needed. Let me search for any references to it outside ofbin/unit_tests.cc.
<function_calls>
#!/bin/bashCheck if the old Runner class is still used anywhere outside bin/unit_tests.cc
echo "=== Searching for Runner::instance() calls ==="
rg -n --type=cpp '\bRunner::instance()' --glob '!bin/unit_tests.cc'echo ""
echo "=== Searching for Runner class references (excluding bin/unit_tests.cc) ==="
rg -n --type=cpp '\bRunner\b' --glob '!bin/unit_tests.cc' -C2 | head -100
</function_calls>tests/unit/Test/Test.cpp (1)
5-26: LGTM as framework demonstration code.This test suite effectively demonstrates the new test framework's capabilities: setup/teardown hooks, skip_test and focus attributes, and TestState returns. Note that the instance variable
xis recreated for each test (per therun_testlambda in Test2.h), so the setup/teardown modifications won't persist across tests—which is typically the desired behavior.include/Test/Test2.h (2)
40-53: LGTM on the run_test lambda design.The use of
requiresconstraints for optionalsetup()andteardown()hooks is elegant C++20 usage. Converting to a function pointer via+[]ensures a stateless callable that fits the expected signature.
62-67: I'll help you verify the test case registration mechanism in the code. Let me search for where_register_functions are called and understand how test cases are being registered.
<function_calls>
#!/bin/bashCheck if _register functions are called anywhere
rg -n 'register' --type=cpp --type=h -C 3
Also check for TEST_CASE and TEST_SUITE patterns
echo "=== TEST_CASE/TEST_SUITE usage ==="
rg -n 'TEST_CASE|TEST_SUITE' --type=cpp --type=h -A 5 | head -100
</function_calls>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
xmake.lua (1)
45-51: Scope of newcpptracedependency inadd_requiresAdding
cpptraceto the top‑leveladd_requiresmakes it a project‑wide requirement, even though it appears to be used only by tests. If you want to keep non‑test builds as lean as possible, consider gating this require behind your test/dev config (e.g.,enable_test) so that release builds that explicitly disable tests don’t have to resolve cpptrace at all.Example refactor (optional):
-add_requires(libuv_require, "toml++", "croaring", "flatbuffers", "cpptrace") +add_requires(libuv_require, "toml++", "croaring", "flatbuffers") +if has_config("enable_test") or has_config("dev") then + add_requires("cpptrace") +endThis keeps behavior the same for normal dev/CI flows while relaxing the dependency for builds where tests are turned off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cmake.yml(1 hunks)xmake.lua(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/cmake.yml
⏰ 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). (8)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
🔇 Additional comments (1)
xmake.lua (1)
149-163: Linkingunit_testsagainstcpptracelooks correct; verify package/toolchain compatibilityWiring
cpptraceonly into theunit_teststarget viaadd_packages("cpptrace")is consistent with keeping it as a test‑only link dependency and matches the CMake side conceptually.Please just double‑check that:
- The
cpptracexmake package you pull in is available on all platforms you run tests on (Windows/Linux/macOS), and- It links cleanly under your
clice_build_configrules (no‑exceptions, no‑RTTI,lld,-static-libstdc++/-static-libc++), i.e., no hidden assumptions in cpptrace about exceptions/RTTI that would conflict here.If everything builds on your CI matrix, this change looks good.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
tests/unit/Test/Runner.cpp (1)
65-68: Typo:skipedshould beskipped.This matches the typo in
Runner.hline 18. Both should be corrected for consistency.include/Test/Runner.h (1)
17-20: Typo:skipedshould beskipped.This affects both the struct member name and all references to it.
include/Test/Test2.h (1)
45-47:std::moveon static vector is destructive.Calling
suites()moves from the statictest_cases()vector, leaving it empty. Ifsuites()is called more than once, subsequent calls will return an empty vector.
🧹 Nitpick comments (6)
include/Test/TExpr.h (1)
40-70: Explicit deduction guide and abbreviated forward declaration look soundThe added deduction guide
template <typename LHS, typename RHS> name(const LHS&, const RHS&) -> name<LHS, RHS>;is consistent with the
name<LHS, RHS>template and itsconst LHS&/const RHS&members, so CTAD for these predicates should behave as expected (for both brace- and paren-initialization). The forward declaration and out-of-namespace definition ofname##_impl(auto&& lhs, auto&& rhs)also match correctly under C++20 abbreviated templates, so there’s no apparent ODR or linkage issue.One minor, optional thought: since
nameis an aggregate and already eligible for implicit CTAD, this explicit guide is largely redundant unless you’re intentionally constraining or documenting the deduction. If you don’t need that, you could omit the guide to keep the macro’s expansion slightly leaner.If you want extra safety, you might quickly verify on all your supported compilers that:
eq{lhs, rhs}andeq(lhs, rhs)both compile and deduce the sameLHS/RHSas before,- no additional CTAD ambiguities are introduced when combining these predicates with temporaries and mixed types.
.clang-format (1)
96-105: Formatting rules align with new test macros; verify clang-format versionConfiguring
WrapNamespaceBodyWithEmptyLines: Alwaysand treatingTEST_SUITEas aNamespaceMacrosentry makes sense for the new test framework and will keep namespaces and test suites consistently spaced. Please just confirm your CI/developer environments are actually running a clang-format version (e.g., 18+) that understands these options.include/Compiler/Diagnostic.h (1)
11-15: Redundant forward declaration ofclang::DiagnosticConsumer
clang/Basic/Diagnostic.halready provides the full definition ofclang::DiagnosticConsumer, so the explicit forward declaration here is redundant (though not incorrect). You can drop it for a slightly cleaner header if you like.tests/unit/Test/Runner.cpp (2)
42-47: Iterating by value causes unnecessary copy ofTestSuite.The
suitesvector is iterated by value, copying eachTestSuitestruct. Use a const reference to avoid the copy.- for(auto suite: suites) { + for(const auto& suite: suites) {
87-87:curr_test_durationis accumulated but never used.This variable is incremented each iteration but only
total_test_durationis used in output. If per-suite duration reporting isn't needed, consider removingcurr_test_duration.include/Test/Test2.h (1)
8-8: Unused include:llvm/ADT/StringMap.h.
StringMapis not used in this file.-#include "llvm/ADT/StringMap.h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.clang-format(2 hunks)bin/unit_tests.cc(2 hunks)docs/semantic-tokens-example.cpp(1 hunks)include/Compiler/Compilation.h(1 hunks)include/Compiler/Diagnostic.h(1 hunks)include/Compiler/Tidy.h(1 hunks)include/Index/IncludeGraph.h(1 hunks)include/Test/Platform.h(1 hunks)include/Test/Runner.h(1 hunks)include/Test/TExpr.h(2 hunks)include/Test/Test.h(1 hunks)include/Test/Test2.h(1 hunks)src/AST/Resolver.cpp(1 hunks)src/Index/MergedIndex.cpp(1 hunks)src/Index/USRGeneration.cpp(2 hunks)src/Support/Doxygen.cpp(2 hunks)tests/unit/AST/Resolver.cpp(3 hunks)tests/unit/Test/Runner.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Support/Doxygen.cpp
- include/Test/Test.h
- include/Test/Platform.h
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/unit_tests.cc
🧰 Additional context used
🧬 Code graph analysis (5)
include/Index/IncludeGraph.h (2)
src/Compiler/CompilationUnit.cpp (1)
CompilationUnit(7-18)include/Compiler/CompilationUnit.h (1)
CompilationUnit(15-204)
tests/unit/AST/Resolver.cpp (1)
include/AST/FilterASTVisitor.h (1)
TraverseDecl(27-45)
include/Test/Test2.h (2)
include/Test/Runner.h (2)
clice(8-45)TestState(10-45)include/Test/Test.h (1)
clice(14-82)
tests/unit/Test/Runner.cpp (1)
include/Test/Test2.h (1)
suites(45-53)
include/Test/Runner.h (2)
include/Test/Test.h (1)
clice(14-82)include/Test/Test2.h (2)
clice(13-75)suites(45-53)
⏰ 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). (8)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
🔇 Additional comments (10)
src/Index/USRGeneration.cpp (1)
53-145: MakingUSRGeneratorinternal via the anonymous namespace looks correctEncapsulating
USRGeneratorin the anonymous namespace tightens linkage and avoids symbol collisions while keeping all existing out-of-class definitions and uses in this TU valid. No functional issues spotted with this refactor.docs/semantic-tokens-example.cpp (1)
61-62: Namespace closure fix looks correctThe added
} // namespace foocleanly balances thenamespace foo {above and confines the symbols to the intended namespace with no other behavioral changes.src/Index/MergedIndex.cpp (1)
70-71: Correctly closing thellvmnamespaceThe added closing brace ensures the
DenseMapInfospecializations are scoped tollvmonly and preventsclice::indexsymbols from accidentally ending up in thellvmnamespace.include/Compiler/Compilation.h (1)
8-12: Forward declaration ofclang::CodeCompleteConsumeris appropriateUsing a forward declaration here is fine since this header only takes a
clang::CodeCompleteConsumer*and doesn’t require the complete type; the implementation ofcompletejust needs to include the proper Clang header.include/Compiler/Tidy.h (1)
7-11: Forward declaration ofclang::CompilerInstanceis safeThis header only needs
clang::CompilerInstanceby reference inconfigure, so the forward declaration is sufficient and helps keep dependencies lighter; the implementation can include the full Clang headers.include/Index/IncludeGraph.h (1)
7-11: Good use of forward declaration forCompilationUnitDeclaring
class CompilationUnit;innamespace cliceis correct and sufficient forIncludeGraph::from(CompilationUnit&), reducing header coupling; the implementation offromjust needs to includeCompilationUnit.h.tests/unit/AST/Resolver.cpp (1)
18-25: DiscardingTraverseDeclreturn value prevents early termination.The return value from
Base::TraverseDecl(decl)is now discarded. According to theRecursiveASTVisitorpattern (as seen inFilterASTVisitor.h), returningfalsesignals early termination of the traversal. With the current implementation, if a nestedVisit*method returnsfalse, the traversal will continue regardless.If early termination is intentional to be disabled, this is fine. Otherwise, consider:
bool TraverseDecl(clang::Decl* decl) { if(decl && (llvm::isa<clang::TranslationUnitDecl>(decl) || unit.file_id(decl->getLocation()) == unit.interested_file())) { - Base::TraverseDecl(decl); + return Base::TraverseDecl(decl); } return true; }tests/unit/Test/Runner.cpp (1)
96-101: Test counts include filtered/skipped tests, which may be misleading.The summary reports
total_tests_countwhich was calculated before filtering. This means the output shows the total registered tests, not the number actually run. Consider trackingrun_tests_countseparately for accurate reporting.include/Test/Runner.h (2)
17-20:focusattribute is defined but never used.The
TestAttrs::focusfield is declared butRunner2::run_tests()never checks it. Either implement focus mode (run only focused tests when any test has focus) or remove the field to avoid dead code.
1-43: LGTM on overall structure.The header provides a clean, minimal interface for the test runner with appropriate type definitions for
TestState,TestAttrs,TestCase,TestSuite, andRunner2.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
include/Test/Test2.h (4)
45-47:std::moveon static vector is destructive.This issue was previously flagged. Calling
suites()moves from the statictest_cases()vector, leaving it empty. If called more than once, subsequent calls return an empty vector.constexpr inline static auto suites() { - return std::move(test_cases()); + return test_cases(); // Return copy instead of destructive move }
77-84:print_traceframe filtering logic appears inverted.This issue was previously flagged. The logic finds the first frame where
filename != location.file_name()and erases from that iterator to end, which removes frames after the test file rather than framework frames.If the goal is to trim leading framework frames (before the test file):
- frames.erase(it, frames.end()); + frames.erase(frames.begin(), it);
88-93:_register_##name()function is never called.This issue was previously flagged. The function defined by TEST_CASE is never invoked, so tests are not registered. The registration should happen via static initialization.
Consider changing the macro to trigger registration at static init:
#define TEST_CASE(name, ...) \ - void _register_##name() { \ - (void)_register_suites<>; \ - (void)_register_test_case<#name, &Self::test_##name __VA_OPT__(, ) __VA_ARGS__>; \ - } \ + inline static bool _registered_##name = \ + ((void)_register_suites<>, (void)_register_test_case<#name, &Self::test_##name __VA_OPT__(, ) __VA_ARGS__>, true); \ void test_##name()
95-100:EXPECT_TRUElogic is inverted.This issue was previously flagged. The macro triggers failure when
expris true, but should fail when false.#define EXPECT_TRUE(expr) \ - if(expr) { \ + if(!(expr)) { \ auto trace = cpptrace::generate_trace(); \ print_trace(trace, std::source_location::current()); \ failure(); \ }tests/unit/Test/Runner.cpp (2)
65-68: Typo: "skiped" should be "skipped".This was previously flagged. The typo originates from
Runner.hand propagates here.
78-78:TestState::Fatalis not handled.This was previously flagged. If a test returns
TestState::Fatal, it will be treated as passed.- bool curr_failed = state == TestState::Failed; + bool curr_failed = state == TestState::Failed || state == TestState::Fatal;
🧹 Nitpick comments (9)
tests/unit/AST/Selection.cpp (2)
4-4: IncludeTest/Test2.hinstead ofTest/Test.hfor consistency.This file uses
TEST_SUITEandTEST_CASEmacros which are defined inTest/Test2.h. The current include ofTest/Test.hmay work ifTest.hincludesTest2.h, but for clarity and consistency with other migrated test files (likeLock.cpp), consider using the direct include.-#include "Test/Test.h" +#include "Test/Test2.h"
635-652: Empty test cases should be marked as skipped.These test cases (Metrics, Selected, PathologicalPreprocessor) contain only FIXME comments and no assertions. They will pass silently. Consider using the test framework's skip functionality to mark them appropriately.
Would you like me to help mark these as skipped tests using the
TestAttrs{.skiped = true}pattern?include/Test/Test2.h (1)
130-146: Consider removing#ifndefguards for macro consistency.The
#ifndef ASSERT_NEand#ifndef EXPECT_NEguards suggest potential conflicts with external test frameworks. If this framework is meant to be self-contained, consider either removing these guards or documenting why they're needed.tests/unit/Test/Runner.cpp (1)
51-63: Redundant and potentially conflicting filter logic.There are two filter checks: one for suite name prefix (lines 52-57) and one for glob pattern matching (lines 61-62). The first check uses simple string comparison which doesn't work correctly with glob patterns containing wildcards. Consider unifying the filter logic.
for(auto& [suite_name, test_cases]: all_suites) { - if(!filter.empty()) { - auto pos = filter.find_first_of('.'); - if(pos != std::string::npos && filter.substr(0, pos) != suite_name) { - continue; - } - } - for(auto& [test_name, test, attrs]: test_cases) { std::string display_name = std::format("{}.{}", suite_name, test_name); if(pattern && !pattern->match(display_name)) { continue; }tests/unit/AST/SourceCode.cpp (1)
69-89: Test case has no assertions.The LexInclude test only verifies the lexer doesn't crash when processing include directives and module declarations. Consider adding assertions to validate the expected tokens, or mark it as skipped if it's work-in-progress.
Would you like me to help add assertions for the expected token sequence (e.g., verifying the
#includedirectives andmodule;declaration are properly lexed)?tests/unit/Async/ThreadPool.cpp (1)
6-37: Thread identity assertions may be brittle if the pool has fewer than 3 workersThe test assumes all three submitted tasks run on distinct threads (
ASSERT_NEon all pairs). That’s only guaranteed if the underlying thread pool always provisions at least 3 workers and never reuses threads in a way that would cause two tasks to run on the same worker.If the implementation or configuration ever changes (e.g., pool size becomes 1–2, or a constrained environment), this test will start failing even though the pool is still correct.
Consider relaxing the assertion to:
- Only verify that work runs off the calling thread (e.g.,
ASSERT_NE(id1, std::this_thread::get_id())), or- Assert a minimum number of distinct worker IDs using a
std::unordered_setand a threshold derived from the documented pool size.tests/unit/Async/FileSystem.cpp (1)
7-43: Test behavior is correct; consider aligning the test header and cleanup semanticsFunctionally the tests look good:
FileRead:
- Creates a temp file, writes
"hello"synchronously, thenco_await async::fs::read(*path)and validates both success and content.FileWrite:
- Writes
"hello"viaasync::fs::writeand validates via a synchronousfs::readafterasync::run(main()).Two minor points to consider:
Header consistency
Other Async tests are including"Test/Test2.h"directly, while this file still includes"Test/Test.h"but usesTEST_SUITE,TEST_CASE, andCO_ASSERT_*. IfTest.his now just a legacy shim over Test2, you’re fine, but it might be clearer to includeTest/Test2.hhere as well for consistency and to avoid surprises ifTest.his trimmed later.Temporary file handling (optional)
Iffs::createTemporaryFiledoes not auto‑cleanup (e.g., via RAII), you might eventually accumulate files on disk. If that’s a concern, consider either a scoped RAII helper that unlinks on destruction or an explicit cleanup path after assertions.tests/unit/Async/Gather.cpp (1)
9-65: Gather tests look correct; just be aware of implicit single-threaded and ordering assumptionsThese tests exercise three important behaviors:
- GatherPack: three tasks sharing a counter
x, asserting that the returned values are{1, 2, 3}.- GatherRange:
async::gather(args, task_gen)withresults.push_back(x)and a final check thatargs == resultsandcore.result() == true.- GatherCancel: same pattern but expecting an early stop (
results.size() == 1andcore.result() == false).All of this looks consistent with a single‑threaded event-loop style implementation of
async::Task/gather.One thing to keep in mind: these tests assume that:
- All
task_genexecutions run on the same thread (soresults.push_backis not contended), andgatherpreserves argument order when populating results.If
gatheris ever changed to run tasks on a thread pool or in a different scheduling order, these tests could become racy or order‑sensitive. If that evolution is on the roadmap, consider adding explicit synchronization (e.g., pushing into per‑task slots) or relaxing the ordering assertions.tests/unit/Async/Event.cpp (1)
1-1: Align event tests with the new Test2 header (optional)This file still includes
"Test/Test.h"while usingTEST_SUITE,TEST_CASE, andEXPECT_EQ, whereas other Async tests have been migrated to include"Test/Test2.h"directly.If
Test.his now just a compatibility wrapper, behavior is fine, but for consistency with the rest of the Async test suite—and to avoid surprises ifTest.his slimmed down later—you may want to switch this include toTest/Test2.h.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
include/Test/Test2.h(1 hunks)tests/unit/AST/Selection.cpp(4 hunks)tests/unit/AST/SourceCode.cpp(1 hunks)tests/unit/Async/Event.cpp(1 hunks)tests/unit/Async/FileSystem.cpp(1 hunks)tests/unit/Async/Gather.cpp(1 hunks)tests/unit/Async/Lock.cpp(1 hunks)tests/unit/Async/Sleep.cpp(1 hunks)tests/unit/Async/Task.cpp(1 hunks)tests/unit/Async/ThreadPool.cpp(1 hunks)tests/unit/Test/Runner.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/unit/Async/Task.cpp (1)
include/Async/Gather.h (1)
task(23-34)
tests/unit/Async/Lock.cpp (1)
include/Async/Task.h (2)
cancel(64-70)cancel(291-293)
tests/unit/AST/SourceCode.cpp (1)
src/Compiler/Preamble.cpp (1)
lexer(20-20)
tests/unit/Async/FileSystem.cpp (1)
src/Async/FileSystem.cpp (8)
write(204-209)write(204-204)write(214-225)write(214-214)read(146-151)read(146-146)read(156-193)read(156-156)
tests/unit/AST/Selection.cpp (1)
include/Test/Tester.h (1)
StringRef(12-28)
include/Test/Test2.h (2)
include/Test/Runner.h (2)
clice(8-45)TestState(10-45)include/Test/Test.h (1)
clice(14-82)
⏰ 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). (8)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
🔇 Additional comments (11)
tests/unit/AST/Selection.cpp (2)
220-235: LGTM!The
select_righthelper function correctly handles single-point and two-point selection scenarios, properly forwards the callback, and uses appropriate assertions.
237-262: LGTM!The
expect_selectfunction properly handles both null and non-null expected node cases, with appropriate range validation.tests/unit/Async/Lock.cpp (3)
1-6: LGTM!Correct include of
Test/Test2.hand proper TEST_SUITE structure for async tests.
8-43: LGTM!The Lock test properly verifies that the async lock serializes access to the shared variable
xacross three concurrent tasks.
62-66: Based on my verification of the codebase, I can now provide the rewritten review comment:
The
cancel()+dispose()pattern is correct and standard; no changes needed.The pattern of calling
task2.cancel()followed bytask2.dispose()is the documented and idiomatic way to clean up a cancelled task. Thedispose()method marks the coroutine handle for destruction when the task finishes or is cancelled (not immediately), and then clears the core reference to prevent double-deletion. This sequence is used consistently throughout the codebase including in production code (e.g.,src/Server/Document.cpp).tests/unit/Test/Runner.cpp (1)
8-11: LGTM!Correct singleton implementation for Runner2.
tests/unit/AST/SourceCode.cpp (2)
1-8: LGTM!Correct includes and TEST_SUITE structure for the SourceCode tests.
10-67: LGTM!The IgnoreComments test case properly verifies both comment-ignoring and comment-retaining lexer modes with appropriate assertions.
tests/unit/Async/Sleep.cpp (1)
6-20: Sleep test logic and coroutine lifetime look soundThe test correctly captures
xby reference into the coroutine, runsasync::run(task)to completion, and then asserts the post‑sleep state. There are no obvious races or lifetime issues given the single test case scope.tests/unit/Async/Task.cpp (2)
9-123: Async task lifecycle tests are well coveredOverall the test set does a good job of exercising
async::Taskbehavior:
- Run / TaskSchedule validate that
async::run()with and without scheduled tasks completes successfully and delivers results.- TaskDispose checks destruction on:
- Disposing a scheduled task that never completes.
- Disposing a cancelled task from within another coroutine.
- TaskCancel validates that cancellation prevents post‑sleep code from running.
- TaskCancelRecursively verifies that cancelling a top‑level task propagates through nested tasks and prevents completion of inner coroutines.
Assuming the underlying
async::Tasksemantics (initial suspend, cancellation, and dispose) match these expectations, the tests look logically consistent and should give good coverage of the lifecycle edge cases.
83-92: The project is configured for C++23 and<print>is already includedThe codebase requires C++23 (set in CMakeLists.txt:
CMAKE_CXX_STANDARD 23withCMAKE_CXX_STANDARD_REQUIRED ON). The<print>header is transitively available through the includedTest/Test2.hheader, which explicitly includes<print>. The test code compiles without issue.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/Compiler/Toolchain.cpp (1)
2-2: Fix inconsistent test framework include.This file still includes
Test/Test.hinstead ofTest/Test2.h, which is inconsistent with all other migrated test files and will cause the tests to use the wrong framework.Apply this fix:
-#include "Test/Test.h" +#include "Test/Test2.h"
♻️ Duplicate comments (1)
tests/unit/Test/Runner.cpp (1)
70-93: Handle TestState::Fatal in failure detection.Line 78 only checks for
TestState::Failed, so tests returningTestState::Fatalwill be treated as passed. Fatal errors should also be counted as failures.Apply this fix:
- bool curr_failed = state == TestState::Failed; + bool curr_failed = (state == TestState::Failed || state == TestState::Fatal);Additionally, consider whether fatal errors should terminate the test run early or just be counted as failures. The current behavior continues running all tests even after a fatal error.
🧹 Nitpick comments (6)
tests/unit/Compiler/Tidy.cpp (2)
10-18: Clarify intent when asserting onstd::optional<bool>
tidy::is_fast_tidy_check(...)returnsstd::optional<bool>, soASSERT_TRUE(tidy::is_fast_tidy_check("..."))only asserts that the optional is engaged, not that the contained value istrue. If the map entry were accidentally markedfalse, these tests would still pass. If you actually want to assert “this check exists and is marked fast”, consider asserting on the value explicitly, e.g. via.value()/.value_or(false)or by comparing againststd::optional{true}.
20-28: Consider tightening the Tidy test input and expectationsThe remapped source
"int main() { return 0 }"is not valid C++ (missing semicolon), soASSERT_FALSE(unit->diagnostics().empty())may be satisfied purely by parse errors rather than clang‑tidy diagnostics. If the goal is specifically to exercise clang‑tidy, you might want to (a) use syntactically valid code and/or (b) assert on the presence or kind of tidy diagnostics instead of only non‑emptiness.tests/unit/Compiler/Module.cpp (1)
69-101: Clean up or re-enable commented test code.A large block of test code for global module fragments and module partitions is commented out. The FIXME on line 69 suggests this is blocked on fixing the standard library search path.
Consider either:
- Removing the commented code if these scenarios are no longer relevant
- Creating a tracked issue for the FIXME and keeping the commented code as reference
- Re-enabling the tests if the resource directory issue has been resolved
Would you like me to help create an issue to track this work?
tests/unit/Compiler/Command.cpp (2)
198-200: Address the Module test case placeholder.This test case is marked as TODO and empty. Either implement the test or remove it if module command handling tests are covered elsewhere.
Would you like me to help implement this test or open an issue to track it?
238-319: Re-enable or remove commented compilation database tests.Two substantial test cases for loading absolute and relative Unix-style paths from compilation databases are commented out. These appear to be complete, working tests.
Consider:
- Re-enabling these tests if they're still relevant
- Adding skip conditions if they need platform-specific handling
- Removing them if compilation database loading is tested elsewhere
tests/unit/Compiler/Directive.cpp (1)
10-16: Consider using test fixtures instead of global mutable state.The global variables (tester, includes, has_includes, conditions, macros, pragmas) create coupling between test cases and potential order dependencies. While this works for a direct migration, consider refactoring to use test fixtures or making these local to each test case.
Example pattern:
TEST_CASE(Include) { Tester tester; // ... local state and execution }This would make tests more independent and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
include/Test/Runner.h(1 hunks)tests/unit/Compiler/Command.cpp(2 hunks)tests/unit/Compiler/Compiler.cpp(3 hunks)tests/unit/Compiler/Diagnostic.cpp(2 hunks)tests/unit/Compiler/Directive.cpp(5 hunks)tests/unit/Compiler/Module.cpp(2 hunks)tests/unit/Compiler/Preamble.cpp(5 hunks)tests/unit/Compiler/Tidy.cpp(1 hunks)tests/unit/Compiler/Toolchain.cpp(1 hunks)tests/unit/Test/Runner.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
tests/unit/Compiler/Tidy.cpp (1)
src/Compiler/Tidy.cpp (2)
is_fast_tidy_check(54-66)is_fast_tidy_check(54-54)
tests/unit/Test/Runner.cpp (3)
bin/unit_tests.cc (6)
instance(50-53)instance(50-50)add_suite(55-57)add_suite(55-55)run_tests(131-179)run_tests(131-131)include/Test/Test2.h (1)
suites(45-53)include/Test/Test.h (1)
string(84-98)
tests/unit/Compiler/Toolchain.cpp (2)
src/Compiler/Toolchain.cpp (4)
driver_family(238-284)driver_family(238-238)query_toolchain(286-373)query_toolchain(286-286)include/Compiler/Toolchain.h (1)
CompilerFamily(9-49)
tests/unit/Compiler/Directive.cpp (1)
src/Feature/FoldingRange.cpp (2)
pragmas(306-320)pragmas(306-306)
tests/unit/Compiler/Command.cpp (1)
include/Support/ObjectPool.h (1)
remove(165-175)
include/Test/Runner.h (1)
include/Test/Test2.h (2)
clice(13-75)suites(45-53)
tests/unit/Compiler/Preamble.cpp (2)
src/Compiler/Preamble.cpp (4)
compute_preamble_bounds(17-58)compute_preamble_bounds(17-17)compute_preamble_bound(8-15)compute_preamble_bound(8-8)src/Compiler/Compilation.cpp (8)
compile(359-364)compile(359-359)compile(366-393)compile(366-366)compile(395-415)compile(395-395)preprocess(355-357)preprocess(355-355)
⏰ 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). (8)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
🔇 Additional comments (18)
tests/unit/Compiler/Tidy.cpp (1)
1-1: Header switch to Test2 looks consistent with the new frameworkIncluding
Test/Test2.hin place of the old test header aligns with the PR’s migration to the new framework; as long asASSERT_*andTEST_*macros are all defined there, this change is fine.tests/unit/Compiler/Preamble.cpp (5)
1-11: LGTM!The includes are appropriate for the test file, and the use of an anonymous namespace for test internals is good practice.
13-23: LGTM!The
expect_boundshelper cleanly validates preamble bounds against annotated source offsets.
25-87: LGTM!The
expect_build_pchhelper properly tests both PCH generation and its subsequent use with the AST build.Minor nit: The trailing semicolon on line 87 is syntactically valid but unconventional for a function definition (likely a remnant from the lambda conversion).
89-114: LGTM!Good coverage of preamble bound scenarios including empty content, single directives, nested preprocessor blocks, and module declarations.
116-207: LGTM!Good test coverage for different compilation scenarios: basic translation units, C++20 modules, and complex header inclusion chains.
tests/unit/Compiler/Module.cpp (1)
1-1: LGTM: Test framework header updated.The migration from
Test/Test.htoTest/Test2.haligns with the broader test framework refactoring.tests/unit/Compiler/Command.cpp (2)
15-33: LGTM: Test-specific print_argv implementation.This implementation differs from the production
print_argvininclude/Compiler/Command.cppby providing more detailed escaping and quoting, which is appropriate for test output readability.
45-196: LGTM: Command tests properly migrated.The test cases for option ID parsing, default filters, argument reuse, and remove/append operations are correctly migrated to the new framework with appropriate ASSERT_* macros.
tests/unit/Compiler/Compiler.cpp (2)
13-37: LGTM: TopLevelDecls test correctly migrated.The test properly verifies that the compiler identifies the expected number of top-level declarations using the new assertion style.
39-65: LGTM: StopCompilation threading test correctly implemented.The test properly verifies that compilation can be stopped via a shared atomic flag by running compilation in a separate thread, signaling stop after a delay, and asserting the result is false.
tests/unit/Compiler/Diagnostic.cpp (1)
81-134: LGTM: Diagnostic tests correctly migrated.All four test cases (CommandError, Error, PCHError, ASTError) are properly migrated with correct assertions that verify the expected diagnostic behavior.
tests/unit/Compiler/Toolchain.cpp (1)
20-117: LGTM: Toolchain tests correctly implemented.The Family, GCC, and Clang test cases are properly migrated with appropriate skip conditions for CI environments. The placeholder tests for MSVC and Zig have clear TODO comments.
tests/unit/Compiler/Directive.cpp (1)
78-198: LGTM: Directive test cases correctly migrated.All five test cases (Include, HasInclude, Condition, Macro, Pragma) properly verify the expected preprocessor directive behavior with correct assertions.
include/Test/Runner.h (1)
10-43: LGTM: Test runner infrastructure well-defined.The Runner2 class and supporting types (TestState, TestAttrs, TestCase, TestSuite) provide a clean API for test registration and execution. The design supports test filtering, skipping, and focus modes.
tests/unit/Test/Runner.cpp (3)
17-49: LGTM: Test runner initialization and filtering.The implementation properly builds the suite list, creates an optional glob pattern for filtering, and prepares for test execution.
51-68: LGTM: Suite and test filtering logic.The nested loop correctly filters suites and test cases based on the filter string and glob pattern, and properly handles skipped tests with appropriate output.
96-104: LGTM: Test summary output.The final summary correctly reports test counts, suite counts, and total duration, and returns a non-zero exit code if any tests failed.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/Feature/Hover.cpp (2)
36-68: Test case has no active assertions.
TEST_CASE(Namespace)callsrun()but allEXPECT_HOVERcalls are commented out. The test passes trivially without verifying any behavior.
135-160: Unused local variables in test cases.Variables like
M_TEXT,FREE_STYLE, andFUNC_STYLEare defined but never used in assertions. These appear to be expected output strings for future hover tests.Consider either:
- Removing these until the assertions are implemented
- Adding
[[maybe_unused]]to suppress warnings- Converting to actual assertions
Also applies to: 179-198, 228-251, 265-282
tests/unit/Feature/DocumentSymbol.cpp (1)
155-170: Complete or document the disabled assertion.The Macro test case calls
run()but has no active assertion—Line 169 shows a commented-out expectation. Either:
- Migrate it to
ASSERT_EQ(total_size_wrapper(symbols), 3U);, or- Add a comment explaining why this assertion is disabled.
Without an assertion, this test provides no verification.
🧹 Nitpick comments (20)
tests/unit/Feature/InlayHint.cpp (7)
15-15: Unusedstd::source_locationparameter.The
locationparameter is captured but never used inrun(),expect_size(), andexpect_hint(). If this is intended for test framework integration to report failure locations, ensure it's wired into the assertion macros (e.g., passed toASSERT_*). Otherwise, remove the parameter.
23-26: Duplicate offsets silently overwritten.If two hints share the same offset, the second overwrites the first in
hints_map, potentially masking test failures. Consider adding an assertion or logging when duplicates are detected.hints_map.clear(); for(auto& hint: hints) { + assert(!hints_map.count(hint.offset) && "Duplicate hint offset detected"); hints_map[hint.offset] = hint; }
54-57: Minor: Grammar and style fixes.
- Line 54: "we has" → "we have"
- Line 57: Unnecessary trailing semicolon after the function's closing brace.
auto& parts = it->second.parts; - /// Currently, we has only one label. + /// Currently, we have only one label. ASSERT_EQ(parts.size(), 1U); ASSERT_EQ(parts[0].name, name); -}; +}
364-377: Missingexpect_size()assertion.The test for function call operators (starting at line 334) runs assertions on individual hints but doesn't verify the total hint count with
expect_size(). This could miss detecting unexpected extra hints. Consider addingexpect_size(12);before the individual hint assertions.} )c"); + expect_size(12); expect_hint("0", "x:"); expect_hint("1", "x:");
401-406: Missingexpect_size()assertion for "Deducing this" test.Similar to the function call operator test, this test case verifies individual hints without first asserting the expected total count.
)c"); + expect_size(4); expect_hint("0", "Param:"); expect_hint("1", "Param:");
568-568: Unnecessary trailing semicolons after TEST_CASE blocks.Lines 568, 810, 891 have trailing semicolons after the closing braces of TEST_CASE blocks. These are not required and are inconsistent with other TEST_CASE blocks (e.g., lines 1491, 1524).
1463-1472: Consider addingexpect_size()and tracking FIXMEs.The commented-out assertions (lines 1464-1466, 1487-1490) indicate known gaps in functionality. Consider:
- Adding
expect_size()to catch unexpected hints- Tracking these FIXMEs in an issue so they don't get lost
tests/unit/Feature/SignatureHelp.cpp (2)
12-19: Guardnameless_points()[0]with an explicit assertion
tester.nameless_points()[0]assumes there is at least one marker in the code. Adding an assertion both documents this contract and gives a clearer failure mode if a future test forgets to add$:void run(llvm::StringRef code) { tester.clear(); tester.add_main("main.cpp", code); tester.prepare(); - tester.params.completion = {"main.cpp", tester.nameless_points()[0]}; + const auto& points = tester.nameless_points(); + ASSERT_FALSE(points.empty()); + tester.params.completion = {"main.cpp", points[0]};This keeps behavior the same when the precondition holds but makes tests easier to debug if it’s violated.
9-36: Consider avoiding shared mutable state at suite scope for future parallelism
Tester tester;andproto::SignatureHelp help;are shared across all cases in the suite.run()currently clears/reassigns them so it’s safe for sequential execution, but if Test2 ever runs cases in parallel within a process, this shared state could become a source of flaky behavior.Not urgent, but you might later consider a fixture or local-per-test setup (constructing
Testerandhelpinsiderun()or eachTEST_CASE) if parallelism becomes a goal.tests/unit/Feature/DocumentLink.cpp (1)
17-17: Trailing semicolons after function bodies are unnecessary.Lines 17, 29 have semicolons after the closing brace of function definitions. This is syntactically valid but unconventional in C++.
-}; +}Also applies to: 22-22, 29-29, 42-42
tests/unit/Feature/SemanticToken.cpp (2)
24-42: Consider early return with failure message for better diagnostics.The
expect_tokenfunction works correctly, but when a token is not found, the failure message fromASSERT_TRUE(found)won't indicate which position failed. Thestd::source_locationparameter is captured but not used in the failure output.
22-22: Trailing semicolons after function bodies.Same as in DocumentLink.cpp - the semicolons after the function closing braces are unconventional.
Also applies to: 42-42
tests/unit/Feature/CodeCompletion.cpp (1)
48-90: Multiple test cases are incomplete placeholders.
Unqualified,Functor, andLambdatest cases contain only TODO/EXPECT comments without actual assertions. Consider either:
- Adding a
GTEST_SKIP()or similar mechanism to mark them as pending- Converting them to tracked issues for future implementation
- Adding basic assertions even if the full functionality isn't implemented yet
Would you like me to open issues to track the implementation of these incomplete test cases?
tests/unit/Feature/Hover.cpp (1)
284-361: Large blocks of commented-out code.Lines 284-361 contain extensive commented-out test code. If these tests are planned for future implementation, consider:
- Removing them and tracking in an issue
- Converting to
SKIPor placeholder tests with clear TODOsCommented-out code adds maintenance burden and reduces readability.
tests/unit/Feature/DocumentSymbol.cpp (2)
10-11: Consider encapsulating test state within each TEST_CASE.While the current approach clears
testerinrun(), shared mutable state can introduce subtle test interdependencies. Encapsulatingtesterandsymbolswithin each test case would improve isolation.
21-32: Consider simplifying the recursive helper.The nested lambda pattern works but could be replaced with a simpler recursive free function or a direct recursive lambda assigned to
std::function.Also, the trailing semicolon on Line 32 after the closing brace is unnecessary (though valid).
Example simplification:
-auto total_size_wrapper(const std::vector<feature::DocumentSymbol>& result) -> size_t { - size_t size = 0; - std::function<void(const std::vector<feature::DocumentSymbol>&, size_t&)> total_size = - [&total_size](const std::vector<feature::DocumentSymbol>& result, size_t& size) { - for(auto& item: result) { - ++size; - total_size(item.children, size); - } - }; - total_size(result, size); - return size; -}; +size_t total_size_wrapper(const std::vector<feature::DocumentSymbol>& result) { + size_t size = result.size(); + for(const auto& item: result) { + size += total_size_wrapper(item.children); + } + return size; +}tests/unit/Feature/FoldingRange.cpp (4)
7-32: Guardexpect_foldingagainst out‑of‑range indices and address unused paramsThe shared
run/expect_foldinghelpers look good and centralize all FoldingRange setup, but two small points:
expect_foldingdoesauto& folding = ranges[index];without a prior bounds check. In a regression where fewer ranges are produced, this could become UB instead of a clean test failure. Consider adding something like:void expect_folding(std::uint32_t index, llvm::StringRef begin, llvm::StringRef end, feature::FoldingRangeKind kind, std::source_location loc = std::source_location::current()) { - auto& folding = ranges[index]; + ASSERT_TRUE(index < ranges.size()); + auto& folding = ranges[index];
kindandlocare currently only referenced in commented‑out code, so they’re effectively unused. Either mark them[[maybe_unused]]for now, or (preferable) wire them up to the new assertion style oncefolding.kindis implemented, e.g. by replacing the oldexpect(...)call with anASSERT_EQ(folding.kind.value(), kind.value());-style check.These don’t block the migration but will make the helper more robust and keep warnings down.
280-301: CompoundStmt and Directive tests currently only check compilation, not folding behavior
TEST_CASE(CompoundStmt)andTEST_CASE(Directive)only callrun(...)and don’t inspectrangesat all. That might be intentional (pure “doesn’t crash / compiles with PCH” regression tests), but given the presence of$(N)markers inCompoundStmtand the file’s overall style, it’s possible earlier tests asserted on the resulting folding ranges.Can you confirm whether the legacy tests for these cases also skipped range assertions? If they previously validated specific folds, it would be worth reintroducing explicit
ASSERT_EQ(ranges.size(), ...)andexpect_folding(...)calls here to avoid losing coverage in the migration.Also applies to: 374-388
303-322: InitializeList and AccessSpecifier tests mostly complete; optionally assert size in AccessSpecifier
InitializeListis aligned with the rest of the file: it runs a small snippet, checksranges.size() == 2, and validates both initializer lists withexpect_folding. Looks good.
AccessSpecifierdoes a nice job covering multiple access-specifier layouts and macro-expanded cases, but it never assertsranges.size(). Since you rely on indices 0–11 inexpect_folding, adding an explicit size check would make the test intention clearer and avoid silent acceptance if additional, unexpected folding ranges start appearing:TEST_CASE(AccessSpecifier) { run(R"cpp( ... )cpp"); + ASSERT_EQ(ranges.size(), 12U); expect_folding(0, "1", "2", Class); ...Optional, but it improves test clarity and removes reliance on implicit bounds.
Also applies to: 324-372
390-414: PragmaRegion test: keep plan to re-enable assertions with new helpersThe
PragmaRegiontest correctly preserves the existing FIXME and has the old expectations commented out due to the PCH issue. Once pragma-region folding becomes reliable again, it’d be good to:
- Switch those commented
expect(...)lines to the sharedASSERT_EQ(ranges.size(), ...)+expect_folding(...)pattern, matching the rest of this file.- Potentially add a quick
ASSERT_EQ(ranges.size(), 3U);before individual folds, mirroring other tests.Nothing to change now, but this clarifies the future migration step for this test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/unit/Feature/CodeCompletion.cpp(3 hunks)tests/unit/Feature/DocumentLink.cpp(3 hunks)tests/unit/Feature/DocumentSymbol.cpp(8 hunks)tests/unit/Feature/FoldingRange.cpp(13 hunks)tests/unit/Feature/Formatting.cpp(1 hunks)tests/unit/Feature/Hover.cpp(12 hunks)tests/unit/Feature/InlayHint.cpp(39 hunks)tests/unit/Feature/SemanticToken.cpp(6 hunks)tests/unit/Feature/SignatureHelp.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/unit/Feature/DocumentSymbol.cpp (2)
src/Feature/DocumentSymbol.cpp (2)
document_symbols(129-136)document_symbols(129-129)tests/unit/Feature/DocumentLink.cpp (2)
run(12-17)run(12-12)
tests/unit/Feature/FoldingRange.cpp (1)
src/Feature/FoldingRange.cpp (2)
folding_ranges(329-331)folding_ranges(329-329)
tests/unit/Feature/Formatting.cpp (1)
src/Feature/Formatting.cpp (2)
document_format(35-58)document_format(35-37)
tests/unit/Feature/InlayHint.cpp (1)
src/Feature/InlayHint.cpp (10)
range(441-453)range(441-446)range(455-476)range(455-460)range(478-503)range(478-478)range(505-512)range(505-505)inlay_hints(882-896)inlay_hints(882-884)
tests/unit/Feature/CodeCompletion.cpp (2)
src/Feature/CodeCompletion.cpp (2)
code_complete(427-438)code_complete(427-428)include/Feature/CodeCompletion.h (1)
CompletionItemKind(37-104)
tests/unit/Feature/SignatureHelp.cpp (3)
src/Feature/SignatureHelp.cpp (2)
signature_help(190-208)signature_help(190-191)tests/unit/Feature/DocumentLink.cpp (2)
run(12-17)run(12-12)tests/unit/Feature/FoldingRange.cpp (2)
run(12-17)run(12-12)
⏰ 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). (8)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, debug)
🔇 Additional comments (20)
tests/unit/Feature/InlayHint.cpp (1)
9-57: Good test infrastructure setup.The helper functions (
run,expect_size,expect_hint) provide a clean DSL for testing inlay hints. The migration to the TEST_SUITE/TEST_CASE framework improves consistency with the rest of the codebase.tests/unit/Feature/SignatureHelp.cpp (1)
7-40: Migration toTEST_SUITE/TEST_CASEandrun()helper looks solidThe new suite structure, shared
tester/helpobjects, and therun()helper align with the patterns in other Feature tests (e.g., DocumentLink, FoldingRange) and keep the test body minimal and readable. I don’t see functional issues with the refactor.tests/unit/Feature/DocumentLink.cpp (1)
31-59: LGTM!The test cases are well-structured with clear setup, execution, and assertions. The migration to the new TEST_CASE macro style is consistent.
Also applies to: 61-78
tests/unit/Feature/SemanticToken.cpp (1)
44-191: LGTM!The semantic token test cases comprehensively cover various C++ constructs (includes, comments, keywords, macros, declarations). The migration to TEST_CASE macros is clean and consistent.
tests/unit/Feature/CodeCompletion.cpp (1)
24-32: LGTM!The
Scoretest case properly verifies completion results with meaningful assertions.tests/unit/Feature/Hover.cpp (1)
362-405: LGTM!
TEST_CASE(AutoAndDecltype)andTEST_CASE(Expr)have the structure in place and callrun()with appropriate test input. The FIXME comments provide useful context about known limitations.tests/unit/Feature/DocumentSymbol.cpp (10)
1-1: LGTM: New test framework include.The inclusion of
Test/Test2.haligns with the PR-wide migration to the new test framework.
13-19: LGTM: Test setup function.The
runfunction correctly sets up the test environment with proper assertions for preconditions.
34-57: LGTM: Namespace test case.The test structure is correct and follows the expected pattern.
59-72: LGTM: Struct test case.Test structure and expected symbol count appear correct.
74-91: LGTM: Field test case.Test structure and expected symbol count (7: struct x, fields x1/x2/x3, struct y, fields y1/y2) appear correct.
93-106: LGTM: Constructor test case.Test structure and expected symbol count (6: struct S, field x, default/parameterized/copy constructors, destructor) appear correct.
108-123: LGTM: Method test case.Test structure and expected symbol count (7: struct _0 plus 6 overloaded method declarations) appear correct.
125-143: LGTM: Enum test case.Test structure and expected symbol count (8: enum A with 3 enumerators, enum B with 3 enumerators) appear correct.
145-153: LGTM: TopLevelVariable test case.Test structure and expected symbol count (2: variables x and y) appear correct.
172-172: LGTM: TEST_SUITE closure.The suite is properly closed with a descriptive comment.
tests/unit/Feature/Formatting.cpp (2)
1-1: LGTM! Clean migration to Test2 framework.The test has been successfully migrated to the new Test2 framework with proper use of
TEST_SUITEandTEST_CASEmacros, and the structure is well-organized.Also applies to: 6-13
9-10: Based on my verification, I can now provide the rewritten review comment.The test is reliable and will consistently produce formatting edits.
The input
"int main() { return 0; }"lacks a trailing newline character, and the project's.clang-formatconfiguration specifiesInsertNewlineAtEOF: true(line 23). This means the formatter will always produce at least one edit—inserting the required newline at the end of the file. The test assertion will consistently pass.tests/unit/Feature/FoldingRange.cpp (2)
36-88: Namespace / Enum tests look like faithful migrationsThe
NamespaceandEnumtest cases userun(...), assertranges.size()explicitly, and then verify each folding span viaexpect_foldingwith the expectedKindvalues. The mapping of$(N)markers to indices and kinds is consistent with the code snippets, so the behavior under test is clear and well covered.
90-127: Record / Method / Lambda / Function / FunctionCall tests are well-structuredThese suites all follow the same good pattern:
- Call
run(...)on a focused snippet.- Assert the exact
ranges.size().- Use
expect_foldingfor each expected span with the appropriateKind(e.g.,Struct,Union,FunctionBody,LambdaCapture,FunctionParams,FunctionCall).The use of
$(N)markers is consistent, and the expected counts (6 for Record, 4 for Method, 7 for Lambda, 7 for Function, 3 for FunctionCall) match the annotated code. I don’t see any functional issues in these migrated tests.Also applies to: 129-159, 161-206, 208-252, 254-278
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/Index/USR.cpp (1)
329-350: Minor inconsistency: missing semicolon afterTEST_CASE(USRDeducingThis).All other
TEST_CASEblocks end with};but this one ends with just}. For consistency with the rest of the file, consider adding the semicolon.ASSERT_NE(usr1, usr2); -} +}; }; // TEST_SUITE(USR)tests/unit/Index/MergedIndex.cpp (2)
15-22: Unusedlocationparameter.The
locationparameter is declared but never used in the function body. Either remove it or use it for better error reporting in assertions.-void build_index(llvm::StringRef code, - std::source_location location = std::source_location::current()) { +void build_index(llvm::StringRef code) { tester.clear(); tester.add_main("main.cpp", code); ASSERT_TRUE(tester.compile()); tu_index = index::TUIndex::build(*tester.unit); -}; +}Also, the trailing semicolon after the function definition is unconventional (though valid).
24-46: Unused variables:locationanderr.Both the
locationparameter and theerrvariable are declared but never used. Additionally,errcontains typos ("offser" → "offset", and duplicate "range:").Consider either removing these unused variables or incorporating them into assertions for better diagnostics.
void expect_select(llvm::StringRef pos, llvm::StringRef expect_range, - llvm::StringRef file = "", - std::source_location location = std::source_location::current()) { + llvm::StringRef file = "") { auto offset = tester.point(pos, file); auto range = tester.range(expect_range, file); auto fid = file.empty() ? tester.unit->interested_file() : tester.unit->file_id(file); auto& index = tu_index.file_indices[fid]; auto it = std::ranges::lower_bound(index.occurrences, offset, {}, [](index::Occurrence& occurrence) { return occurrence.range.end; }); - auto err = - std::format("Fail to find symbol for offser: {} range: range: {}", offset, dump(range)); - ASSERT_TRUE(it != index.occurrences.end()); /// FIXME: Make eq pretty print reflectable struct. ASSERT_EQ(dump(it->range), dump(range)); }tests/unit/Index/TUIndex.cpp (2)
13-20: Unusedlocationparameter.Same as in MergedIndex.cpp, the
locationparameter is declared but never used. Consider removing it or using it for enhanced assertion diagnostics.
46-86: Unused variables in helper functions.In both
expect_selectandgo_to_definition:
offsetis computed but only used in commented-out error messageslocationparameter is unusedThe commented-out
std::formatlines suggest these were intended for better error messages. Consider either:
- Removing the unused variables if diagnostics won't be added soon
- Integrating them into assertion macros if they support custom messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/unit/Index/MergedIndex.cpp(3 hunks)tests/unit/Index/TUIndex.cpp(5 hunks)tests/unit/Index/USR.cpp(7 hunks)
⏰ 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). (8)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
🔇 Additional comments (6)
tests/unit/Index/USR.cpp (3)
1-6: LGTM on includes and setup.The new
Test/Test2.hinclude is correctly added alongside the existing includes.
73-103: LGTM on disabled test documentation.The FIXME comment clearly explains why the test is commented out (missing headers for
<concepts>).
104-327: LGTM on TEST_CASE implementations.All test cases correctly follow the pattern of creating a
USRTester, running compilation, looking up USRs, and asserting the expected relationships. The assertions appropriately useASSERT_EQfor expected equality andASSERT_NEfor expected inequality.tests/unit/Index/MergedIndex.cpp (1)
48-74: LGTM on serialization test.The test correctly verifies round-trip serialization by building an index, merging file indices, serializing each merged index, deserializing it, and asserting equality.
tests/unit/Index/TUIndex.cpp (2)
22-44: LGTM onselecthelper.The function correctly performs a lower-bound search on occurrences and collects all that contain the given offset.
88-242: LGTM on test case implementations.The test cases provide comprehensive coverage for:
- Basic index operations
- Class templates (primary, partial specialization, full specialization, explicit/implicit instantiation)
- Function templates
- Alias templates
- Variable templates
- Concepts
The commented-out test calls are properly documented with FIXME notes explaining the underlying issues (e.g., clang limitations on explicit function instantiation location info).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/unit/Support/GlobPattern.cpp (1)
9-16: Consider using framework assertions inPATDEFmacro.The
PATDEFmacro usesassert()which doesn't integrate with the new test framework's assertion reporting and may not provide useful diagnostics on failure. Consider using the framework'sASSERT_TRUEmacro instead for consistency.#define PATDEF(NAME, PAT) \ const char* PatString_##NAME = PAT; \ auto Res##NAME = clice::GlobPattern::create(PatString_##NAME, 100); \ - if(!Res##NAME.has_value()) { \ - std::cout << Res##NAME.error() << '\n'; \ - } \ - assert(Res##NAME.has_value()); \ + ASSERT_TRUE(Res##NAME.has_value()); \ auto NAME = Res##NAME.value();This would also allow removing the
#include <iostream>at line 1.tests/unit/Support/Binary.cpp (1)
21-22: Signed/unsigned comparison in loop.
auto i = 0deduces toint, whileproxy.size()returns an unsigned type (likelysize_t). This causes a signed/unsigned comparison warning and potential issues with large sizes.- for(auto i = 0; i < proxy.size(); i++) { + for(std::size_t i = 0; i < proxy.size(); i++) {The same pattern occurs in multiple loops throughout this file (lines 34, 47, 64, 77, 90, 107).
tests/unit/Support/StructedText.cpp (1)
11-115: Test cases lack assertions.All three test cases (
Paragraph,BulletList,FullText) constructStructedTextobjects but don't validate the output. They only verify that construction doesn't crash. Consider adding assertions to validate the markdown output, e.g.:TEST_CASE(Paragraph) { // ... existing construction code ... auto markdown = st.as_markdown(); ASSERT_TRUE(markdown.find("CodeBlock Example:") != std::string::npos); ASSERT_TRUE(markdown.find("para1") != std::string::npos); }Would you like me to help generate meaningful assertions for these test cases?
tests/unit/Support/JSON.cpp (1)
155-190: Struct serialization tests are correct, minor style note.The test properly validates JSON serialization of simple and nested structs. Both structs correctly use defaulted equality operators for comparison.
Minor style observation: Lines 160 and 176 have an extra space before the opening parenthesis in
operator== ((typically written asoperator==(), but this doesn't affect functionality.tests/unit/Support/Doxygen.cpp (3)
13-60: Tighten assertions inDoxygenInfoand simplify map/tag handlingThe test does a good job covering both param and block command comments, but you could make failures easier to diagnose and the loop slightly clearer:
- Split the combined
ASSERT_TRUE(param_foo.has_value() && ...)/ASSERT_TRUE(param_bar.has_value() && ...)into separateASSERT_TRUE/ASSERT_EQcalls so it’s obvious whether lookup or content comparison failed.- Optionally also assert the expected
ParamDirectionforfoo,bar, andbazto fully exerciseadd_param_command_comment’s third argument.- In the
bcc_listloop, consider capturingtag.str()once into a localstd::stringto avoid recomputing it forcontains,operator[], anderase.For example:
- auto param_foo = di.find_param_info("foo"); - ASSERT_TRUE(param_foo.has_value() && param_foo.value()->content.compare("Doc for foo") == 0); + auto param_foo = di.find_param_info("foo"); + ASSERT_TRUE(param_foo.has_value()); + ASSERT_EQ(param_foo.value()->content, "Doc for foo"); - for(auto& [tag, content]: bcc_list) { + for(auto& [tag, content] : bcc_list) { std::set<std::string> actual; for(auto& block: content) { actual.insert(block.content); } - ASSERT_TRUE(expected.contains(tag.str())); - ASSERT_EQ(actual, expected[tag.str()]); - expected.erase(tag.str()); + std::string tag_str = tag.str(); + ASSERT_TRUE(expected.contains(tag_str)); + ASSERT_EQ(actual, expected[tag_str]); + expected.erase(tag_str); }
62-145: MakeDoxygenParserSimplemore self-checking instead of relying on printed output
DoxygenParserSimplecovers several useful shapes of raw comments, but a couple of blocks (e.g., starting at Line [75] and Line [83]) only callstrip_doxygen_infoandstd::printlnwithout asserting anything aboutdiormd. That turns them into “no-crash/manual-inspection” checks rather than true regression tests.Consider, for each block, asserting at least a minimal invariant that captures what you expect from the parser, for example:
- That
di.get_block_command_comments()is empty or non-empty as appropriate.- That
mdis empty or contains specific substrings (or is at least non-empty).Illustratively:
{ constexpr auto raw_comment = R"( @)"; std::println("Processing raw comment: `{}`", raw_comment); - auto [di, md] = strip_doxygen_info(raw_comment); - std::println("Rest:\n```\n{}\n```\n", md); + auto [di, md] = strip_doxygen_info(raw_comment); + ASSERT_TRUE(di.get_block_command_comments().empty()); + // Or whatever invariant you actually expect here. + std::println("Rest:\n```\n{}\n```\n", md); }This keeps the handy debugging prints while ensuring the test will fail automatically if the parser behavior regresses.
147-323: Optional: strengthen integrated parser checks and consider reducing log noise
DoxygenParserIntegratedexercises a rich, realistic Doxygen block and already validates:
- Param presence and directions for
width,height,foo,bar,baz,awa.- The number of block command comment groups.
- Presence of return info.
Two optional improvements if you want these tests to catch more subtle regressions:
- Add a few targeted assertions on the actual contents of
mdand selected block command items (e.g., that certain substrings or line counts are present in the@details,@warning,@note, and@returnsections), not just their existence.- Across both parser tests, there’s a lot of
std::printlnoutput. If your CI logs become noisy, consider guarding these behind a verbosity flag or only printing on failure while keeping the assertions as the primary signal.These are non-blocking, but would make the integrated tests more robust as the Doxygen parser evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tests/unit/Server/ActiveFileManager.cpp(1 hunks)tests/unit/Support/Binary.cpp(2 hunks)tests/unit/Support/Compare.cpp(2 hunks)tests/unit/Support/Doxygen.cpp(3 hunks)tests/unit/Support/Enum.cpp(2 hunks)tests/unit/Support/GlobPattern.cpp(2 hunks)tests/unit/Support/JSON.cpp(2 hunks)tests/unit/Support/Struct.cpp(2 hunks)tests/unit/Support/StructedText.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unit/Support/Doxygen.cpp (1)
src/Support/Doxygen.cpp (2)
strip_doxygen_info(237-249)strip_doxygen_info(237-237)
tests/unit/Support/Binary.cpp (1)
include/Support/Binary.h (1)
is_directly_binarizable_v(32-41)
tests/unit/Support/Enum.cpp (1)
include/Support/Enum.h (4)
Enum(70-73)Enum(157-157)Enum(161-164)Enum(269-271)
tests/unit/Support/GlobPattern.cpp (1)
src/Support/GlobPattern.cpp (4)
create(162-212)create(162-163)create(214-289)create(215-215)
⏰ 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). (8)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
🔇 Additional comments (19)
tests/unit/Server/ActiveFileManager.cpp (5)
1-5: LGTM! Headers are appropriate for the migrated test framework.The added standard headers (
<format>,<limits>,<optional>,<string>) and the new Test2.h framework header support the migration from the legacy testing framework.
12-14: LGTM! Test suite declaration and type alias enhance clarity.The TEST_SUITE macro properly declares the suite, and the
Managertype alias improves test readability.
16-26: LGTM! Boundary testing is thorough.The test correctly validates default max size, minimum capability constraint (0 → 1), and maximum capability clamping.
28-42: LGTM! LRU eviction logic is correctly validated.The test properly verifies that with capacity 1, adding a second file evicts the first, and the returned references point to different objects.
44-70: LGTM! Iterator ordering validates LRU semantics.The test correctly verifies that iteration proceeds in reverse insertion order (most recent first), which is appropriate for an LRU cache.
tests/unit/Support/Compare.cpp (1)
18-65: LGTM!The migration to the new TEST_SUITE/TEST_CASE framework is clean and correct. All compile-time assertions using
static_assertare preserved, and the test logic forrefl::equalandrefl::lessremains functionally equivalent.tests/unit/Support/GlobPattern.cpp (1)
18-496: LGTM!Comprehensive test coverage for GlobPattern functionality. The migration to TEST_SUITE/TEST_CASE is well-executed with proper use of ASSERT_TRUE/ASSERT_FALSE macros throughout all test cases.
tests/unit/Support/Struct.cpp (1)
17-151: LGTM!The migration preserves all reflection testing logic correctly. The use of
staticfor local struct/union definitions withinTEST_CASEblocks is appropriate for enabling compile-time member name extraction via pointers-to-members. The test coverage forrefl::foreach,inherited_struct, and tuple-like reflection is comprehensive.tests/unit/Support/Binary.cpp (1)
185-211: LGTM!The recursive
Nodestructure test correctly validates deep serialization/deserialization usingrefl::equalfor structural comparison. This approach is appropriate for testing nested structures without requiring explicitoperator==definitions.tests/unit/Support/Enum.cpp (4)
1-32: LGTM! Clean migration to Test2 framework.The header update and enum type definitions are correctly structured. The Color and StringEnum types properly inherit from refl::Enum with appropriate template parameters.
33-55: LGTM! Test suite and enum name validation are correct.The TEST_SUITE macro and EnumName test case properly validate the enum_name functionality for both scoped and unscoped enums.
57-129: LGTM! Normal and mask enum test cases are thorough and correct.Both test cases properly validate enum functionality:
- NormalEnum: Tests construction, comparison, naming, and value access
- MaskEnum: Tests bitmask operations including bitwise OR and AND operations
The use of both compile-time (static_assert) and runtime (ASSERT_EQ/ASSERT_TRUE) assertions is appropriate.
131-145: LGTM! StringEnum test case validates string-based enums correctly.The test properly validates enums with string_view underlying types, checking value access and comparison operations. The suite end marker is correctly placed.
tests/unit/Support/JSON.cpp (5)
1-17: LGTM! Include changes support the test migration.The addition of
<unordered_map>and LLVM ADT headers (SmallString, StringMap, StringSet) properly supports the types tested in the JSON serialization/deserialization test cases. The migration to Test2.h is consistent with the framework update.
48-101: LGTM! String and MapRange test cases properly validate JSON serialization.Both test cases correctly validate:
- String: Tests multiple string type variants (std::string, std::string_view, llvm::StringRef, llvm::SmallString)
- MapRange: Tests map containers (std::map, std::unordered_map, llvm::DenseMap) with proper key-value serialization
The bidirectional serialization/deserialization tests with ASSERT_EQ are appropriate.
103-126: LGTM! SetRange and SequenceRange test cases are correct.Both test cases properly validate collection serialization:
- SetRange: Tests set deserialization (serialization order would be non-deterministic)
- SequenceRange: Tests vector and array types with appropriate serialization/deserialization coverage
The selective testing (e.g., ArrayRef only for serialization) is appropriate given the nature of these types.
128-153: LGTM! Enum test case validates both plain and framework enums.The test correctly validates JSON serialization for:
- Plain enum class (E::B → 1)
- refl::Enum-based Color type (Color::Blue → 2)
Both serialization and deserialization paths are tested with appropriate assertions.
192-192: LGTM! Test suite properly closed.The TEST_SUITE end marker correctly closes the JSON test suite.
tests/unit/Support/Doxygen.cpp (1)
1-11: Test2 migration and suite scaffolding look correctIncluding
Test/Test2.hand wrapping these tests inTEST_SUITE(Doxygen)insideclice::testing+ an anonymous namespace is consistent with the PR’s new framework and keeps test symbols local. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/Support/GlobPattern.cpp (2)
9-16: Integrate PATDEF with the test framework instead of rawassert.
PATDEFusesassert(Res##NAME.has_value()), which is compiled out under NDEBUG, yet still calls.value()right after. In a non-debug build, an unexpected error fromGlobPattern::createwould then go straight into.value()on an invalidexpected, likely causing a hard terminate instead of a clean ASSERT_* failure.Consider rewriting the macro to use the test framework directly, e.g.:
-#define PATDEF(NAME, PAT) \ - const char* PatString_##NAME = PAT; \ - auto Res##NAME = clice::GlobPattern::create(PatString_##NAME, 100); \ - if(!Res##NAME.has_value()) { \ - std::cout << Res##NAME.error() << '\n'; \ - } \ - assert(Res##NAME.has_value()); \ - auto NAME = Res##NAME.value(); +#define PATDEF(NAME, PAT) \ + const char* PatString_##NAME = PAT; \ + auto Res##NAME = clice::GlobPattern::create(PatString_##NAME, 100); \ + if(!Res##NAME.has_value()) { \ + std::cout << Res##NAME.error() << '\n'; \ + } \ + ASSERT_TRUE(Res##NAME.has_value()); \ + auto NAME = Res##NAME.value();This keeps the helpful error printing while routing failures through the new test harness.
31-40: Make themax_subpattern_numargument explicit (or confirm the default).In
MaxSubGlobyou call:auto Pat1 = clice::GlobPattern::create("{AAA,BBB,AB*}");Every other call in this file passes an explicit
max_subpattern_num(e.g.,..., 100)), and the implementation definition you provided isGlobPattern::create(llvm::StringRef s, size_t max_subpattern_num)with no visible default. If the header does not declare a default value, this will fail to compile; even if it does, the inconsistency is a bit confusing.I’d suggest either:
- auto Pat1 = clice::GlobPattern::create("{AAA,BBB,AB*}"); + auto Pat1 = clice::GlobPattern::create("{AAA,BBB,AB*}", 100);or, if the intention is to test the default, add a brief comment explaining that you rely on the default
max_subpattern_numhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/Support/GlobPattern.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/Support/GlobPattern.cpp (1)
src/Support/GlobPattern.cpp (4)
create(162-212)create(162-163)create(214-289)create(215-215)
⏰ 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). (8)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, releasedbg)
🔇 Additional comments (1)
tests/unit/Support/GlobPattern.cpp (1)
18-495: GlobPattern tests migrate cleanly to TEST_SUITE/TEST_CASE/ASSERT_ macros.*The bulk of the file is a straightforward migration to the new test framework: suite structure, individual TEST_CASE groupings, and ASSERT_TRUE/ASSERT_FALSE checks all look consistent and preserve the original intent of the glob, brace, bracket, and wildcard coverage. Nice, dense coverage of tricky pattern corner cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/Server/ActiveFileManager.cpp (1)
84-85: Previously flagged issues remain unaddressed.
- Line 84: Typo "inseted" should be "inserted"
- Line 85:
std::optionalwithout template argument may cause type deduction issues
🧹 Nitpick comments (2)
tests/unit/Server/ActiveFileManager.cpp (2)
28-41: Incomplete LRU test and unused variable.The variable
secondis declared but never used, and the test doesn't verify that:
- "first" was evicted after adding "second"
- "second" is now the active entry
Apply this diff to complete the test:
auto& second = actives.add("second", OpenFile{.version = 2}); ASSERT_EQ(actives.size(), 1U); + ASSERT_FALSE(actives.contains("first")); + ASSERT_TRUE(actives.contains("second")); + ASSERT_EQ(second->version, 2U); }
100-108: Consider using consistent types for loop counter.
TotalInsertedNumissize_tbutiis declared asint. While this works for small values, using consistent types avoids potential signed/unsigned comparison warnings and is safer for larger values.- int i = TotalInsertedNum; + size_t i = TotalInsertedNum; while(iter != manager.end()) { auto& [path, openfile] = *iter; ASSERT_EQ(path, std::to_string(i)); - ASSERT_EQ(openfile->version, static_cast<uint32_t>(i)); + ASSERT_EQ(openfile->version, i); iter++; i--; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/Server/ActiveFileManager.cpp(1 hunks)
⏰ 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). (8)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
🔇 Additional comments (4)
tests/unit/Server/ActiveFileManager.cpp (4)
1-14: LGTM!Clean setup with appropriate includes and a helpful type alias for the test suite.
16-26: LGTM!Good boundary condition testing for the capacity limits.
43-69: LGTM!Thorough iterator test verifying the expected MRU-first ordering.
111-114: LGTM!Proper closing structure with helpful end-of-scope comment.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/Feature/FoldingRange.cpp (2)
273-293: TEST_CASE(CompoundStmt) has no assertions.The test runs code but never validates any expectations. Either add assertions or document why the test is intentionally assertion-free.
364-377: TEST_CASE(Directive) has no assertions.Similar to CompoundStmt, this test runs code without validating results. Consider adding assertions or a TODO/FIXME comment explaining the missing coverage.
♻️ Duplicate comments (8)
tests/unit/Support/Binary.cpp (1)
57-98: Test logic is correct.The vector serialization/deserialization tests properly validate empty, small, and larger vectors.
Note: The same loop index type issue and duplication pattern identified in the String test also apply here.
tests/unit/Compiler/Module.cpp (1)
116-124: Complete or remove the empty Normal test case.This test case builds a PCM but all assertions are commented out, providing no verification. Either re-enable the assertions or remove the test case entirely.
tests/unit/Feature/DocumentLink.cpp (1)
19-29: Missing bounds check before accessinglinks[index]- already flagged.This issue was identified in a previous review. Add
ASSERT_LT(index, links.size());before accessinglinks[index].tests/unit/Feature/CodeCompletion.cpp (2)
34-38: Test case has no assertions.This was flagged in a previous review. The test calls
code_completebut doesn't verify results.
40-46: Test case has no assertions.This was flagged in a previous review. The test calls
code_completebut doesn't verify results.tests/unit/Feature/Hover.cpp (1)
25-34:declsmap is populated but never used.This was flagged in a previous review. The
DeclCollectorpopulatesdecls, but none of the test cases use it.tests/unit/Compiler/Preamble.cpp (2)
244-246: Inverted condition breaks PCH chaining.The condition should check when
output_fileis NOT empty to use the previous PCH. Currently,pchis set to an empty path on the first iteration (when there's no previous PCH to chain), which is incorrect.- if(params.output_file.empty()) { + if(!params.output_file.empty()) { params.pch = {params.output_file.str().str(), last_bound}; }
275-290: Test case lacks assertions.This test only prints output without validating
scan()behavior. It will always pass regardless of the actual results.Consider adding assertions:
auto result = scan(content); + ASSERT_FALSE(result.module_name.empty()); for(auto& tok: result.module_name) { std::println("{}", tok.text(content)); } + ASSERT_EQ(result.includes.size(), 2U); for(auto& tok: result.includes) { std::println("include: {}", tok.file); }
🧹 Nitpick comments (27)
tests/unit/Support/Binary.cpp (2)
17-54: Consider extracting the repeated test pattern.The three test blocks follow an identical pattern: serialize, verify size/elements, check conversion, and deserialize. This could be refactored into a helper function to reduce duplication.
Example helper:
template<typename T> void test_serialization_roundtrip(const T& value, auto check_proxy) { auto [buffer, proxy] = binary::serialize(value); check_proxy(value, proxy); T deserialized = binary::deserialize(proxy); ASSERT_EQ(value, deserialized); }
156-157: Consider inline comparison to avoid temporary.Creating a temporary vector just for comparison adds slight overhead.
- auto vec = std::vector{1, 2, 3}; - ASSERT_EQ(proxy.get<"scores">().as_array().vec(), vec); + ASSERT_EQ(proxy.get<"scores">().as_array().vec(), (std::vector{1, 2, 3}));tests/unit/Support/JSON.cpp (2)
21-42: UnusedSerde<ValueRef>specialization.The custom
ValueRefstruct and itsSerdespecialization are defined but never tested in anyTEST_CASE. If this was testing stateful serialization, consider adding a test case; otherwise, remove the dead code.
103-111: Consider adding serialization tests forstd::set.This test only verifies deserialization. Since
std::setmaintains order, its serialization should be deterministic and testable.std::unordered_setordering is non-deterministic, so skipping its serialize test makes sense.TEST_CASE(SetRange) { json::Value expected = {1, 2, 3, 4, 5}; std::set<int> input = {1, 2, 3, 4, 5}; + ASSERT_EQ(json::serialize(input), expected); ASSERT_EQ(json::deserialize<decltype(input)>(expected), input); std::unordered_set<int> input2 = {1, 2, 3, 4, 5}; ASSERT_EQ(json::deserialize<decltype(input2)>(expected), input2); }tests/unit/Compiler/Diagnostic.cpp (1)
120-132: Remove unusedPCHInfovariable.Line 129 declares
PCHInfo infobut it's never used. This appears to be leftover from copy-paste of the PCHError test case.TEST_CASE(ASTError) { /// Event fatal error may generate incomplete AST, but it is fine. CompilationParams params; params.arguments = {"clang++", "main.cpp"}; params.add_remapped_file("main.cpp", R"( void foo() {} void foo() {} )"); - PCHInfo info; auto unit = compile(params); ASSERT_TRUE(unit.has_value()); }tests/unit/Feature/FoldingRange.cpp (2)
19-32: Missing bounds check before accessingranges[index].If
index >= ranges.size(), accessingranges[index]causes undefined behavior. Add a bounds check for clearer test failure messages.void expect_folding(std::uint32_t index, llvm::StringRef begin, llvm::StringRef end, feature::FoldingRangeKind kind, std::source_location loc = std::source_location::current()) { + ASSERT_LT(index, ranges.size()); auto& folding = ranges[index];
315-362: Missing size assertion for AccessSpecifier test.Unlike other test cases, this test calls
expect_folding12 times (indices 0-11) without first assertingranges.size() == 12U. Add the assertion for consistency and to catch regressions.)cpp"); + ASSERT_EQ(ranges.size(), 12U); expect_folding(0, "1", "2", Class);tests/unit/Feature/SignatureHelp.cpp (1)
12-20: Consider validating nameless_points is non-empty.
tester.nameless_points()[0]will cause undefined behavior if the code snippet has no$markers. While current tests have markers, a defensive check would improve robustness.void run(llvm::StringRef code) { tester.clear(); tester.add_main("main.cpp", code); tester.prepare(); + ASSERT_FALSE(tester.nameless_points().empty()); tester.params.completion = {"main.cpp", tester.nameless_points()[0]}; help = feature::signature_help(tester.params, {}); };tests/unit/Index/USR.cpp (1)
75-102: Commented-out test uses outdated API.The commented code references
tester.lookupUSR(...)but the current API usestester.lookup(...). If this test is re-enabled in the future, update the method calls.tests/unit/Async/Task.cpp (2)
27-59: Consider using a non-static variable or resetting state explicitly.The
static int x = 1can cause test pollution if tests are run multiple times in the same process or if test ordering changes. The test currently relies on the initial value being1and expectsxto become2after dispose and3after cancel+dispose.If the test framework runs tests repeatedly (e.g., for stress testing), subsequent runs would start with
x = 3instead ofx = 1, causing failures.Consider resetting
xat the start of the test:TEST_CASE(TaskDispose) { - static int x = 1; + static int x; + x = 1; // Reset for each test run
88-93: Remove debug print statement.The
std::println("Task1 done")appears to be leftover debug output. This will clutter test output and should be removed.auto task1 = [&]() -> async::Task<> { x = 1; co_await async::sleep(300); - std::println("Task1 done"); x = 2; };tests/unit/Support/Doxygen.cpp (2)
97-102: Redundantifcheck afterASSERT_TRUE.The
if(info_foo.has_value())block is redundant sinceASSERT_TRUE(info_foo.has_value())on line 96 should abort the test on failure. This pattern repeats throughout the file.Consider removing the redundant conditionals:
ASSERT_TRUE(info_foo.has_value()); - if(info_foo.has_value()) { - llvm::StringRef doc = info_foo.value()->content; - ASSERT_EQ(info_foo.value()->direction, - DoxygenInfo::ParamCommandCommentContent::ParamDirection::InOut); - std::println("Doc:\n```\n{}\n```\n", doc); - } + llvm::StringRef doc = info_foo.value()->content; + ASSERT_EQ(info_foo.value()->direction, + DoxygenInfo::ParamCommandCommentContent::ParamDirection::InOut); + std::println("Doc:\n```\n{}\n```\n", doc);This applies to similar patterns at lines 119-125, 127-134, 136-143, 176-181, 183-189, 206-208, 274-278, 280-286, 288-294, 296-302, 319-321.
62-145: Consider reducing verbose debug output.The numerous
std::printlncalls throughout the tests output substantial text during test runs. While useful for debugging, this may clutter CI logs.Consider either:
- Removing debug prints for production test runs
- Gating them behind a verbose flag
- Using a test-framework-specific logging mechanism
tests/unit/Feature/DocumentSymbol.cpp (2)
21-32: Minor:std::functionadds overhead for the recursive lambda.Using
std::functionfor the recursive lambdatotal_sizeintroduces indirection. For test code this is acceptable, but a simpler iterative approach or Y-combinator pattern could avoid it.Alternative using a stack-based iteration:
auto total_size_wrapper(const std::vector<feature::DocumentSymbol>& result) -> size_t { size_t size = 0; std::vector<const std::vector<feature::DocumentSymbol>*> stack = {&result}; while (!stack.empty()) { auto* current = stack.back(); stack.pop_back(); for (const auto& item : *current) { ++size; stack.push_back(&item.children); } } return size; };
167-170: Commented-out assertion left in test case.The Macro test case has a commented-out expectation with unclear intent. Either restore the assertion with the correct expected value, or remove it if the behavior is intentionally not tested.
run(main); - - /// expect(that % total_size_wrapper(symbols) == 3); + ASSERT_EQ(total_size_wrapper(symbols), 3U); // or remove if not applicable }tests/unit/Compiler/Command.cpp (2)
198-200: Empty test case is a placeholder.The
Moduletest case contains only a TODO comment. Consider either implementing it or removing it until ready.-TEST_CASE(Module) { - // TODO: revisit module command handling. -} +// TODO: Add Module test case when module command handling is implemented
238-319: Large block of commented-out code.Over 80 lines of commented-out test code for
LoadAbsoluteUnixStyleandLoadRelativeUnixStyle. This clutters the file and adds maintenance burden.Options:
- Remove entirely if the tests are no longer needed
- Restore and skip using a platform-specific skip mechanism if they should run on Linux/macOS
- Move to a separate file or tracking issue if they're planned for future implementation
The
skip_unless(Linux || MacOS)pattern suggests these were platform-conditional tests that need to be reimplemented with the new framework.tests/unit/Compiler/Directive.cpp (1)
31-41: Helper functions lack bounds checking before vector access.The
expect_include,expect_has_inl,expect_con,expect_macro, andexpect_pragmafunctions access vectors by index without validating bounds. If a test passes an invalid index (e.g., due to incorrectsize()assertion), this will cause undefined behavior rather than a clear test failure.Consider adding bounds assertions:
void expect_include(u32 index, llvm::StringRef position, llvm::StringRef path) { + ASSERT_TRUE(index < includes.size()); auto& include = includes[index];This pattern should be applied to all helper functions.
tests/unit/Feature/CodeCompletion.cpp (1)
19-20: Extraneous semicolon after function body.The semicolon after the closing brace of
code_completeis unnecessary (though valid C++).items = feature::code_complete(params, options); -}; +}tests/unit/Feature/Hover.cpp (2)
284-360: Large blocks of commented-out test code.These commented-out test blocks add noise without value. Consider removing them or converting them to proper
TEST_CASEplaceholders with TODO comments if they represent planned functionality.
36-68: Test cases lack active assertions.Multiple test cases (Namespace, RecordScope, EnumStyle, FunctionStyle, VariableStyle) call
run()and define expected text but have allEXPECT_HOVERcalls commented out. These tests will always pass without validating anything.Consider adding TODO comments at the test level or using a skip mechanism to indicate these are incomplete.
include/Test/Test.h (1)
133-149:#ifndefguards suggest potential macro conflicts.
ASSERT_NEandEXPECT_NEare wrapped in#ifndefguards while other assertion macros are not. If these macros might conflict with another testing framework (e.g., Google Test), consider applying consistent guarding to all macros or using a namespace prefix.tests/unit/Feature/InlayHint.cpp (2)
41-44: Unusedlocationparameter.The
locationparameter is declared but never used. Either remove it or pass it to the assertion for better error reporting.-void expect_size(std::uint32_t size, - std::source_location location = std::source_location::current()) { +void expect_size(std::uint32_t size) { ASSERT_EQ(hints.size(), size); }
46-57: Unusedlocationparameter.Same issue as
expect_size- thelocationparameter is declared but not used in the assertions.void expect_hint(llvm::StringRef pos, - llvm::StringRef name, - std::source_location location = std::source_location::current()) { + llvm::StringRef name) { auto offset = tester.point(pos);tests/unit/Index/TUIndex.cpp (2)
46-59: Unusedoffsetvariable.The
offsetvariable is computed but only used conceptually - the actual position lookup happens inselect()which recomputes it internally.void expect_select(llvm::StringRef pos, llvm::StringRef expect_range, llvm::StringRef file = "", std::source_location location = std::source_location::current()) { - auto offset = tester.point(pos, file); auto range = tester.range(expect_range, file); auto occurrences = select(pos, file); ASSERT_FALSE(occurrences.empty()); - /// << std::format("Fail to find symbol for offset: {}, target range: {}", offset, dump(range));
61-86: Unusedoffsetvariable.Same issue as
expect_select- theoffsetvariable is computed but only used in a commented-out error message.Either remove the unused variable or uncomment the error message to use it:
- auto offset = tester.point(pos, file); auto range = tester.range(definition, file); auto occurrences = select(pos, file);tests/unit/Feature/SemanticToken.cpp (1)
24-42:expect_tokenlogic is sound; consider using or annotatinglocLinear search over
tokensbytester.range(pos)withASSERT_EQon kind/modifiers and a finalASSERT_TRUE(found)is straightforward and correct for these small test datasets.The
std::source_location locparameter is currently unused, which may trigger -Wunused-parameter warnings. Either route it into your assertion/logging (e.g., to improve failure diagnostics) or mark it[[maybe_unused]]to keep the signature ready for future use without warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
bin/unit_tests.cc(2 hunks)include/Test/LocationChain.h(0 hunks)include/Test/Test.h(1 hunks)tests/unit/AST/SourceCode.cpp(1 hunks)tests/unit/Async/Gather.cpp(1 hunks)tests/unit/Async/Lock.cpp(1 hunks)tests/unit/Async/Sleep.cpp(1 hunks)tests/unit/Async/Task.cpp(1 hunks)tests/unit/Async/ThreadPool.cpp(1 hunks)tests/unit/Compiler/Command.cpp(1 hunks)tests/unit/Compiler/Compiler.cpp(3 hunks)tests/unit/Compiler/Diagnostic.cpp(1 hunks)tests/unit/Compiler/Directive.cpp(5 hunks)tests/unit/Compiler/Module.cpp(1 hunks)tests/unit/Compiler/Preamble.cpp(4 hunks)tests/unit/Compiler/Tidy.cpp(1 hunks)tests/unit/Feature/CodeCompletion.cpp(3 hunks)tests/unit/Feature/DocumentLink.cpp(3 hunks)tests/unit/Feature/DocumentSymbol.cpp(8 hunks)tests/unit/Feature/FoldingRange.cpp(13 hunks)tests/unit/Feature/Formatting.cpp(1 hunks)tests/unit/Feature/Hover.cpp(12 hunks)tests/unit/Feature/InlayHint.cpp(39 hunks)tests/unit/Feature/SemanticToken.cpp(6 hunks)tests/unit/Feature/SignatureHelp.cpp(2 hunks)tests/unit/Index/MergedIndex.cpp(3 hunks)tests/unit/Index/TUIndex.cpp(5 hunks)tests/unit/Index/USR.cpp(7 hunks)tests/unit/Server/ActiveFileManager.cpp(1 hunks)tests/unit/Support/Binary.cpp(2 hunks)tests/unit/Support/Compare.cpp(1 hunks)tests/unit/Support/Doxygen.cpp(3 hunks)tests/unit/Support/Enum.cpp(1 hunks)tests/unit/Support/GlobPattern.cpp(1 hunks)tests/unit/Support/JSON.cpp(2 hunks)tests/unit/Support/Struct.cpp(1 hunks)tests/unit/Support/StructedText.cpp(2 hunks)tests/unit/Test/Example.cpp(0 hunks)
💤 Files with no reviewable changes (2)
- include/Test/LocationChain.h
- tests/unit/Test/Example.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/Support/GlobPattern.cpp
- tests/unit/Async/Gather.cpp
- tests/unit/Compiler/Compiler.cpp
- tests/unit/Server/ActiveFileManager.cpp
🧰 Additional context used
🧬 Code graph analysis (18)
tests/unit/Feature/FoldingRange.cpp (2)
src/Feature/FoldingRange.cpp (2)
folding_ranges(329-331)folding_ranges(329-329)tests/unit/Feature/DocumentLink.cpp (2)
run(12-17)run(12-12)
tests/unit/Async/Lock.cpp (2)
include/Async/Gather.h (1)
task(23-34)include/Async/Task.h (2)
cancel(64-70)cancel(291-293)
tests/unit/Support/Doxygen.cpp (1)
src/Support/Doxygen.cpp (2)
strip_doxygen_info(237-249)strip_doxygen_info(237-237)
tests/unit/AST/SourceCode.cpp (1)
src/Compiler/Preamble.cpp (1)
lexer(20-20)
tests/unit/Feature/SemanticToken.cpp (3)
tests/unit/Feature/DocumentLink.cpp (2)
run(12-17)run(12-12)tests/unit/Feature/DocumentSymbol.cpp (2)
run(13-19)run(13-13)tests/unit/Feature/FoldingRange.cpp (2)
run(12-17)run(12-12)
tests/unit/Index/MergedIndex.cpp (6)
src/Index/TUIndex.cpp (2)
build(162-170)build(162-162)include/Index/TUIndex.h (1)
range(15-30)include/Support/Format.h (1)
dump(125-168)tests/unit/Index/TUIndex.cpp (2)
build_index(13-20)build_index(13-14)src/Index/MergedIndex.cpp (5)
MergedIndex(171-172)MergedIndex(174-174)MergedIndex(176-177)MergedIndex(179-179)MergedIndex(183-183)include/Index/MergedIndex.h (1)
MergedIndex(9-81)
tests/unit/Support/Binary.cpp (1)
include/Support/Binary.h (1)
is_directly_binarizable_v(32-41)
tests/unit/Feature/DocumentSymbol.cpp (1)
src/Feature/DocumentSymbol.cpp (2)
document_symbols(129-136)document_symbols(129-129)
tests/unit/Feature/DocumentLink.cpp (1)
src/Feature/DocumentLink.cpp (2)
document_links(10-39)document_links(10-10)
tests/unit/Compiler/Command.cpp (1)
src/Compiler/Command.cpp (4)
get_option_id(736-754)get_option_id(736-736)print_argv(793-804)print_argv(793-793)
tests/unit/Feature/InlayHint.cpp (4)
tests/unit/Feature/DocumentLink.cpp (2)
run(12-17)run(12-12)tests/unit/Feature/DocumentSymbol.cpp (2)
run(13-19)run(13-13)tests/unit/Feature/FoldingRange.cpp (2)
run(12-17)run(12-12)tests/unit/Feature/SemanticToken.cpp (2)
run(17-22)run(17-17)
tests/unit/Compiler/Tidy.cpp (1)
src/Compiler/Tidy.cpp (2)
is_fast_tidy_check(54-66)is_fast_tidy_check(54-54)
tests/unit/Compiler/Diagnostic.cpp (1)
src/Compiler/Compilation.cpp (6)
compile(359-364)compile(359-359)compile(366-393)compile(366-366)compile(395-415)compile(395-395)
tests/unit/Feature/SignatureHelp.cpp (9)
src/Feature/SignatureHelp.cpp (2)
signature_help(190-208)signature_help(190-191)tests/unit/Compiler/Directive.cpp (2)
run(19-29)run(19-19)tests/unit/Feature/DocumentLink.cpp (2)
run(12-17)run(12-12)tests/unit/Feature/DocumentSymbol.cpp (2)
run(13-19)run(13-13)tests/unit/Feature/FoldingRange.cpp (2)
run(12-17)run(12-12)tests/unit/Feature/Hover.cpp (2)
run(27-34)run(27-27)tests/unit/Feature/InlayHint.cpp (2)
run(15-35)run(15-15)tests/unit/Feature/SemanticToken.cpp (2)
run(17-22)run(17-17)tests/unit/AST/Resolver.cpp (2)
run(42-54)run(42-42)
tests/unit/Async/Task.cpp (1)
include/Async/Gather.h (1)
task(23-34)
tests/unit/Feature/Formatting.cpp (1)
src/Feature/Formatting.cpp (2)
document_format(35-58)document_format(35-37)
tests/unit/Compiler/Preamble.cpp (1)
src/Compiler/Preamble.cpp (4)
compute_preamble_bounds(17-58)compute_preamble_bounds(17-17)compute_preamble_bound(8-15)compute_preamble_bound(8-8)
include/Test/Test.h (1)
include/Test/Runner.h (1)
TestState(10-45)
⏰ 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). (8)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, releasedbg)
🔇 Additional comments (71)
tests/unit/Support/Compare.cpp (3)
18-43: LGTM! Clean migration to TEST_SUITE/TEST_CASE macros.The refactoring successfully migrates from the lambda-based DSL to the standardized macro-based test framework. The Equal test case correctly validates both equality and inequality for Point and Line structures using compile-time assertions.
45-63: LGTM! Comparison test correctly preserved.The Less test case properly validates the ordering comparison for both Point and Line structures, testing in both directions to ensure consistency.
65-65: LGTM! Proper suite closure.The closing comment correctly identifies the TEST_SUITE being closed, improving code readability.
tests/unit/Support/Binary.cpp (5)
15-55: Test logic is correct.The string serialization/deserialization tests properly validate empty, short, and long strings.
100-113: LGTM!The string array test is concise and correctly validates serialization/deserialization of nested containers.
122-183: Struct serialization tests are comprehensive.The tests properly validate directly binarizable structs (Point), structs with dynamic members (Foo), and nested struct containers (Points). Good use of
refl::equalfor deep equality checks.
185-211: LGTM!The recursive structure test properly validates serialization/deserialization of deeply nested self-referential types. The use of
refl::equalis appropriate for the complex comparison.
3-3: I'll help you verify the review comment about the include statement at line 3 oftests/unit/Support/Binary.cpp. Let me check ifSupport/Compare.hexists and provides therefl::equalfunction.
<function_calls>
#!/bin/bashSearch for Compare.h in the repository
echo "=== Searching for Compare.h ==="
fd -t f "Compare.h"echo ""
echo "=== Checking if include/Support/Compare.h exists ==="
ls -la include/Support/Compare.h 2>/dev/null || echo "File not found at include/Support/Compare.h"echo ""
echo "=== Checking Support directory contents ==="
ls -la include/Support/ 2>/dev/null | head -20
</function_calls><function_calls>
#!/bin/bashIf Compare.h exists, check for refl::equal definition
echo "=== Searching for refl::equal in the codebase ==="
rg -n "refl::equal" --type cppecho ""
echo "=== Searching for Compare.h in include paths ==="
rg -n "Compare.h" --type cppecho ""
echo "=== Checking the actual test file ==="
cat -n tests/unit/Support/Binary.cpp | head -20
</function_calls>tests/unit/Async/ThreadPool.cpp (2)
34-36: Thread ID uniqueness assertions may be flaky.The test assumes all three tasks run on distinct threads. However, this depends on the thread pool size and scheduling. If the pool has fewer than 3 threads, or if a thread completes and picks up another task, thread IDs could repeat. Consider either:
- Verifying the thread pool has at least 3 threads, or
- Asserting that at least one ID differs from the main thread (weaker but stable), or
- Documenting that the test requires a pool size ≥ 3.
6-37: LGTM on the test structure and framework migration.The migration to TEST_SUITE/TEST_CASE is clean. The task scheduling pattern (create → schedule → run → verify) correctly demonstrates the async framework usage.
tests/unit/Compiler/Tidy.cpp (1)
8-28: LGTM!The test cases are well-structured:
FastCheckvalidates the fast check lookup against known values and edge cases.Tidycorrectly uses malformed code (return 0 }missing semicolon) to verify diagnostic generation with clang-tidy enabled.tests/unit/Async/Sleep.cpp (1)
6-20: LGTM!Clean migration to the new test framework. The test correctly validates that
async::sleepallows the coroutine to complete and that state modifications occur as expected.tests/unit/Feature/Formatting.cpp (1)
6-11: LGTM!The test correctly validates that
document_formatproduces edits for the given input. Using0Ufor the size comparison is appropriate forsize_t.tests/unit/Support/JSON.cpp (1)
48-190: LGTM on the overall test migration.Comprehensive test coverage for JSON serialization across string types, maps, sets, sequences, enums, and structs. The inclusion of both standard library and LLVM ADT types ensures broad compatibility testing.
tests/unit/Compiler/Module.cpp (1)
57-114: LGTM!The Scan test case correctly validates module scanning behavior with proper assertions for both interface units and implementation units. The FIXME comments adequately document the temporarily disabled tests pending standard library path resolution.
tests/unit/Compiler/Diagnostic.cpp (3)
83-90: LGTM!The CommandError test correctly validates that compilation fails when no input file is provided in the arguments.
92-103: LGTM!The Error test properly validates that syntax errors produce diagnostics while still yielding a compilation unit.
105-118: LGTM!The PCHError test correctly validates that PCH generation fails when the source contains fatal errors (duplicate function definitions).
tests/unit/Support/StructedText.cpp (1)
9-117: LGTM, but consider adding output assertions.The test cases successfully exercise the StructedText API for various formatting scenarios. Currently they serve as smoke tests (verifying no crashes during construction). For stronger regression protection, consider uncommenting the print statements or adding assertions that validate
st.as_markdown()output against expected strings.tests/unit/Async/Lock.cpp (2)
8-43: LGTM!The Lock test correctly validates that
async::Lockserializes access across concurrent tasks. The assertions verify that each task sees the expected value ofxset by the previous task holder.
45-76: Task scheduling and disposal pattern are correct and consistent with codebase patterns.Verification confirms the test is properly written:
Pre-scheduled tasks with
async::run(): The pattern of scheduling tasks viaschedule()before callingasync::run()is consistent across the codebase (ThreadPool.cpp and Task.cpp tests). Scheduled tasks execute within the event loop managed byasync::run().
dispose()aftercancel(): This cleanup pattern is demonstrated in Task.cpp (lines 42-43 and 50-53) and is the correct pattern for cancelled tasks. Thedispose()call sets the Disposable flag, enabling the coroutine to be destroyed when finished/cancelled, rather than left suspended.The test assertions are sound:
x == 3: All three tasks increment x in the lambda before attempting locky == 2: Only task1 and task3 complete their locked sections; task2 is cancelled while awaiting the locktests/unit/Support/Struct.cpp (4)
17-63: LGTM!The FieldName test case effectively validates compile-time reflection for member names using
static_assert. The union workaround for non-default-constructible types is a valid approach for obtaining pointers-to-members at compile time.
65-85: LGTM!The Foreach test properly validates both single-object iteration and two-object mutation patterns with
refl::foreach.
87-117: LGTM!The Inheritance test correctly validates that
inherited_structproperly exposes base class members to reflection, enabling iteration over the complete field set including inherited members.
119-149: LGTM!The TupleLike test validates that
std::pairandstd::tuplework correctly with the reflection system using index-based member names ("0", "1").tests/unit/Feature/DocumentLink.cpp (1)
31-78: LGTM!The test cases are well-structured with appropriate assertions for size validation and link expectations. The path normalization workaround is documented with a FIXME comment.
tests/unit/Support/Enum.cpp (1)
33-145: LGTM!Good migration to the new test framework. Appropriate use of
static_assertfor compile-time checks andASSERT_*macros for runtime verification in the MaskEnum test.tests/unit/Feature/SignatureHelp.cpp (1)
22-40: LGTM - acknowledged minimal coverage.The test structure is correct and the FIXME comment appropriately notes that more tests are needed. Consider expanding coverage when time permits.
tests/unit/Index/USR.cpp (1)
104-348: LGTM!The USR tests are well-structured with proper isolation (each test creates its own USRTester), appropriate assertions, and comprehensive coverage of various template and constraint scenarios.
tests/unit/Async/Task.cpp (4)
9-11: LGTM!Simple test case that exercises
async::run()without any scheduled tasks. Good baseline test.
13-25: LGTM!The task scheduling test correctly verifies that a scheduled task runs to completion and produces the expected result.
61-81: LGTM!The cancel test correctly verifies that cancellation prevents continuation past the await point. The captured variable approach works well here.
119-123: LGTM!Good assertions verifying that recursive cancellation propagates correctly through the task hierarchy. The comment clarifies the expected behavior.
tests/unit/Support/Doxygen.cpp (2)
13-60: LGTM!Comprehensive test for DoxygenInfo covering parameter commands, block command comments, and direction handling. The set-based comparison ensures all expected values are present.
147-324: LGTM!The integrated parser tests provide thorough coverage of complex Doxygen comment scenarios including multi-line blocks, inline commands, and mixed content.
tests/unit/Feature/DocumentSymbol.cpp (2)
10-19: Shared mutable state between test cases may cause test pollution.
testerandsymbolsare declared at the suite level and mutated across all test cases. If a test case fails mid-execution, subsequent tests may start with corrupted state. Therun()function does calltester.clear(), which mitigates this, but thesymbolsvector retains the previous result untilrun()is called again.Consider whether test isolation could be improved by declaring these as local variables within each test case, or verify that the current pattern is intentional for performance reasons.
34-57: LGTM!The test cases comprehensively cover namespaces, structs, fields, constructors, methods, enums, and top-level variables with appropriate assertions.
Also applies to: 59-72, 74-91, 93-106, 108-123, 125-143, 145-153
tests/unit/AST/SourceCode.cpp (1)
10-67: LGTM!The IgnoreComments test thoroughly validates both comment-ignoring and comment-retaining lexer modes with explicit token kind assertions.
tests/unit/Compiler/Command.cpp (6)
15-33: Localprint_argvdiffers from the one insrc/Compiler/Command.cpp.This local version produces space-separated output with escaping (
clang++ main.cpp), while the version insrc/Compiler/Command.cpp(lines 792-803) produces bracket-wrapped output ([clang++ main.cpp]). This is intentional for test comparison, but the naming collision could cause confusion.Consider renaming to
format_argv_for_testor similar to clarify that this is test-specific formatting.
45-91: LGTM!Comprehensive coverage of option ID parsing across different option classes (Group, Input, Unknown, Flag, Joined, Separate, CommaJoined, JoinedOrSeparate).
103-123: LGTM!Good coverage of default filter behavior including PCH-related filtering for different build systems (CMake, cl.exe).
125-146: LGTM!Tests verify command reuse and argument sharing between compilations.
148-196: LGTM!Thorough testing of remove/append operations with various argument formats including wildcards.
202-214: LGTM!The ResourceDir test correctly validates that the resource directory is injected into the argument list.
tests/unit/Compiler/Directive.cpp (2)
8-16: Shared mutable state at suite level may cause test interdependencies.The
Tester, vectors, and other state are declared at suite scope and shared across all test cases. Whilerun()clears and repopulates these, any test that forgets to callrun()will operate on stale data from the previous test. This is acceptable if the pattern is followed consistently, but worth noting as a potential pitfall.
78-108: Test caseIncludeis well-structured.The test properly validates the expected count before accessing individual elements and uses the helper functions consistently.
bin/unit_tests.cc (1)
54-54: LGTM!The migration to
Runner2::instance().run_tests(test_filter)aligns with the new test framework API.tests/unit/Feature/CodeCompletion.cpp (1)
24-32: LGTM!
TEST_CASE(Score)properly validates the completion results with assertions.include/Test/Test.h (2)
80-87: Stack trace filtering may remove relevant frames.The
print_tracefunction erases all frames from the first frame with a different filename onwards. This assumes all relevant frames are in the same file as the assertion, which may not hold true for helper functions or nested calls across test utilities.Consider keeping a few frames beyond the assertion location for better context, or filtering more selectively.
18-78: Test framework infrastructure looks well-designed.The
TestSuiteDeftemplate with CRTP pattern, automatic test registration via static initialization, and support for setup/teardown is a clean design. The separation ofTestStateandTestAttrsinRunner.hprovides good extensibility.tests/unit/Index/MergedIndex.cpp (2)
15-22: LGTM!The
build_indexhelper follows the same pattern as the one inTUIndex.cpp(lines 12-19), maintaining consistency across the index test files.
48-74: LGTM!The serialization test properly builds an index, merges file indices, serializes and deserializes each merged index, and asserts equality. The control flow is clear and assertions are appropriate.
tests/unit/Compiler/Preamble.cpp (3)
13-23: LGTM!The
expect_boundshelper cleanly validates preamble bounds against annotated marks usingASSERT_EQ.
25-87: LGTM!The
expect_build_pchhelper properly tests PCH building workflow: creates temp file, sets up compilation params, builds PCH, then rebuilds AST with the PCH. Error handling withASSERT_TRUEis appropriate.
89-114: LGTM!The Bounds test case covers multiple scenarios including empty preambles, single includes, preprocessor blocks, and module declarations.
tests/unit/Feature/InlayHint.cpp (3)
59-568: LGTM!The Parameters test case is comprehensive, covering normal params, anonymous params, references, variadic templates, forwarding, operators, constructors, function pointers, and many edge cases. Good test coverage.
570-810: LGTM!The Types test case thoroughly tests type hints for auto, decorations, decltype, lambdas, structured bindings, return type deduction, and template handling.
812-891: LGTM!The skipped test cases (Designators, BlockEnd, DefaultArguments) are properly marked with
{.skip = true}, indicating they are work-in-progress or known-failing tests.Also applies to: 893-1285, 1287-1329
tests/unit/Index/TUIndex.cpp (3)
22-44: LGTM!The
selecthelper properly handles file ID resolution and useslower_boundwith a custom comparator to efficiently find occurrences containing the target offset.
88-104: LGTM!The Basic test case properly validates index structure (relations count, occurrences count) and tests the
expect_selecthelper for all annotated positions.
106-242: LGTM!The template test cases (ClassTemplate, FunctionTemplate, AliasTemplate, VarTemplate, Concept) comprehensively test go-to-definition for various template specialization scenarios. The commented-out test cases with FIXMEs properly document known limitations.
tests/unit/Feature/SemanticToken.cpp (10)
1-3: Test framework include is appropriateIncluding
Test/Test.halongsideTest/Tester.hand the feature header matches the new TEST_SUITE/TEST_CASE-based infrastructure; nothing else needed here.
12-22: Suite-scoped tester andrunhelper look correctKeeping
Tester tester;andfeature::SemanticTokens tokens;at suite scope and centralizing compilation inrun()(clear → add_main → compile_with_pch → semantic_tokens) matches the pattern from other Feature tests and ensures each test runs on a fresh unit with updated tokens.
44-58: Include-token expectations align with annotated sourceThe
Includetest covers angled vs quoted headers and the split#/includeform, and the expectedDirective/Headerkinds for each annotated span look consistent with the markup.
60-67: Comment token test is minimal and adequateThe
Commenttest verifies a single line comment is recognized asComment, which is sufficient sanity coverage for this feature.
69-78: Keyword token expectations look correctMarking
intandreturnasKeywordmatches typical semantic token classification and provides a basic regression check for keyword handling.
80-87: Macro directive vs macro name classification is clearDistinguishing the directive token (
#defineasDirective) from the macro identifier (FOOasMacro) mirrors expected LSP-style semantic token behavior.
89-109: Final/override classification matches C++ semanticsTagging
final/overrideoccurrences asKeywordin different inheritance positions (A,C,D) exercises the semantic tokenizer around specifiers on virtual functions; expectations look consistent.
111-145: Variable declaration/definition and template modifiers are well-coveredThis block exercises:
- extern vs non-extern (
x1declaration vsx2definition),- templated extern/definition pairs (
y1–y4) withTemplatedmodifier,- explicit specialization (
y5) treated as a plainDefinition,- and a non-modified use (
x3).The chosen
Variablekind andDeclaration/Definition/Templatedmodifiers for each annotation look consistent and provide good coverage of the variable cases.
147-168: Function declaration/definition and templated function tokens are consistentThe
foo1/foo2andbar1/bar2pairs mirror the variable tests:
Function + Declaration/Definitionfor non-template,Function + (Declaration|Definition, Templated)for templates.This is a clear and coherent set of expectations for function tokens.
170-191: Record declaration vs definition coverage is completeThe
RecordDecltest validates:
class,struct, andunionforward declarations vs definitions,- with appropriate
Class/Struct/Unionkinds andDeclaration/Definitionmodifiers.This nicely rounds out coverage for record types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/Support/Doxygen.cpp (1)
22-27: Consider splitting compound assertions for clearer failure messages.When combining multiple conditions in a single
ASSERT_TRUE, a failure won't indicate which part failed. Splitting them improves debuggability.auto param_foo = di.find_param_info("foo"); - ASSERT_TRUE(param_foo.has_value() && param_foo.value()->content.compare("Doc for foo") == 0); + ASSERT_TRUE(param_foo.has_value()); + ASSERT_EQ(param_foo.value()->content, "Doc for foo"); auto param_bar = di.find_param_info("bar"); - ASSERT_TRUE(param_bar.has_value() && param_bar.value()->content.compare("Doc for bar") == 0); + ASSERT_TRUE(param_bar.has_value()); + ASSERT_EQ(param_bar.value()->content, "Doc for bar");tests/unit/Test/Annotation.cpp (1)
27-28: Use consistent types for offset variables.Lines 27-28 use
uint32_tandsize_trespectively, while the rest of the code consistently usesstd::uint32_tfor offsets.Apply this diff for consistency:
- uint32_t key_start = i + 2; - size_t key_end = content.find(')', key_start); + std::uint32_t key_start = i + 2; + auto key_end = content.find(')', key_start);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/Test/Annotation.h(2 hunks)include/Test/TExpr.h(0 hunks)tests/unit/Support/Doxygen.cpp(3 hunks)tests/unit/Test/Annotation.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- include/Test/TExpr.h
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/Support/Doxygen.cpp (1)
src/Support/Doxygen.cpp (2)
strip_doxygen_info(237-249)strip_doxygen_info(237-237)
⏰ 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). (8)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
🔇 Additional comments (8)
tests/unit/Support/Doxygen.cpp (4)
1-5: LGTM!The include for
Support/Logging.his correctly added to support theLOG_DEBUGcalls used throughout the test cases.
60-143: LGTM!The test case covers good edge cases including malformed commands (
@,@paramwithout arguments) and validates parameter directions correctly. The defensiveif(x.has_value())checks after assertions are a reasonable safety pattern.
145-322: LGTM!Comprehensive integration tests that cover real-world doxygen comment formats including multiline content, multiple block commands, inline formatting, and return documentation. Good validation of expected block command counts.
9-10: LGTM!The TEST_SUITE/TEST_CASE migration is correctly structured and aligns with the broader test refactoring effort in this PR.
Also applies to: 324-324
include/Test/Annotation.h (1)
26-26: LGTM! Clean separation of interface and implementation.The refactoring moves method implementations from the header to the source file, which is appropriate for test infrastructure code. The declarations correctly match the implementations in
tests/unit/Test/Annotation.cpp.Also applies to: 48-48
tests/unit/Test/Annotation.cpp (3)
62-107: Range annotation parsing logic looks solid.The implementation correctly handles:
- Range syntax
@key[...]with proper key extraction- Nested brackets via bracket level tracking
- Point annotations embedded within range content
- Proper offset tracking for LocalSourceRange construction
36-39: I'll help you verify the empty-key annotation behavior. Let me search for test cases that use the$()pattern to understand if this behavior is intentional.
<function_calls>
#!/bin/bashSearch for test cases that use $() with empty keys
rg -n '$()' tests/ --type=cpp --type=txt
</function_calls>Let me also search for related test files and examine the test structure:
<function_calls>
#!/bin/bashFind all test files related to Annotation
find tests/ -name "[Aa]nnotation" -type f
Also search for references to "nameless" which appears in the code
rg -n "nameless" tests/ --type=cpp -B2 -A2
</function_calls>Let me examine the actual implementation file to better understand the context:
<function_calls>
#!/bin/bashRead the specific file to see the implementation around lines 36-39
cat -n tests/unit/Test/Annotation.cpp | sed -n '30,50p'
</function_calls>
147-153: No issue found. Content before the first file marker is intentionally discarded.The documentation in
include/Test/Annotation.hexplicitly states thatadd_sourcesuses#[filename]to mark a new file start. The provided example shows input beginning directly with a marker. The!curr_file.empty()check on line 150 is the correct and intended implementation—any content before the first file marker is silently skipped, as expected.The test cases in
tests/unit/Compiler/Preamble.cppconfirm this is the intended behavior; they include leading content/blank lines before the first marker but do not assert these should be preserved.
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.