Add integration tests for secrets#1601
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds end-to-end secret management tests: a new Gherkin feature exercising secret CRUD, validation, and LlmProvider secret references, plus Go step definitions implementing HTTP interactions and test-suite registration for those steps. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
gateway/it/suite_test.go (1)
328-328: MissingsecretSteps.Reset()call in theBeforehook may cause state leakage between scenarios.Unlike
httpStepsandjwtStepswhich are global variables withReset()called in theBeforehook (lines 286-291), thesecretStepsinstance is created locally withinRegisterSecretStepsand is not accessible for reset. This meanslastSecretstate may persist across scenarios, potentially causing test flakiness.Consider either:
- Making
secretStepsa package-level variable (likejwtSteps) so it can be reset in theBeforehook, or- Having
RegisterSecretStepsreturn the instance so it can be stored and reset.Option 1: Add secretSteps as a package-level variable
// jwtSteps provides JWT authentication steps jwtSteps *JWTSteps + + // secretSteps provides secret management steps + secretSteps *SecretStepsThen in
InitializeTestSuiteafter line 227:secretSteps = NewSecretSteps(testState, httpSteps)Update the
Beforehook:if jwtSteps != nil { jwtSteps.Reset() } + if secretSteps != nil { + secretSteps.Reset() + }And modify
RegisterSecretStepsto accept the pre-created instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/it/suite_test.go` at line 328, The tests leak secretSteps state because RegisterSecretSteps constructs a local secretSteps whose lastSecret isn't reset between scenarios; change to create a package-level secretSteps (or have RegisterSecretSteps return the instance) so it can be reset in the Before hook like jwtSteps/httpSteps. Concretely, instantiate secretSteps via NewSecretSteps(testState, httpSteps) during InitializeTestSuite (or capture the returned instance from RegisterSecretSteps) and add secretSteps.Reset() in the Before hook alongside jwtSteps.Reset()/httpSteps.Reset(); update RegisterSecretSteps signature to accept or return the secretSteps instance accordingly so lastSecret is cleared each scenario.gateway/it/features/secrets.feature (1)
346-348: Scenario title is misleading: returning 404 for non-existent resource is not idempotent behavior.The title says "Delete secret is idempotent" but the expectation is 404 for a non-existent secret. True idempotent delete operations typically return success (200/204) regardless of whether the resource exists. Consider renaming the scenario to better reflect what's being tested:
- Scenario: Delete secret is idempotent - deleting non-existent secret returns 404 + Scenario: Delete non-existent secret returns 404🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/it/features/secrets.feature` around lines 346 - 348, The scenario title is misleading: it claims idempotency but asserts a 404 for a missing secret; update the Scenario line that currently reads "Delete secret is idempotent - deleting non-existent secret returns 404" to a clear title such as "Delete non-existent secret returns 404" (or similar) so the title matches the expectation in the steps ("When I delete the secret \"non-existent-secret-99999\"" / "Then the response status should be 404").gateway/it/steps_secrets.go (1)
89-101: Silent error swallowing on JSON unmarshal could mask API response format changes.If the API response structure changes or the unmarshal fails for other reasons, the error is silently ignored and
lastSecretwon't be updated. This could lead to confusing test failures later whenstoreSecretNameis called.Consider logging or returning the error:
Suggested improvement
if s.httpSteps.LastResponse().StatusCode == 201 { var response struct { Secret struct { Metadata struct { Name string `json:"name"` } `json:"metadata"` } `json:"secret"` } - if err := json.Unmarshal(s.httpSteps.LastBody(), &response); err == nil { + if err := json.Unmarshal(s.httpSteps.LastBody(), &response); err != nil { + return fmt.Errorf("failed to parse secret response: %w", err) + } + if response.Secret.Metadata.Name != "" { s.lastSecret.name = response.Secret.Metadata.Name s.lastSecret.exists = true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/it/steps_secrets.go` around lines 89 - 101, The JSON unmarshal error is currently ignored which can hide API changes and leave s.lastSecret unset; update the block around s.httpSteps.LastResponse(), s.httpSteps.LastBody() and the json.Unmarshal call so that if json.Unmarshal returns a non-nil err you surface it (e.g., call t.Fatalf / s.t.Fatalf or s.logger.Errorf and fail the test) instead of swallowing it, and include the error and raw response body in the log/failure message so callers of storeSecretName can reliably rely on s.lastSecret being set when a 201 response is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/it/features/secrets.feature`:
- Around line 392-399: The cleanup misses deleting the secret "upstream-secret"
created earlier; update the scenario to delete that secret as part of teardown
by adding a step to authenticate (if needed) and call the secret-deletion step
(e.g., the same step pattern used for LLM provider deletion: "When I delete the
secret \"upstream-secret\"") after the provider deletion so the secret is
removed and won't pollute subsequent runs.
In `@gateway/it/steps_secrets.go`:
- Around line 159-171: The updateSecretWithValue function currently builds JSON
via fmt.Sprintf which allows injection (same issue as createSecretWithValue);
replace the manual sprintf JSON construction in updateSecretWithValue by
constructing a Go struct or map with fields apiVersion, kind, metadata (name),
spec (displayName, description, value) and then use json.Marshal to produce the
payload so values are properly escaped; mirror the same fix applied to
createSecretWithValue and update any variables referencing the old config string
to use the marshaled bytes.
- Around line 106-118: createSecretWithValue currently constructs JSON via
fmt.Sprintf which allows JSON injection/malformed output when name or value
contain quotes, backslashes, or newlines; change it to build a Go struct
(matching apiVersion, kind, metadata.name, spec.displayName, spec.description,
spec.value) or map[string]interface{} populated with the name and value, then
call json.Marshal to produce the request body string; replace the fmt.Sprintf
usage in createSecretWithValue with the marshaled JSON (handle/maybe return
marshal errors) so values are properly escaped.
- Around line 184-188: Change the status check in the test helper from 204 to
200 so the test updates secret state correctly: in the block that inspects
s.httpSteps.LastResponse().StatusCode, replace the 204 check with 200 (so when
DeleteSecret returns http.StatusOK the code sets s.lastSecret.exists = false for
the matching s.lastSecret.name). This aligns the test helper with the
DeleteSecret handler and feature expectations.
---
Nitpick comments:
In `@gateway/it/features/secrets.feature`:
- Around line 346-348: The scenario title is misleading: it claims idempotency
but asserts a 404 for a missing secret; update the Scenario line that currently
reads "Delete secret is idempotent - deleting non-existent secret returns 404"
to a clear title such as "Delete non-existent secret returns 404" (or similar)
so the title matches the expectation in the steps ("When I delete the secret
\"non-existent-secret-99999\"" / "Then the response status should be 404").
In `@gateway/it/steps_secrets.go`:
- Around line 89-101: The JSON unmarshal error is currently ignored which can
hide API changes and leave s.lastSecret unset; update the block around
s.httpSteps.LastResponse(), s.httpSteps.LastBody() and the json.Unmarshal call
so that if json.Unmarshal returns a non-nil err you surface it (e.g., call
t.Fatalf / s.t.Fatalf or s.logger.Errorf and fail the test) instead of
swallowing it, and include the error and raw response body in the log/failure
message so callers of storeSecretName can reliably rely on s.lastSecret being
set when a 201 response is returned.
In `@gateway/it/suite_test.go`:
- Line 328: The tests leak secretSteps state because RegisterSecretSteps
constructs a local secretSteps whose lastSecret isn't reset between scenarios;
change to create a package-level secretSteps (or have RegisterSecretSteps return
the instance) so it can be reset in the Before hook like jwtSteps/httpSteps.
Concretely, instantiate secretSteps via NewSecretSteps(testState, httpSteps)
during InitializeTestSuite (or capture the returned instance from
RegisterSecretSteps) and add secretSteps.Reset() in the Before hook alongside
jwtSteps.Reset()/httpSteps.Reset(); update RegisterSecretSteps signature to
accept or return the secretSteps instance accordingly so lastSecret is cleared
each scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b81c986-feb3-4355-8795-b4a90a2e315d
📒 Files selected for processing (3)
gateway/it/features/secrets.featuregateway/it/steps_secrets.gogateway/it/suite_test.go
RakhithaRR
force-pushed
the
secret-tests
branch
from
4747707 to
de81fb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway/it/steps_secrets.go`:
- Around line 89-100: The JSON unmarshal error in the create-success branch is
being ignored; update the block that calls
json.Unmarshal(s.httpSteps.LastBody(), &response) to check the returned error
and treat it as a test/step failure (do not silently continue). If
json.Unmarshal returns an error, fail the step (e.g. return the error or call
the test failure helper) and include the raw LastBody() and the unmarshal error
in the failure message; only set s.lastSecret.name and s.lastSecret.exists =
true when unmarshal succeeds. Reference: s.httpSteps.LastResponse(),
s.httpSteps.LastBody(), json.Unmarshal, and s.lastSecret.
- Around line 57-78: RegisterSecretSteps reuses a single SecretSteps instance
across scenarios so lastSecret state persists; ensure SecretSteps.Reset() is
invoked before each scenario (add ctx.Before hook calling secretSteps.Reset() or
call secretSteps.Reset() from InitializeScenario) and update createSecret (and
createSecretWithValue) to handle json.Unmarshal errors properly by
returning/propagating the error or failing the step instead of silently ignoring
it so lastSecret.name is not left unset; update storeSecretName/deleteSecret
consumers to assume lastSecret is valid only after successful unmarshalling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22db5dc0-8691-4369-96ce-6e716cd81fb0
📒 Files selected for processing (2)
gateway/it/features/secrets.featuregateway/it/steps_secrets.go
✅ Files skipped from review due to trivial changes (1)
- gateway/it/features/secrets.feature
RakhithaRR
force-pushed
the
secret-tests
branch
from
de81fb6 to
9564d1f
Compare
2 participants
Purpose
This PR adds integration tests for Secret operations and LLM Provider flows that involve secrets.
Summary by CodeRabbit