test: ✅ Add end-to-end completion-handler test for relationship-hop wiring (#219) - #223
Merged
mikebronner merged 1 commit intoJun 18, 2026
Conversation
…iring.
Cover the main.rs `try_query_chain_completion` dispatch of
`apply_relation_method_hops`, which was only exercised indirectly: the
unit tests hit the hop and collection-column functions directly, and the
diagnostics path is covered end-to-end, but the completion glue had no
regression guard. A bad re-wire (wrong order, missing dispatch, lost
`effective_model`) would have slipped through every existing test.
Drive the real completion entry point with the property-access receiver
shape `$user->competitions->where('|')`, priming a genuine backend's
`initialized_root` (User/Competition models) and `database_schema` (a
seeded provider where `type` lives only on `competitions` and `email`
only on `users`). One test asserts `type` is offered; the mirror asserts
`email` is not — together proving the hop advanced the effective model
from User to Competition. Both fail when the dispatch block is removed.
Expose `DatabaseSchemaProvider::set_test_schema` as `#[doc(hidden)] pub`
rather than `#[cfg(test)]`: Cargo does not enable `cfg(test)` on a library
consumed as a dependency, so the bin test crate could not otherwise reach
the schema-seeding seam.
Fixes: #219
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Approved
Review Summary
A tight, well-documented test-only PR that closes the exact gap #219 named: the main.rs::try_query_chain_completion glue dispatching apply_relation_method_hops had no end-to-end regression guard. Reviewed via four blind lens passes (AC conformance, correctness, security, test honesty) over the PR checkout; CI is green (LSP test/fmt/clippy, 3m9s).
Acceptance criteria — all 6 met:
- ✅ New file
tests/query_chain_completion_handler.rsregistered intests/mod.rs:31(alphabetical). - ✅ Real backend via
LspService::new(LaravelLanguageServer::new);initialized_rootprimed with a tempdir ofUser.php(hasManycompetitions) +Competition.php;database_schemaseeded sotypelives oncompetitionsbut notusers. - ✅
property_receiver_hop_offers_related_table_columnsdrivestry_query_chain_completionwith/** @var User $user */+$user->competitions->where('◊')->get()and assertstypeis offered. (Divergence, fine: a trailing->get()completes the chain shape; cursor is still inside thewherestring arg.) - ✅ Companion
property_receiver_hop_excludes_parent_only_columnsassertsemail(users-only) is absent. - ✅ Genuine regression guard, not coverage theatre — confirmed by code logic and your documented reverse check (
["id","email"]with the dispatch neutralized). Without the hop,effective_modelstaysUser→userstable →typemissing (test 1 fails) andemailleaks (test 2 fails). The asymmetric schema catches the missing hop from both directions, and the positive assertion rules out a vacuous pass (.expect()onNone, empty-vec both fail). - ✅
cargo test+ clippy-D warningsgreen in CI.
What's Good
- The asymmetric fixture (
typeonly oncompetitions,emailonly onusers) is a sharp design — both assertions can pass only if the hop advanced the model. That's a real guard. - Hermetic and deterministic:
set_test_schemaseeds the in-memory cache, no live DB or network, tempdir RAII-cleaned. - You flagged the one production-lib change yourself in the PR body. Concur with the call: the test must live in the binary crate (
crate::LaravelLanguageServer), which consumes the lib withoutcfg(test), so a#[cfg(test)]seam is invisible cross-crate.#[doc(hidden)] pubis the right pragmatic fit here — it mirrors the already-pubinvalidate_cachecache-mutator (database.rs:522), and the feature-flag alternative would be strictly worse in this same-package bin-test topology (it'd break plaincargo testin CI). Well-documented rationale on both sides of the seam.
📋 Non-blocking follow-ups
- None.
Ready for @mikebronner to merge.
mikebronner
deleted the
chore/219-add-an-end-to-end-completion-handler-test-for-the-
branch
June 18, 2026 16:36
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 #219 — an end-to-end completion-handler regression test for the
relationship-hop wiring that #211 / PR #215 left covered only indirectly.
The unit tests exercise
apply_relation_method_hops+columns_for_collectiondirectly with a hand-built
ChainContext, and the diagnostics path is coveredend-to-end through
chain_diagnostics. The glue inmain.rs::try_query_chain_completionthat dispatchesapply_relation_method_hopsinside the real
textDocument/completionflow had no regression guard — abad re-wire (wrong call order, missing dispatch, lost
effective_model) wouldhave slipped past every existing test. These tests close that gap, mirroring the
diagnostics-side end-to-end fixtures from PR #215.
Changes
laravel-lsp/src/tests/query_chain_completion_handler.rs,registered in
laravel-lsp/src/tests/mod.rs. Builds a realLaravelLanguageServerviaLspService::new(LaravelLanguageServer::new),primes
initialized_rootwith a tempdir ofUser/Competitionmodels anddatabase_schemawith a seeded provider, then drives the realtry_query_chain_completionwith$user->competitions->where('|').property_receiver_hop_offers_related_table_columns— asserts thecompetitionscolumntypeis offered.property_receiver_hop_excludes_parent_only_columns— asserts theusers-only column
emailis not offered, proving the hop advancedeffective_modelfromUsertoCompetition.laravel-lsp/src/database.rs—DatabaseSchemaProvider::set_test_schemais now
#[doc(hidden)] pubinstead of#[cfg(test)].this is the one production-lib change. Cargo does not enable
cfg(test)on alibrary consumed as a dependency, so a
#[cfg(test)]seam is invisible to thebinary's
src/tests/crate — the AC requires primingdatabase_schemafromthere.
#[doc(hidden)]keeps it off the documented public surface while makingit reachable cross-crate. The schema mutation itself is unchanged.
Acceptance Criteria
laravel-lsp/src/tests/query_chain_completion_handler.rscreated and registered in
laravel-lsp/src/tests/mod.rs.LaravelLanguageServerviatower_lsp::LspService::new(LaravelLanguageServer::new), primesinitialized_rootwith a tempdir holdingUser.php(acompetitionshasManyto
Competition::class) andCompetition.php, and primesdatabase_schemawith a
DatabaseSchemaProviderwhosecompetitionstable carries a column(
type) absent fromusers.property_receiver_hop_offers_related_table_columnscallstry_query_chain_completionwith/** @var User $user */and$user->competitions->where('|'), cursor inside thewherestring argument,and asserts the
competitionscolumntypeis among the returned items.users-only column (email) is not amongthe returned items, confirming the hop advanced
effective_modelfromUserto
Competition.apply_relation_method_hopsdispatch block inmain.rsis commented out (verified locally: completion returns["id", "email"], sotypeis missing andemailleaks).cargo testpasses with no new warnings from the test file.Test Plan
cargo fmt --check— clean.cargo clippy --all-targets— no warnings (CI runs-D warnings).cargo test— full unit suite green (lib 1894 + bin 418, including the 2new tests).
apply_relation_method_hopsdispatchblock and confirmed both new tests fail, then restored it.
integration_teststarget needs the CI-only fixture bootstrap(
cp test-project/.env.example test-project/.env+ composer install); thosepre-existing, environment-dependent tests are untouched by this change and
pass in CI.
Fixes #219