Skip to content

test infra: dead mock_lsp.rs module and unused skip_if_no_rust_analyzer! macro duplicated 19x by hand #307

Description

@bug-ops

Finding

Two independent pieces of unused/duplicated test infrastructure in crates/mcpls-core/tests/common/:

1. mock_lsp.rs is entirely dead code. MockLspServer, MockBehaviorBuilder, MockLspChannel (116 lines) are declared pub and every item is wrapped in #[allow(dead_code)] — the annotation itself is evidence nothing constructs them. A repo-wide search confirms zero call sites for MockLspServer::new, MockBehaviorBuilder::new, or MockLspChannel::new anywhere under tests/. The project's actual LSP-facing tests (ra_e2e.rs, integration/rust_analyzer_tests.rs) exercise a real rust-analyzer process instead, so this channel-based mock harness was superseded but never removed.

2. skip_if_no_rust_analyzer! is defined but never invoked, while its exact body is hand-duplicated 19 times. tests/common/test_utils.rs:28 defines:

macro_rules! skip_if_no_rust_analyzer {
    () => {
        if !$crate::common::test_utils::rust_analyzer_available() {
            eprintln!("Skipping test: rust-analyzer not available");
            return;
        }
    };
}

A repo-wide search shows zero invocations of this macro. Meanwhile tests/integration/rust_analyzer_tests.rs repeats the equivalent 3-line guard by hand at 19 separate call sites (lines 149, 195, 236, 281, 322, 366, 407, 454, 500, 557, 603, 647, 690, 716, 737, 772, 818, 857, 891), e.g.:

if !rust_analyzer_available() {
    eprintln!("Skipping: rust-analyzer not available");
    return;
}

(message text drifted slightly from the macro's — "Skipping" vs "Skipping test" — which is itself a symptom of the copies no longer having one source of truth.)

Location

  • crates/mcpls-core/tests/common/mock_lsp.rs (whole file)
  • crates/mcpls-core/tests/common/test_utils.rs:27-35 (macro definition)
  • crates/mcpls-core/tests/integration/rust_analyzer_tests.rs (19 duplicated guard blocks)

Why

Dead test scaffolding and un-DRY'd boilerplate both cost real maintenance time without adding coverage: mock_lsp.rs is compiled into the integration_tests test binary for zero benefit, and any future tweak to the skip message/behavior (e.g. adding a MCPLS_SKIP_RA-style opt-out like ra_e2e.rs already has) requires touching 19 sites instead of one. Either delete mock_lsp.rs, or wire it into a test that needs channel-based LSP mocking; and replace the 19 hand-rolled guards with skip_if_no_rust_analyzer!().

Metadata

Metadata

Assignees

No one assigned

    Labels

    P3Low: cosmetic, edge case unlikely in practiceenhancementNew feature or requestmcpls-coremcpls-core crate changestestingTest-related changes

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions