Skip to content

Add handler-level tests for Blade variable rename wiring - #165

Merged
mikebronner merged 2 commits into
mainfrom
chore/95-add-handler-level-tests-for-blade-variable-rename-wir
Jun 16, 2026
Merged

mikebronner merged 2 commits into
mainfrom
chore/95-add-handler-level-tests-for-blade-variable-rename-wir

Conversation

@mikebronner

@mikebronner mikebronner commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements #95 — adds the first handler-level tests for the Blade variable rename wiring, closing the gap Holmes flagged in the review of #55 (PR #89): the prepare_blade_var_rename / blade_var_rename_edit handlers in main.rs are only exercised indirectly through pure functions, so a wiring inversion (a bypassed/reversed is_template_variable gate, a dropped unresolved-loop refusal) would slip past the existing suite.

Changes

  • src/tests/blade_var_rename_handler.rs (new) — four #[tokio::test] handler tests.
  • minimal_backend() helper reuses the established LspService::new(LaravelLanguageServer::new).inner().clone() harness (the same one folio_rename.rs / folio_cursor_containment.rs use). The empty document cache drives the filesystem-fallback path in rename_document_text, reading a tempfile rather than an editor buffer.
  • src/tests/mod.rs — registers the new module (alphabetical).

Note on the helper vs. "no Salsa actor"

AC #1 asks for a backend with "no Salsa actor." The repo's only established backend-test harness (test_server() in folio_rename.rs) builds through LspService::new(LaravelLanguageServer::new), which spawns a real (but here inert) actor. The two rename handlers under test never touch Salsa — they read the document and call pure functions — so I followed the existing convention rather than introduce a new disconnected-SalsaHandle production API just to drop an inert thread. Happy to switch if you'd prefer the dedicated constructor.

Acceptance Criteria

  • A minimal_backend() test helper in src/tests/blade_var_rename_handler.rs constructs a LaravelLanguageServer with an empty document cache, sufficient for the filesystem-fallback path in rename_document_text to resolve a file:// URI pointing at a tempfile
  • A handler test calls prepare_blade_var_rename with a cursor on a @php-only variable (no @foreach/@forelse, no controller binding) and asserts None — catching a bypassed/reversed is_template_variable gate
  • A handler test calls prepare_blade_var_rename on a valid file-scoped Blade variable and asserts Some(Range) covering the name (not the $)
  • A handler test calls prepare_blade_var_rename inside an unresolved loop region and asserts None — covering the cursor_in_unresolved_loop gate
  • A handler test calls blade_var_rename_edit on a file-scoped Blade variable and asserts the WorkspaceEdit rewrites all in-scope occurrences and no occurrences outside scope (a loop that re-binds the same name)
  • All new tests are #[tokio::test], use tempfile::TempDir, and pass under cargo test -p laravel-lsp with no network or Laravel project required

Test Plan

  • cargo test -p laravel-lsp — lib (1760) and bin (305, incl. the 4 new) targets pass. The 8 tests/integration_tests.rs failures seen locally are pre-existing env-fixture dependencies (test-project/.env is gitignored); CI provisions them via cp test-project/.env.example test-project/.env + composer update.
  • cargo fmt --check — clean.
  • cargo clippy --all-targets --all-features -- -D warnings — clean (matches CI's warnings-as-errors gate).

Fixes #95

Cover the async main.rs rename handlers the pure blade_var_rename unit
tests can't reach: a wiring inversion in prepare_blade_var_rename or
blade_var_rename_edit (a bypassed/reversed is_template_variable gate, a
dropped unresolved-loop refusal) would slip past the pure-function suite.

- minimal_backend() reuses the existing LspService::new(..).inner().clone()
  harness from folio_rename.rs; the empty document cache drives the
  filesystem-fallback path in rename_document_text against a tempfile.
- prepare rejects a @php-only variable (is_template_variable gate).
- prepare accepts a file-scoped variable; the range excludes the $ sigil.
- prepare refuses a cursor in an unresolved loop (cursor_in_unresolved_loop).
- blade_var_rename_edit rewrites in-scope occurrences and skips a loop that
  re-binds the same name.

All #[tokio::test], tempfile-backed, no network or Laravel project required.

Refs #95
@mikebronner
mikebronner marked this pull request as ready for review June 16, 2026 00:45

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved

Review Summary

  • Reviewed PR #165 — four #[tokio::test] handler-level tests for the Blade variable rename wiring, plus the one-line mod registration. Pure test additions (+186/−0), no production code touched.
  • CI green: LSP — test, fmt, clippy passed, so the suite compiles and runs.
  • All six acceptance criteria met. Each test was verified not just to compile but to genuinely exercise the real async handler path (read URI → filesystem-fallback in rename_document_text → pure-function gates → result), so a wiring inversion would actually trip it:
    • AC #1 — minimal_backend() added; empty document cache; the Some(Range) accept test proves the filesystem-fallback path resolves an unopened file:// tempfile off disk. Note on the "no Salsa actor" clause: the only constructor (LaravelLanguageServer::new) spawns a Salsa actor unconditionally, so a literally actor-less backend isn't constructible. The helper does the right and idiomatic thing — the actor is inert (the rename handlers never message it), exactly as the existing folio_rename.rs::test_server() harness does. The AC's substance (a backend sufficient for the fallback path, not dependent on Salsa) is satisfied; "no Salsa actor" is a benign wording imprecision, and the test comment calls it out honestly.
    • AC #2 — @php-only $secret → None. Not vacuous: bypassing the is_template_variable gate would let variable_spans find $secret on that line and return Some, failing the test.
    • AC #3 — file-scoped $name → Some(Range) covering "name", with the char before the range asserted to be $. Sigil exclusion proven.
    • AC #4 — cursor in the opaque @foreach ($users /* :) */ as $user) → None. The fixture genuinely produces an unresolved loop (see follow-up below), and removing the cursor_in_unresolved_loop gate would return Some.
    • AC #5 — blade_var_rename_edit on file-scoped $item → WorkspaceEdit whose changes rewrite only lines 0 and 4; the loop-shadowed $item on line 2 is correctly skipped (lines == [0,4]). Scope shadowing proven.
    • AC #6 — all four are #[tokio::test], use tempfile::TempDir, no network, no Laravel project.

What's Good

  • Excellent doc comments: each test states the exact wiring inversion it guards against, so the why survives. This is precisely the handler-level coverage the original follow-up (#95, from PR #89's review) asked for.
  • Cursor positions are carefully placed and self-documenting; the sigil-exclusion and [0,4] scope assertions are tight and non-tautological.
  • Reuses the established handler-test harness pattern (LspService::new + inner().clone()) rather than inventing a parallel one.

📋 Non-blocking follow-ups

  • matching_paren (laravel-lsp/src/blade_loops.rs:159) is quote-aware but comment-unaware — a ) inside a /* */ comment in a @foreach header desyncs the paren scan, so an otherwise-resolvable loop is treated as unresolved and rename is refused inside it. Safe/conservative today (refuses rather than mis-renaming), and this PR legitimately exploits it to build the unresolved-loop fixture — but it suppresses a valid rename for that (rare) construct. Outside this PR's diff and not named by any AC. Tracking separately.

Ready for @mikebronner to merge.

@mikebronner
mikebronner merged commit e542b90 into main Jun 16, 2026
5 checks passed
@mikebronner
mikebronner deleted the chore/95-add-handler-level-tests-for-blade-variable-rename-wir branch June 16, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add handler-level tests for Blade variable rename wiring

1 participant