refactor(tests): reorganize integration tests into domain-based subdirectories - #409
Merged
Conversation
…rectories Extract shared test utilities into tests/integration/utils/ (client, workspace, assertions, wait, cache) and migrate 12 test files into categorized subdirectories: lifecycle/, compilation/, features/, modules/, extensions/, stress/. Merge include_completion + import_completion into features/test_completion.py. All 113 tests collected successfully with zero import errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… conftest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughConsolidates integration test utilities into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 7
🧹 Nitpick comments (9)
tests/integration/features/test_index.py (1)
144-144: Remove unnecessaryfprefix from string literal.This string has no placeholders, so the
fprefix is extraneous.♻️ Suggested fix
- assert len(result) > 0, f"prepareTypeHierarchy returned empty" + assert len(result) > 0, "prepareTypeHierarchy returned empty"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_index.py` at line 144, The assertion message uses an unnecessary f-string; update the assertion in the test that checks prepareTypeHierarchy (the line asserting len(result) > 0) to use a normal string literal instead of an f-string (remove the leading `f` from `"prepareTypeHierarchy returned empty"`), leaving the rest of the assertion unchanged.tests/integration/lifecycle/test_file_operation.py (1)
6-13: Remove unused importVersionedTextDocumentIdentifier.After refactoring to use the
did_changehelper,VersionedTextDocumentIdentifieris no longer used directly in this file.♻️ Suggested fix
from lsprotocol.types import ( CompletionParams, DidCloseTextDocumentParams, HoverParams, Position, SignatureHelpParams, - VersionedTextDocumentIdentifier, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/lifecycle/test_file_operation.py` around lines 6 - 13, Remove the unused import VersionedTextDocumentIdentifier from the import list in tests/integration/lifecycle/test_file_operation.py; specifically edit the import block that currently lists CompletionParams, DidCloseTextDocumentParams, HoverParams, Position, SignatureHelpParams, VersionedTextDocumentIdentifier and delete VersionedTextDocumentIdentifier so only the used symbols remain.tests/integration/features/test_completion.py (4)
6-14: Remove unusedTextDocumentIdentifierimport and usedocconsistently.
docis imported butTextDocumentIdentifieris used directly on line 180. For consistency with the rest of the refactored test suite, usedoc(uri)instead.♻️ Suggested fix for imports
from lsprotocol.types import ( HoverParams, Position, - TextDocumentIdentifier, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_completion.py` around lines 6 - 14, Remove the unused TextDocumentIdentifier import and replace any direct usages of TextDocumentIdentifier(...) with the helper doc(uri) to keep consistency with the refactored tests; update the imports to only include HoverParams, Position, and did_change (remove TextDocumentIdentifier) and change the creation site (where TextDocumentIdentifier is used) to call doc(uri) instead so the test uses the shared document helper.
188-191: Consider using the sharedget_errorshelper for consistency.The error filtering logic duplicates what
get_errorsfromtests.integration.utils.assertionsalready provides. Using the shared helper improves consistency and maintainability.♻️ Suggested fix
+from tests.integration.utils.assertions import get_errors + ... diags = client.diagnostics.get(uri, []) # Should have no errors if Math PCM was built successfully from buffer scan. - errors = [d for d in diags if d.severity == 1] + errors = get_errors(diags) assert len(errors) == 0, f"Expected no errors, got: {errors}"Note:
get_errorsmay need to be added to the__all__exports intests/integration/utils/__init__.pyif not already exported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_completion.py` around lines 188 - 191, Replace the manual error filtering of diagnostics (the diags variable and the errors list comprehension filtering for d.severity == 1) with the shared helper get_errors from tests.integration.utils.assertions: call get_errors(client.diagnostics, uri) or get_errors(diags) as appropriate and assert its length is zero; if get_errors is not exported from tests.integration.utils.__init__, add it to __all__ so the test can import it consistently.
109-164: Import completion tests are missing document close calls.The include completion tests (lines 17-106) properly close documents with
client.close(uri), but the import completion tests (test_import_completion_basic,test_import_completion_with_prefix,test_import_completion_dotted_names) do not close the opened documents. This inconsistency could lead to resource leaks or flaky tests.♻️ Suggested fix for test_import_completion_basic
assert "A" in labels, f"Expected 'A' in completion labels, got: {labels}" + + client.close(uri_b)Apply similar fixes to
test_import_completion_with_prefixandtest_import_completion_dotted_names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_completion.py` around lines 109 - 164, The three import-completion tests (test_import_completion_basic, test_import_completion_with_prefix, test_import_completion_dotted_names) open documents with client.open/open_and_wait but never close them; add client.close(uri) at the end of each test (use the uri returned when opening: uri_b or uri_app) to mirror the include-completion tests and prevent resource leaks/flakiness, ensuring each test closes any file it opened.
177-183: Usedoc(uri)instead ofTextDocumentIdentifier(uri=uri)for consistency.The shared
dochelper is already imported but not used here.♻️ Suggested fix
await client.text_document_hover_async( HoverParams( - text_document=TextDocumentIdentifier(uri=uri), + text_document=doc(uri), position=Position(line=0, character=0), ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_completion.py` around lines 177 - 183, Replace the explicit TextDocumentIdentifier usage with the shared helper: in the call to client.text_document_hover_async (the HoverParams construction), use doc(uri) instead of TextDocumentIdentifier(uri=uri) so the test is consistent with other tests; update the HoverParams argument accordingly (HoverParams(text_document=doc(uri), position=Position(...))).tests/integration/utils/__init__.py (1)
17-30: Consider sorting__all__alphabetically.The static analysis tool flags that
__all__is not sorted. While this doesn't affect functionality, sorting alphabetically improves readability and makes it easier to verify that all exports are listed.♻️ Suggested fix
__all__ = [ + "assert_diagnostics_count", + "assert_has_errors", + "assert_no_errors", "CliceClient", "doc", - "write_cdb", - "write_source", - "assert_no_errors", - "assert_has_errors", - "assert_diagnostics_count", - "wait_for_recompile", - "wait_for_index", "list_pch_files", "list_pcm_files", "read_cache_json", + "wait_for_index", + "wait_for_recompile", + "write_cdb", + "write_source", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/utils/__init__.py` around lines 17 - 30, The __all__ list is not alphabetized; please reorder the entries in the __all__ variable so they are sorted alphabetically (e.g., "CliceClient" then "assert_diagnostics_count", "assert_has_errors", "assert_no_errors", "doc", "list_pch_files", "list_pcm_files", "read_cache_json", "wait_for_index", "wait_for_recompile", "write_cdb", "write_source") to satisfy the static analysis rule and improve readability.tests/integration/features/test_server.py (2)
18-19: Avoid hard-coding server version in an integration test.Line 19 (
"0.1.0") will break on every release even when behavior is correct. Prefer asserting non-empty version (or comparing against package metadata) instead of a fixed literal.Suggested adjustment
async def test_server_info(client, workspace): assert client.init_result.server_info.name == "clice" - assert client.init_result.server_info.version == "0.1.0" + assert client.init_result.server_info.version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_server.py` around lines 18 - 19, The test currently hard-codes server version equality (assert client.init_result.server_info.version == "0.1.0"), which will break on releases; change the assertion to verify the version is non-empty or matches package metadata instead. Update the assertion on client.init_result.server_info.version to check truthiness/non-empty string (or fetch the package/module version and compare against that value) while keeping the existing name assertion (client.init_result.server_info.name == "clice") unchanged. Ensure you reference and use client.init_result.server_info.version in the updated assertion so the test remains robust across releases.
109-133: Several request tests don’t assert outcomes.These calls only verify “no exception”, but currently look like behavioral tests. Either add explicit assertions (preferred) or assign to
_with a short comment clarifying smoke-test intent.Also applies to: 161-177, 221-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_server.py` around lines 109 - 133, Several request tests (e.g., test_hover_before_compile, test_completion_request, test_signature_help_request, test_definition_request) only call client.hover_at / client.completion_at / client.signature_help_at / client.definition_at and don't assert anything; update each test to either assert expected properties on the returned result (preferred) — for example check result is not None and contains expected keys/fields — or explicitly assign the return to _ and add a short comment like "# smoke test: ensure no exception" so intent is clear; apply the same change pattern to the other affected tests referenced around the later blocks (lines 161-177 and 221-225) that use the same client.*_at helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/features/test_server.py`:
- Around line 137-141: Wrap the interaction/assertion sequence in
test_document_symbol_request (and the similar tests noted) with a try/finally so
client.close(uri) always runs: after obtaining uri via client.open_and_wait and
calling client.document_symbols (and performing assertions), ensure
client.close(uri) is invoked from a finally block to guarantee cleanup even when
assertions fail; update the tests at the other locations (lines 145-150,
153-158, 180-185, 214-249) using the same try/finally pattern around
client.open_and_wait, client.* calls, assertions, and client.close.
- Line 73: Replace the hardcoded await asyncio.sleep(...) calls in
tests/integration/features/test_server.py with the shared deterministic wait
helpers from the integration utils (e.g., wait_for_predicate or wait_for_event)
so tests wait on explicit conditions instead of time; for each sleep instance,
call the appropriate helper to poll/await a clear predicate (server
ready/connection established, recompile_complete flag, or notification queue
length > 0) with a sensible timeout, and remove the fixed-delay sleeps (the ones
currently at the three spots in this file) so the test asserts the specific
condition rather than sleeping.
In `@tests/integration/utils/cache.py`:
- Around line 31-38: The helper function list_tmp_files is defined but not
exported and appears unused; either add it to the package exports in
tests/integration/utils/__init__.py alongside list_pch_files, list_pcm_files,
and read_cache_json so it becomes public, or delete the unused function from
tests/integration/utils/cache.py if it’s unnecessary; locate the function by
name (list_tmp_files) and update the exports list or remove the function, then
run tests/grep to ensure no callers are relying on it.
In `@tests/integration/utils/client.py`:
- Around line 145-157: The helper open_and_wait currently awaits
text_document_hover_async(...) without a timeout, allowing a stalled server to
hang; wrap the hover request in asyncio.wait_for using the same timeout (or
compute a shared deadline and pass the remaining time to both calls) so the
hover call and the subsequent await asyncio.wait_for(event.wait(),
timeout=timeout) share the same deadline; update open_and_wait to call await
asyncio.wait_for(self.text_document_hover_async(...), timeout=timeout) (or
compute remaining_time after the hover to pass into event.wait()) to ensure both
operations time out predictably.
- Around line 80-85: The tests decode URIs before sending protocol messages
which breaks LSP document matching; update _normalize_uri and path_to_uri so
protocol-facing methods return the percent-encoded URI (i.e., remove or avoid
calling unquote for values returned to callers) and use _diagnostics_key(uri)
only when indexing/looking up self.diagnostics and self.diagnostics_events (keep
returned/propagated URIs percent-encoded). Also in open_and_wait wrap the
hover_async(...) call with asyncio.wait_for(..., timeout) (use the same timeout
already applied to event.wait()) so the hover request respects the timeout;
adjust callers that expect decoded paths to perform local filesystem decoding
only where needed.
In `@tests/integration/utils/wait.py`:
- Around line 30-56: The wait_for_index helper can exceed its timeout because
client.text_document_hover_async and client.workspace_symbol_async calls are
unbounded; update wait_for_index to compute a deadline (e.g., using
time.monotonic() + timeout) and before each await compute remaining = deadline -
now and return False if remaining <= 0, then wrap both
client.text_document_hover_async(...) and client.workspace_symbol_async(...)
calls with asyncio.wait_for(..., timeout=remaining) and handle
asyncio.TimeoutError by returning False (or continuing the loop for symbol polls
as appropriate) so every external request is bounded by the overall timeout;
refer to wait_for_index, client.text_document_hover_async, and
client.workspace_symbol_async to locate the changes.
- Around line 13-27: wait_for_recompile currently awaits
client.text_document_hover_async without a timeout so the helper can hang before
reaching the timed diagnostics wait; wrap the hover call in asyncio.wait_for
(use asyncio.wait_for(client.text_document_hover_async(...), timeout=timeout))
and measure elapsed time (e.g., start = loop.time()) to compute remaining =
timeout - elapsed, then call await asyncio.wait_for(event.wait(),
timeout=remaining) (and if remaining <= 0 raise/timeout immediately) so both
hover and diagnostics are bounded and share the original timeout; update
references: wait_for_recompile, client.text_document_hover_async, event.wait,
HoverParams, and asyncio.wait_for.
---
Nitpick comments:
In `@tests/integration/features/test_completion.py`:
- Around line 6-14: Remove the unused TextDocumentIdentifier import and replace
any direct usages of TextDocumentIdentifier(...) with the helper doc(uri) to
keep consistency with the refactored tests; update the imports to only include
HoverParams, Position, and did_change (remove TextDocumentIdentifier) and change
the creation site (where TextDocumentIdentifier is used) to call doc(uri)
instead so the test uses the shared document helper.
- Around line 188-191: Replace the manual error filtering of diagnostics (the
diags variable and the errors list comprehension filtering for d.severity == 1)
with the shared helper get_errors from tests.integration.utils.assertions: call
get_errors(client.diagnostics, uri) or get_errors(diags) as appropriate and
assert its length is zero; if get_errors is not exported from
tests.integration.utils.__init__, add it to __all__ so the test can import it
consistently.
- Around line 109-164: The three import-completion tests
(test_import_completion_basic, test_import_completion_with_prefix,
test_import_completion_dotted_names) open documents with
client.open/open_and_wait but never close them; add client.close(uri) at the end
of each test (use the uri returned when opening: uri_b or uri_app) to mirror the
include-completion tests and prevent resource leaks/flakiness, ensuring each
test closes any file it opened.
- Around line 177-183: Replace the explicit TextDocumentIdentifier usage with
the shared helper: in the call to client.text_document_hover_async (the
HoverParams construction), use doc(uri) instead of
TextDocumentIdentifier(uri=uri) so the test is consistent with other tests;
update the HoverParams argument accordingly (HoverParams(text_document=doc(uri),
position=Position(...))).
In `@tests/integration/features/test_index.py`:
- Line 144: The assertion message uses an unnecessary f-string; update the
assertion in the test that checks prepareTypeHierarchy (the line asserting
len(result) > 0) to use a normal string literal instead of an f-string (remove
the leading `f` from `"prepareTypeHierarchy returned empty"`), leaving the rest
of the assertion unchanged.
In `@tests/integration/features/test_server.py`:
- Around line 18-19: The test currently hard-codes server version equality
(assert client.init_result.server_info.version == "0.1.0"), which will break on
releases; change the assertion to verify the version is non-empty or matches
package metadata instead. Update the assertion on
client.init_result.server_info.version to check truthiness/non-empty string (or
fetch the package/module version and compare against that value) while keeping
the existing name assertion (client.init_result.server_info.name == "clice")
unchanged. Ensure you reference and use client.init_result.server_info.version
in the updated assertion so the test remains robust across releases.
- Around line 109-133: Several request tests (e.g., test_hover_before_compile,
test_completion_request, test_signature_help_request, test_definition_request)
only call client.hover_at / client.completion_at / client.signature_help_at /
client.definition_at and don't assert anything; update each test to either
assert expected properties on the returned result (preferred) — for example
check result is not None and contains expected keys/fields — or explicitly
assign the return to _ and add a short comment like "# smoke test: ensure no
exception" so intent is clear; apply the same change pattern to the other
affected tests referenced around the later blocks (lines 161-177 and 221-225)
that use the same client.*_at helpers.
In `@tests/integration/lifecycle/test_file_operation.py`:
- Around line 6-13: Remove the unused import VersionedTextDocumentIdentifier
from the import list in tests/integration/lifecycle/test_file_operation.py;
specifically edit the import block that currently lists CompletionParams,
DidCloseTextDocumentParams, HoverParams, Position, SignatureHelpParams,
VersionedTextDocumentIdentifier and delete VersionedTextDocumentIdentifier so
only the used symbols remain.
In `@tests/integration/utils/__init__.py`:
- Around line 17-30: The __all__ list is not alphabetized; please reorder the
entries in the __all__ variable so they are sorted alphabetically (e.g.,
"CliceClient" then "assert_diagnostics_count", "assert_has_errors",
"assert_no_errors", "doc", "list_pch_files", "list_pcm_files",
"read_cache_json", "wait_for_index", "wait_for_recompile", "write_cdb",
"write_source") to satisfy the static analysis rule and improve readability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 996ecb22-6f46-41e9-8151-9367301fc54b
📒 Files selected for processing (27)
tests/conftest.pytests/integration/compilation/__init__.pytests/integration/compilation/test_pch.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/integration/extensions/__init__.pytests/integration/extensions/test_header_context.pytests/integration/features/__init__.pytests/integration/features/test_completion.pytests/integration/features/test_index.pytests/integration/features/test_server.pytests/integration/lifecycle/__init__.pytests/integration/lifecycle/test_file_operation.pytests/integration/lifecycle/test_lifecycle.pytests/integration/modules/__init__.pytests/integration/modules/test_modules.pytests/integration/stress/__init__.pytests/integration/stress/test_rapid_edit.pytests/integration/test_import_completion.pytests/integration/test_include_completion.pytests/integration/test_server.pytests/integration/utils/__init__.pytests/integration/utils/assertions.pytests/integration/utils/cache.pytests/integration/utils/client.pytests/integration/utils/wait.pytests/integration/utils/workspace.py
💤 Files with no reviewable changes (3)
- tests/integration/test_import_completion.py
- tests/integration/test_server.py
- tests/integration/test_include_completion.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/master_server.cpp (1)
311-320:⚠️ Potential issue | 🟠 MajorGeneration counter reset creates ABA race with in-flight compilations.
Resetting the session via
Session{}resetsgenerationto 0. After the subsequentgeneration++, it becomes 1. If an in-flight compilation from a previous open capturedgen=1, the staleness check (sess->generation != gen) in the detached compilation task (compiler.cpp:727) will incorrectly pass, potentially applying old compilation results to new content.Preserve the generation counter before resetting to maintain monotonicity:
Proposed fix
auto [it, inserted] = sessions.try_emplace(path_id); auto& session = it->second; if(!inserted) { // DenseMap tombstone may retain stale data — reset to a fresh Session. + auto old_gen = session.generation; session = Session{}; + session.generation = old_gen; } session.path_id = path_id; session.version = params.text_document.version; session.text = params.text_document.text; session.generation++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 311 - 320, The reset of a Session via session = Session{} wipes session.generation and can create an ABA race with in-flight compilations (sess->generation vs gen). Fix by saving the current generation before resetting (auto old_gen = session.generation), perform the reset (session = Session{}), restore the saved generation (session.generation = old_gen) and then apply the normal update/increment (session.generation++) along with setting session.path_id, session.version and session.text; target the code around sessions.try_emplace/path_id and Session to preserve monotonic generation used by the detached compilation task check (sess->generation != gen).
♻️ Duplicate comments (2)
tests/integration/features/test_server.py (2)
105-108:⚠️ Potential issue | 🟠 MajorGuarantee
client.close(uri)withtry/finallyin document-open tests.Across these ranges,
closeis not protected if an assertion or awaited request fails. Wrap open/use/assert flows intry/finallyso cleanup always runs.Suggested pattern
async def test_document_symbol_request(client, workspace): uri, _ = await client.open_and_wait(workspace / "main.cpp") - result = await client.document_symbols(uri) - assert result is not None - client.close(uri) + try: + result = await client.document_symbols(uri) + assert result is not None + finally: + client.close(uri)Also applies to: 111-120, 123-127, 130-134, 137-141, 144-149, 152-157, 160-165, 168-175, 178-185, 188-193, 221-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_server.py` around lines 105 - 108, The test currently calls uri, _ = await client.open_and_wait(workspace / "main.cpp") and then client.close(uri) but leaves close unprotected if assertions or awaits fail; wrap the open/use/assert sequence in a try/finally so client.close(uri) always runs — i.e., after obtaining uri from client.open_and_wait(), perform assertions and awaited operations inside a try block and call client.close(uri) in the finally block (apply the same pattern to other similar ranges), referencing the existing client.open_and_wait and client.close calls and the uri variable.
75-75:⚠️ Potential issue | 🟠 MajorReplace fixed sleeps with deterministic waits.
Line 75, Line 98–99, Line 201, and Line 208–210 use time-based delays, which makes these tests timing-sensitive and flaky under slow CI. Please wait on explicit conditions/events from shared integration utils instead of
asyncio.sleep(...).Also applies to: 98-99, 201-201, 208-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_server.py` at line 75, Several tests use fixed sleeps (await asyncio.sleep(...)) which causes flaky timing; replace each sleep with deterministic waits on explicit conditions from the shared integration utilities. Locate every occurrence of await asyncio.sleep(...) in tests/integration/features/test_server.py (the instances called around the async test flows) and replace them with calls to the appropriate shared integration utility (for example wait_for_event, wait_until_condition, wait_for_server_ready, or a client.wait_for_message) that checks the exact state or event the test expects; ensure the new wait asserts a timeout and returns the condition result so the test fails deterministically if the event never occurs. Use the existing helper functions in the integration utils module rather than sleeping, and match the awaited condition to the specific expectation (e.g., server ready, websocket message received, or background task completion).
🧹 Nitpick comments (1)
tests/integration/features/test_server.py (1)
123-133: Strengthen request tests with explicit response assertions.These tests currently mostly validate “no exception thrown.” Add minimal contract assertions (e.g., expected type/non-empty fields when applicable) so regressions in response content are caught, not just transport success.
Also applies to: 137-140, 168-185
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/master_server.cpp`:
- Around line 311-320: The reset of a Session via session = Session{} wipes
session.generation and can create an ABA race with in-flight compilations
(sess->generation vs gen). Fix by saving the current generation before resetting
(auto old_gen = session.generation), perform the reset (session = Session{}),
restore the saved generation (session.generation = old_gen) and then apply the
normal update/increment (session.generation++) along with setting
session.path_id, session.version and session.text; target the code around
sessions.try_emplace/path_id and Session to preserve monotonic generation used
by the detached compilation task check (sess->generation != gen).
---
Duplicate comments:
In `@tests/integration/features/test_server.py`:
- Around line 105-108: The test currently calls uri, _ = await
client.open_and_wait(workspace / "main.cpp") and then client.close(uri) but
leaves close unprotected if assertions or awaits fail; wrap the open/use/assert
sequence in a try/finally so client.close(uri) always runs — i.e., after
obtaining uri from client.open_and_wait(), perform assertions and awaited
operations inside a try block and call client.close(uri) in the finally block
(apply the same pattern to other similar ranges), referencing the existing
client.open_and_wait and client.close calls and the uri variable.
- Line 75: Several tests use fixed sleeps (await asyncio.sleep(...)) which
causes flaky timing; replace each sleep with deterministic waits on explicit
conditions from the shared integration utilities. Locate every occurrence of
await asyncio.sleep(...) in tests/integration/features/test_server.py (the
instances called around the async test flows) and replace them with calls to the
appropriate shared integration utility (for example wait_for_event,
wait_until_condition, wait_for_server_ready, or a client.wait_for_message) that
checks the exact state or event the test expects; ensure the new wait asserts a
timeout and returns the condition result so the test fails deterministically if
the event never occurs. Use the existing helper functions in the integration
utils module rather than sleeping, and match the awaited condition to the
specific expectation (e.g., server ready, websocket message received, or
background task completion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1227b22e-54cc-45a5-bafc-0d8b2e8f0ae5
📒 Files selected for processing (2)
src/server/master_server.cpptests/integration/features/test_server.py
…one bug)
llvm::DenseMap::erase marks slots as tombstones without clearing the
value. When try_emplace reuses a tombstone slot, the stale Session
data (ast_dirty=false) persists, causing ensure_compiled() to skip
recompilation on file reopen. Fix by explicitly resetting to a fresh
Session{} when try_emplace finds an existing entry.
Also fix test_hover_before_compile timeout on Windows Debug by removing
the 30s asyncio.wait_for wrapper — this test intentionally triggers a
first compilation via hover, which can exceed 30s in Debug builds.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
refactor/integration-tests
branch
from
April 8, 2026 17:24
39633ae to
eea35aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/features/test_server.py (1)
116-133: Consider adding assertions for completion, signature help, and definition tests.These tests call the respective methods but don't assert on the results, making them purely "doesn't crash" smoke tests. While acceptable for initial coverage, consider adding basic assertions (e.g.,
result is not Noneor checking specific expected values) to validate actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/features/test_server.py` around lines 116 - 133, Update the three tests (test_completion_request, test_signature_help_request, test_definition_request) to assert that the returned results are meaningful instead of only calling the API: after calling client.completion_at, client.signature_help_at, and client.definition_at, add assertions such as result is not None and additional basic checks (e.g., for completion: result.items or result['items'] is non-empty; for signature help: result.signatures or result['signatures'] exists and length > 0; for definition: result.location or result[0] is present) to ensure the methods actually return expected structures. Keep the existing open_and_wait/close calls and make assertions minimal and robust to small variations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/features/test_server.py`:
- Around line 77-79: The test test_shutdown_exit calls
client.shutdown_async(None) directly but the client fixture already calls
_shutdown_client(c) in teardown, causing a double shutdown; remove the explicit
call to shutdown_async in test_shutdown_exit (or replace the test with
assertions that verify state after the fixture-managed teardown), or
alternatively create/use a dedicated fixture that skips the automatic
_shutdown_client teardown when you really need to exercise shutdown_async
explicitly; locate test_shutdown_exit and the client fixture/_shutdown_client to
implement the chosen fix.
---
Nitpick comments:
In `@tests/integration/features/test_server.py`:
- Around line 116-133: Update the three tests (test_completion_request,
test_signature_help_request, test_definition_request) to assert that the
returned results are meaningful instead of only calling the API: after calling
client.completion_at, client.signature_help_at, and client.definition_at, add
assertions such as result is not None and additional basic checks (e.g., for
completion: result.items or result['items'] is non-empty; for signature help:
result.signatures or result['signatures'] exists and length > 0; for definition:
result.location or result[0] is present) to ensure the methods actually return
expected structures. Keep the existing open_and_wait/close calls and make
assertions minimal and robust to small variations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd0dc53f-f36e-4dc1-8a83-77456ef31c19
📒 Files selected for processing (2)
src/server/master_server.cpptests/integration/features/test_server.py
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/utils/__init__.py`:
- Around line 18-32: The __all__ export list in
tests/integration/utils/__init__.py is missing symbols and not alphabetized; add
the missing exports assert_clean_compile and, if present in workspace.py,
did_change to the list and then sort the entire __all__ alphabetically (apply
isort-style ordering) so entries like CliceClient, assert_clean_compile,
assert_diagnostics_count, assert_has_errors, assert_no_errors, did_change, doc,
list_pcm_files, list_pch_files, list_tmp_files, read_cache_json, wait_for_index,
wait_for_recompile, write_cdb, write_source appear in sorted order; verify
did_change exists before adding.
- Around line 3-16: Add the missing public exports by importing
assert_clean_compile from tests.integration.utils.assertions and did_change from
tests.integration.utils.workspace into the package __init__.py and then include
"assert_clean_compile" and "did_change" in the module's __all__ list (keep the
list sorted with the other names); update the existing import blocks for
assertions and workspace to include these symbols so tests can import them from
tests.integration.utils directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ed5faf7-77b3-40f1-a8b9-c50658aab5cc
📒 Files selected for processing (1)
tests/integration/utils/__init__.py
This was referenced Jun 29, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/integration/utils/(client, workspace, assertions, wait, cache)lifecycle/,compilation/,features/,modules/,extensions/,stress/test_include_completion.py+test_import_completion.py→features/test_completion.pyTest plan
pytest --collect-onlycollects all 113 testspixi run formatapplied🤖 Generated with Claude Code
Summary by CodeRabbit