fix(ide): assert the contract NewBridge actually has - #23
Conversation
TestBridge_NewBridge_Bad has failed on main since 1523144, which made NewBridge apply cfg.WithDefaults() so a zero Config cannot leave ReconnectInterval at 0 — connectLoop's min(delay*2, max) backoff would stay pinned at zero and reconnect in a tight loop. The fix was right. The test was left asserting the behaviour the fix removed: AssertEqual(t, "", bridge.cfg.LaravelWSURL) want="" got="ws://localhost:9876/ws" So the assertion has been describing the bug rather than the contract, and every push to main since has been red on it. It now asserts what NewBridge guarantees: a zero Config comes back defaulted, with a URL, a workspace root, and both reconnect intervals non-zero — the property the change existed to establish, rather than the one string it happened to set. Not caused by #20, which is red for this and touches no Go at all: its fourteen files are php/ plus composer.json and composer.lock. go test ./... now passes for the whole module. Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughThe bridge test now verifies that ChangesBridge default configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go/pkg/mcp/ide/bridge_test.go (1)
529-530: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact reconnect defaults.
These assertions only check that both intervals are positive. They pass if
Config.WithDefaults()returns any other positive values. Assert the documented defaults of2 * time.Secondand30 * time.Secondinstead.Suggested test change
- AssertTrue(t, bridge.cfg.ReconnectInterval > 0) - AssertTrue(t, bridge.cfg.MaxReconnectInterval > 0) + AssertEqual(t, 2*time.Second, bridge.cfg.ReconnectInterval) + AssertEqual(t, 30*time.Second, bridge.cfg.MaxReconnectInterval)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/pkg/mcp/ide/bridge_test.go` around lines 529 - 530, Update the assertions in the reconnect configuration test to compare bridge.cfg.ReconnectInterval exactly with 2 * time.Second and bridge.cfg.MaxReconnectInterval exactly with 30 * time.Second, preserving the existing test structure while validating Config.WithDefaults() uses the documented values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@go/pkg/mcp/ide/bridge_test.go`:
- Around line 529-530: Update the assertions in the reconnect configuration test
to compare bridge.cfg.ReconnectInterval exactly with 2 * time.Second and
bridge.cfg.MaxReconnectInterval exactly with 30 * time.Second, preserving the
existing test structure while validating Config.WithDefaults() uses the
documented values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0120111-913a-4e0a-b7df-5ef8926a22b4
📒 Files selected for processing (1)
go/pkg/mcp/ide/bridge_test.go
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
1 participant
Unblocks the
Test + Coveragegate onmain— and therefore on #20, which is red for this and touches no Go at all.The drift
TestBridge_NewBridge_Badhas failed onmainsince commit1523144, which madeNewBridgeapplycfg.WithDefaults()so a zeroConfigcannot leaveReconnectIntervalat 0 —connectLoop'smin(delay*2, max)backoff would stay pinned at zero and reconnect in a tight loop.The fix was right. The test was left asserting the behaviour the fix removed:
So the assertion has been describing the bug rather than the contract, and every push to
mainsince has been red on it.What it asserts now
That a zero
Configcomes back defaulted — a URL, a workspace root, and both reconnect intervals non-zero. The property the change existed to establish, rather than the one string it happened to set.Not caused by #20
#20's fourteen files are
php/pluscomposer.jsonandcomposer.lock— zero Go files. It inherited a redmain.Verification
go test ./...passes for the whole module.Separately: the lint gate is still red, and bigger than it looks
golangci-lintalso fails onmain, and not because of anything here. Uncapped it reports 248 issues, not the 71 the default display shows — golangci-lint caps atmax-issues-per-linter: 50andmax-same-issues: 3, the same fiction found and fixed indappcore/agent(see agent#25).This repo also has no
.golangci.ymland passes--tests=falsein CI — the same configuration that in agent both reported production seams as dead and hid dead scaffolding in the test files.That is a dedicated pass, not a rider on this PR, so it is left out and stated rather than started.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit