Merged
Conversation
Contributor
WalkthroughAdds extensive test coverage and integration scenarios across gateway-controller and integration tests, a new health-step for polling endpoints with method/body, and a production change forcing LLM internal upstreams to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/controlplane/controlplane_test.go`:
- Around line 580-597: The test currently ignores the error returned by
client.Start(); update the "Client lifecycle with Start and Stop" subtest to
capture and assert the Start() error (e.g., err := client.Start()) and fail the
test if it returns non-nil so failures aren't silently ignored; keep the rest of
the flow (sleep, client.Stop(), and IsConnected() assertion) intact and
reference the same createTestClient, Start, Stop and IsConnected calls.
- Around line 651-667: The test TestClient_CalculateNextRetryDelay_CappedAtMax
is calling calculateNextRetryDelay() repeatedly but never increments
client.state.RetryCount, so the exponential backoff never progresses and the max
cap isn't exercised; update the loop to increment client.state.RetryCount on
each iteration (or set it appropriately before each call) so
calculateNextRetryDelay uses increasing retry counts, ensuring
client.state.NextRetryDelay can reach and be capped by
client.config.ReconnectMax; reference the calculateNextRetryDelay method and
client.state.RetryCount (created via createTestClient) when making the change.
- Around line 478-540: The tests use long fixed sleeps and the second subtest
doesn't call the real monitor; replace the custom goroutine in the second
subtest with a real invocation of client.heartbeatMonitor() (use
client.wg.Add(1) and go client.heartbeatMonitor()), and shorten both waits by
deriving them from the client's monitoring parameters: set/compare
atomic.StoreInt64(&client.state.LastHeartbeat, ...) and then sleep for small,
calculated durations like a fraction of client.heartbeatTimeout or
client.heartbeatInterval plus a small margin so the "timeout" case sleeps just
past the timeout and the "recent heartbeat" case sleeps well under the timeout;
ensure you still close(client.stopChan) and client.wg.Wait() after each subtest.
In `@gateway/gateway-controller/pkg/utils/api_deployment_test.go`:
- Around line 415-523: The tests in
TestDeployAPIConfiguration_WebSubTopicOperations assume port 8084 is free,
causing flakiness; replace the hardcoded port usage in the test setup by
creating an in-process deterministic HTTP test server via httptest.NewServer
that simulates the failing WebSub hub (returning non-200 or delayed responses)
and configure the APIDeploymentService routerConfig.EventGateway to point at
that server (use the server's listener address/port or URL) so
DeployAPIConfiguration and any topic operations hit the controllable server;
ensure the test server is closed at the end of each subtest to avoid port leaks
and keep references like NewAPIDeploymentService, DeployAPIConfiguration, and
store.TopicManager unchanged.
In `@gateway/gateway-controller/pkg/xds/translator_test.go`:
- Around line 1107-1178: The tests call Configuration.Spec.FromWebhookAPIData
and currently discard its returned error (e.g. "_ =
webhookConfig.Configuration.Spec.FromWebhookAPIData(...)") which can hide setup
failures; update both calls in TestTranslator_TranslateAsyncAPIConfig to assert
the setup succeeded by replacing the ignored assignment with require.NoError(t,
webhookConfig.Configuration.Spec.FromWebhookAPIData(...)) so any
parsing/validation errors surface before calling
translator.translateAsyncAPIConfig and make the test assertions meaningful.
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/utils/api_deployment_test.go (3)
21-23: Rebuild gateway images and run gateway-controller unit tests before verification.These changes touch a gateway component, so rebuild local gateway images before validating, and run the gateway-controller unit suite.
As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
Based on learnings: Run unit tests for gateway-controller usingcd gateway/gateway-controller && go test ./....
525-607: Test name doesn’t match behavior.Both subtests cover success paths, but the function name implies a failure scenario.
✏️ Rename to reflect what’s actually tested
-func TestSaveOrUpdateConfig_MemoryStoreFailure(t *testing.T) { +func TestSaveOrUpdateConfig_MemoryStoreSuccessPaths(t *testing.T) {
609-663: “Rollback” test only exercises the success path.Consider renaming or add a failing store stub if you want rollback coverage.
✏️ Rename if keeping success-only coverage
-func TestUpdateExistingConfig_Rollback(t *testing.T) { +func TestUpdateExistingConfig_SuccessPath(t *testing.T) {
Krishanx92
approved these changes
Feb 5, 2026
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
2 participants
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.
Purpose
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment
Summary by CodeRabbit
Tests
Tests (Integration)
Chores