test(live-preview): assert capabilities contains 'change', not exact equality#98
Conversation
…equality livetemplate PR #395 (issue #252) extends the capabilities list with "progressive_enhancement", "validate", and "upload" — detected automatically based on controller methods and mountConfig. The TestWebSocketCapabilities equality assertion broke when run against that PR's core changes. Switch to a contains-style check: the test's intent is "this app's controller advertises change", not "this is the only capability the framework supports". The latter is brittle and breaks whenever the framework's auto-detected capability set grows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code ReviewSummary: This PR tightens the Correctness ✅The logic is correct. The test's actual intent was always "this controller advertises Code QualityMinor: for _, c := range caps {
if c == "change" {
for _, c := range caps {
if s, ok := c.(string); ok && s == "change" {Alternatively, if the project is on Go 1.21+, you could convert first: strCaps := make([]string, 0, len(caps))
for _, c := range caps {
if s, ok := c.(string); ok {
strCaps = append(strCaps, s)
}
}
if !slices.Contains(strCaps, \"change\") {
t.Errorf(\"Expected capabilities to include \\\"change\\\", got %v\", caps)
}That said, the current loop is perfectly readable and the implicit comparison is not a practical risk given the protocol. Nit: Comment length. The updated doc comment is 5 lines. Per the project's CLAUDE.md, comments should only appear when "the WHY is non-obvious". The // TestWebSocketCapabilities verifies the controller advertises "change".
// Other capabilities may also be present (livetemplate#252); test asserts
// presence, not exact equality.This isn't a blocking concern — the existing comment is clear — just a style note. Test Coverage ✅The change doesn't reduce coverage. Every existing subtest still runs; the weaker assertion is intentional and appropriate. The PR description correctly explains why a strict equality check is the wrong contract here. Risk Assessment: LowThis is a pure test-side change. No production code is modified. The new assertion is strictly more permissive than the old one, so it can't introduce false negatives in the current framework version, and it correctly handles the expanded capability set from livetemplate#395. Overall: Approve with optional suggestions. The type-assertion nit and comment brevity are both optional polish. The core change is correct and well-motivated. |
There was a problem hiding this comment.
Pull request overview
Updates the live-preview example’s WebSocket capabilities test to be resilient to upstream framework changes that add new auto-detected capabilities, while still asserting the app advertises the "change" capability.
Changes:
- Relax
TestWebSocketCapabilitiesfrom exact list equality (["change"]) to a presence/contains check for"change". - Expand the test comment to clarify why additional capabilities may appear (and why the test should not enforce exact equality).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
livetemplate PR #395 (issue #252) extends the auto-detected capabilities list with
"progressive_enhancement","validate", and"upload". The existingTestWebSocketCapabilitiesequality assertion (len(caps) != 1 || caps[0] != "change") broke when run against that PR's core changes via the cross-repo CI.Switch to a
contains-style check: the test's intent is "this app's controller advertiseschange", not "this is the only capability the framework supports". The latter is brittle and breaks whenever the framework's auto-detected capability set grows.Test plan
go test ./live-preview/...passes locally"change"(the original intent)🤖 Generated with Claude Code