test: integration coverage for Folio route find-references usages leg - #114
Merged
mikebronner merged 2 commits intoJun 14, 2026
Merged
mikebronner merged 2 commits into
mikebronner merged 2 commits into
Conversation
Adds an end-to-end assertion that a route('...') call-site for a
Folio-injected route name is returned by the shared SymbolIndex,
closing AC #4's explicit test gap. The route name is anchored through
the real Folio pipeline (inject_folio_routes + folio_name_for_file)
rather than hard-coded, and the call-site ParsedPatternsData +
SymbolIndex are built directly — no Backend, SalsaActor, or tokio
runtime, matching the integration suite's no-async-harness convention.
Also asserts an unrelated route name shares no bucket (no
cross-contamination between Folio and conventional route names).
Fixes #97
mikebronner
marked this pull request as ready for review
June 14, 2026 19:09
There was a problem hiding this comment.
✅ Approved
Review Summary
- Reviewed the test-only PR for #97 (+71/−0, one new unit test in
laravel-lsp/src/folio_discovery/tests.rs). This closes the find-references usages-leg coverage gap (AC #4) that round-2/3 review of #57 (PR #91) deferred into #97. - All four acceptance criteria met — verified leg by leg, not on faith:
- AC #1 ✅ —
folio_route_usages_are_returned_by_symbol_indexbuilds aParsedPatternsDatawith aroute_refscall-site entry, feeds it viaindex.insert_file(&call_site, &patterns)(tests.rs:499), and assertsfind(Route("users.show"))returns exactly that location —file_path,line == 17,column == 23(tests.rs:504-507). - AC #2 ✅ —
find(Route("other.route"))asserted empty (tests.rs:511-516), a genuine negative control. - AC #3 ✅ — no
Backend,SalsaActor, or tokio runtime; those words appear only in the doc comment. Construction isRouteIndex::new()/ParsedPatternsData::default()/SymbolIndex::default(). - AC #4 ✅ — CI green (LSP test/fmt/clippy, wasm check, CodeQL all pass); API signatures all match the tree.
- AC #1 ✅ —
- The test is honest, not vacuous.
SymbolIndex::insert_filekeys refs under(SymbolKind::Route, name)andfindis a real HashMap lookup (symbol_index.rs:120-127,:315-321) — not an echo of the input. The non-tautology pivot isassert_eq!(route_name, "users.show")(tests.rs:484): the name is recovered through the real Folio pipeline (inject_folio_routes→folio_name_for_file), and that checkpoint fails if the pipeline ever produces the wrong name. The file/line/column assertions are meaningful guards against mis-wiring. mergeable: MERGEABLE; theBLOCKEDmerge state is just this review gate.
What's Good
- Anchoring the expected route name through the real pipeline rather than hard-coding it is the right call — it makes the round-trip assertion mean something. 👍
- Clean negative control (
other.route→ empty) rules out bucket cross-contamination / fuzzy matching. - Respects the suite's no-async-harness convention exactly as the AC required — synthetic in-memory
PathBufcall-site is never read from disk, real FS access is TempDir-scoped, no new deps, nounsafe.
📋 Non-blocking follow-ups
- None.
Accepted limitation (not a follow-up): this test covers the component round-trip (Folio name → SymbolIndex insert/find), not the async-actor find-references handler that wires them together — a handler bug passing the wrong name (e.g. uri vs name) wouldn't be caught here. That's a deliberate, documented deferral: the integration suite intentionally avoids the Backend/SalsaActor/tokio harness ("not economical to fixture"), and this PR's AC explicitly required avoiding it. Tracking it as a new issue would just recreate the loop that produced #97, so I'm recording it here rather than spinning one out.
Ready for @mikebronner to merge.
mikebronner
deleted the
chore/97-test-integration-coverage-for-folio-route-find-ref
branch
June 14, 2026 20:13
4 tasks
9 tasks done
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
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
Implements #97 — adds the explicit end-to-end assertion for the Folio route find-references usages leg (AC #4) that round-2 review of #57 (PR #91) deferred. The wiring was already correct and covered implicitly by the shared reference machinery; this test makes the call-site leg explicit.
Changes
folio_route_usages_are_returned_by_symbol_indexinlaravel-lsp/src/folio_discovery/tests.rs.inject_folio_routes+folio_name_for_file) rather than hard-coding it, then builds aroute('users.show')call-site as aParsedPatternsData.route_refsentry and feeds it to a freshSymbolIndexviainsert_file.SymbolIndex::find(Route("users.show"))returns exactly that call-site (file path, line, column), and that an unrelated route name returns empty (no cross-contamination).Backend,SalsaActor, or tokio runtime — matches the integration suite's no-async-harness convention.Acceptance Criteria
folio_discovery/tests.rsbuildsParsedPatternsDatawith aroute_refscall-site for a Folio-injected route name, inserts viainsert_file, and assertsfind(Route("users.show"))returns the call-site location (file/line/column).find(Route("other.route"))is empty (no cross-contamination).Backend,SalsaActor, or anytokioruntime — usesinject_folio_routesto anchor the name plusSymbolIndex+ParsedPatternsDatabuilt directly.cargo test -p laravel-lsppasses green with the new test (no existing tests regressed).Test Plan
cargo test -p laravel-lsp folio_route_usages_are_returned_by_symbol_index.cargo fmt --checkandcargo clippy --testsclean.tests/integration_tests.rscases fail in a fresh clone (missing gitignored.env/test-projectfixtures) — pre-existing, confirmed identical on the untouched base commit; unrelated to this change.Fixes #97