test: strengthen helper-hover assertions — config content check + table-driven over all 7 helpers - #135
Conversation
Replace the bare `.is_some()` check in the config-hover test with content assertions (`**config**` header + "configuration variable" synopsis), mirroring the existing route-hover test. Add a table-driven test that renders every curated helper card (route, view, config, auth, app, session, cache) and asserts each carries its own `**name**` header plus a synopsis keyword unique to that one helper. This locks in that each helper renders its own card, not a wrong or empty one — a bare `is_some()` would pass even if the wrong card rendered. Fixes #119
There was a problem hiding this comment.
✅ Approved
Review Summary
- Reviewed PR #135 — strengthens the helper-hover test assertions in
laravel-lsp/src/tests/helper_identifier_hover.rs(+39/−1, test-only). - AC #1 met —
hovering_config_identifier_resolves_a_helper_carddrops the bare.is_some()and now assertscard.contains("**config**")andcard.contains("configuration variable")(lines 53–59). - AC #2 met — new
every_curated_helper_renders_its_own_card(lines 113–142) is table-driven over all 7 curated helpers in order (route, view, config, auth, app, session, cache), assertingSome,**{name}**, and a distinctive synopsis keyword each. - AC #3 met — the four protected tests (
hovering_route_identifier_resolves_a_helper_card,hovering_a_non_curated_helper_yields_no_card,hovering_the_string_argument_is_not_a_helper_identifier,helper_identifier_hover_works_in_blade_embedded_php) are untouched; the diff is exactly one removed line plus one appended test. - AC #4 met — CI green: "LSP — test, fmt, clippy" and "Extension — wasm check, fmt, clippy" both pass.
What's Good
- The keyword choices are genuinely load-bearing, not trivia. I verified each against
HELPER_CARDSinlaravel-lsp/src/hover.rs: every keyword is present in its own helper's synopsis and absent from the other six. Notably the implementer used"session store"/"cache store"(not bare"session"/"cache", which would have matched the**name**header) — that's a sharper choice than the AC's own examples and correctly defeats a session↔cache card swap. - Clear, intent-documenting comment on the new test explaining why
.is_some()was insufficient.
📋 Non-blocking follow-ups
- The Blade-embedded test
helper_identifier_hover_works_in_blade_embedded_phpstill uses a bare.is_some()assertion —laravel-lsp/src/tests/helper_identifier_hover.rs:110— its own comment claims "the card renders the same regardless of host file type" but nothing asserts the card content. Outside this PR's diff and forbidden by AC #3 from being touched here, so it's a tracked follow-up rather than a change request. (Tracking issue opened.)
Ready for @mikebronner to merge.
|
Please fix merge conflicts |
…est conflict Both branches appended independent tests to laravel-lsp/src/tests/ helper_identifier_hover.rs after the Blade test: - this branch: every_curated_helper_renders_its_own_card (#119) - main (#133): vendored_helpers_file_{present,absent} source-link tests Conflict was purely structural (overlapping closing braces) — kept both test functions. fmt, clippy --all-targets -D warnings, and test --all-features (2105 tests) all green locally with CI fixtures.
|
Merge conflicts resolved. 🔀 Rebased intent without rewriting history — merged Verified locally with the CI fixtures (
CI is green again and the PR is |
Summary
Implements #119 — strengthens the curated helper-identifier hover tests so they assert rendered card content, not just that a card exists.
Changes
hovering_config_identifier_resolves_a_helper_card: replaced the barehover::helper_identifier_card(...).is_some()with content assertions —card.contains("**config**")(header) andcard.contains("configuration variable")(synopsis) — mirroring the existingroutetest.every_curated_helper_renders_its_own_card: a table-driven test over all 7 curated helpers (route,view,config,auth,app,session,cache). For each it asserts the card isSome, contains**{name}**, and contains a synopsis keyword unique to that one helper — so a swapped or empty card now fails.Note on keyword choice
For
session/cachethe AC's example keywords ("session"/"cache") also appear in the**name**header, which would make the synopsis check redundant with the header check. I usedsession store/cache storeinstead — distinctive across all 7 synopses — which better delivers the AC's stated goal ("lock in that each helper renders its own card"). Within the AC's "e.g." latitude; no production code touched.Acceptance Criteria
hovering_config_identifier_resolves_a_helper_cardupdated: bare.is_some()replaced withcard.contains("**config**")andcard.contains("configuration variable")Some,**{name}**, and a distinctive synopsis keyword eachhovering_route_identifier_resolves_a_helper_card,hovering_a_non_curated_helper_yields_no_card,hovering_the_string_argument_is_not_a_helper_identifier,helper_identifier_hover_works_in_blade_embedded_php) remain green and unmodifiedcargo test --lib --bins,cargo fmt --check, andcargo clippypass with no new warningsTest Plan
cargo test --lib --bins— 2014 tests pass (1735 lib + 279 bin), 0 failures; all 6 helper-hover tests greencargo fmt --check— cleancargo clippy --lib --bins -- -D warnings— clean (exit 0)Fixes #119