Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: minor — test-coverage follow-ups surfaced by the review. Existing suites are green and substantial; these are the gaps that could mask real regressions.
ide-db invalidation (ide/ide-db/tests/database.rs)
- Selectivity is never tested with more than one memoized analysis coexisting (every test calls
db.analysis() for exactly one entry) — the headline contract "a keystroke in one buffer must not force every other open buffer to re-analyze" is a statement about multiple analyses.
- No test for transitive closure invalidation (editing C in A→B→C) or editing one member of an import cycle (only termination is tested).
close_document's documented disk-fallback (overlay removed → disk re-read) is never verified with divergent overlay/disk content; overlay_text_beats_disk_contents tests only the open direction.
LSP e2e (apps/lsp/tests/e2e.rs, harness/mod.rs)
- No request is ever issued against a document after didClose (VS Code fires hover/definition on preview-tab close races).
- didChange-before-didOpen is unpinned:
handlers::did_change silently interns and starts tracking never-opened URIs (handlers.rs:163-176), enrolling them in future dependents republishes — decide intended semantics and pin it.
- The harness ships a full
percent_encode (harness/mod.rs:625) but every fixture path is plain ASCII, so URI encode/decode round-trips (including server-emitted URIs in publishes and cross-file definitions) are untested end-to-end — use a fixture dir with a space or non-ASCII char.
inlayHint is only requested with whole-document ranges; a handler regression that ignores params.range would be invisible.
- No e2e sends a position past EOF / beyond the last line (stale positions from rapid typing) — the wire-level null-result contract is unpinned (unit tests cover the conversion layer only).
non_file_uri_is_ignored_without_crashing claims "publishes nothing" but never asserts absence of a publish (the sibling query/fragment test shows the correct pattern).
- Flake risk:
editing_an_imported_file_republishes_open_dependents picks the FIRST main.inf publish after a fixed 500ms pre-drain (e2e.rs:843 area); a straggling clean republish under CI load causes a spurious failure. Make the drain deterministic (e.g. sequence on a request/response barrier instead of a timer).
VS Code extension
- The client lifecycle promise-queue (enqueue/doStart/doStop serialization, "a rejected operation must not wedge the queue") documents race-freedom as its core invariant but has zero tests; the pure-module + injected-deps pattern used by resolve.ts tests can cover it.
Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: minor — test-coverage follow-ups surfaced by the review. Existing suites are green and substantial; these are the gaps that could mask real regressions.
ide-db invalidation (ide/ide-db/tests/database.rs)
db.analysis()for exactly one entry) — the headline contract "a keystroke in one buffer must not force every other open buffer to re-analyze" is a statement about multiple analyses.close_document's documented disk-fallback (overlay removed → disk re-read) is never verified with divergent overlay/disk content;overlay_text_beats_disk_contentstests only the open direction.LSP e2e (apps/lsp/tests/e2e.rs, harness/mod.rs)
handlers::did_changesilently interns and starts tracking never-opened URIs (handlers.rs:163-176), enrolling them in future dependents republishes — decide intended semantics and pin it.percent_encode(harness/mod.rs:625) but every fixture path is plain ASCII, so URI encode/decode round-trips (including server-emitted URIs in publishes and cross-file definitions) are untested end-to-end — use a fixture dir with a space or non-ASCII char.inlayHintis only requested with whole-document ranges; a handler regression that ignoresparams.rangewould be invisible.non_file_uri_is_ignored_without_crashingclaims "publishes nothing" but never asserts absence of a publish (the sibling query/fragment test shows the correct pattern).editing_an_imported_file_republishes_open_dependentspicks the FIRST main.inf publish after a fixed 500ms pre-drain (e2e.rs:843area); a straggling clean republish under CI load causes a spurious failure. Make the drain deterministic (e.g. sequence on a request/response barrier instead of a timer).VS Code extension