test: migrate route service upstream coverage to ginkgo e2e#24
test: migrate route service upstream coverage to ginkgo e2e#24
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates CLI command tests from httpmock-based unit suites to Ginkgo/Gomega end-to-end suites against real APISIX; deletes many mocked command tests, adds small local argument/TTY unit checks, updates docs, go.mod, e2e bootstrap, helpers, and comprehensive Ginkgo e2e specs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This PR migrates CLI command coverage for route, service, and upstream from mocked Admin API unit tests to real end-to-end coverage using a shared Ginkgo E2E suite, aligning tests with actual APISIX behavior.
Changes:
- Adds a Ginkgo/Gomega E2E suite bootstrap and new Ginkgo specs for
route,service, andupstreamcommands. - Replaces mocked Admin API command tests with smaller “local” unit tests for purely self-contained validation paths.
- Updates E2E docker-compose and documents the revised testing strategy.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/suite_test.go | Adds Ginkgo suite bootstrap (RunSpecs) for the E2E package. |
| test/e2e/route_ginkgo_test.go | New Ginkgo E2E coverage for route command behaviors against real APISIX. |
| test/e2e/service_ginkgo_test.go | New Ginkgo E2E coverage for service command behaviors against real APISIX. |
| test/e2e/upstream_ginkgo_test.go | New Ginkgo E2E coverage for upstream behaviors, including Control API health. |
| test/e2e/scenario_coverage_test.go | Adds scenario-style E2E tests covering multi-resource interactions and real traffic. |
| test/e2e/docker-compose.yml | Makes local E2E stack ports configurable and adds host.docker.internal mapping. |
| pkg/cmd/**/*_local_test.go | Keeps only local/unit tests for CLI validation & deterministic helpers. |
| go.mod / go.sum | Adds Ginkgo/Gomega dependencies for the E2E suite. |
| docs/testing-strategy.md | Documents the new unit vs E2E testing split and Ginkgo-based E2E approach. |
| AGENTS.md | Updates repository/testing guidance to reflect the E2E-first approach for APISIX behavior. |
Comments suppressed due to low confidence (1)
docs/testing-strategy.md:146
- This doc now states e2e tests should be written with Ginkgo/Gomega, but the “Assertions” section still implies
testifyas the default assertion library. Consider clarifying that unit tests usetestifywhile Ginkgo e2e specs use Gomega matchers, or update this section to cover both to avoid mixed guidance.
## Assertions
Use the `testify` library for assertions:
```go
import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
require.NoError(t, err) // Fatal if an error occurs
assert.Equal(t, expected, actual) // Continue if the assertion fails
assert.Contains(t, output, "ID") // Check for a substring
</details>
---
💡 <a href="/api7/a6/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| time.Sleep(3 * time.Second) | ||
| for range 5 { | ||
| resp, reqErr := http.Get(gatewayURL + "/ginkgo-upstream-health/get") | ||
| if reqErr == nil && resp != nil { | ||
| g.Expect(resp.Body.Close()).To(Succeed()) | ||
| } | ||
| time.Sleep(500 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
for range 5 { ... } does not compile in Go (including go 1.22.x in this repo). Replace it with a standard counted loop (e.g., for i := 0; i < 5; i++ { ... }).
| - "${A6_E2E_GATEWAY_PORT:-19080}:9080" | ||
| - "${A6_E2E_CONTROL_PORT:-19090}:9090" |
There was a problem hiding this comment.
The docker-compose default host port mappings here (gateway/control defaulting to 19080/19090) don’t match the e2e test defaults in test/e2e/setup_test.go (APISIX_GATEWAY_URL defaults to :9080 and APISIX_CONTROL_URL defaults to :9090) or the make docker-up healthcheck URL. With the current defaults, make test-e2e against make docker-up will hit the wrong ports. Consider keeping the compose defaults aligned (9080/9090), or update make docker-up to export the corresponding env vars (APISIX_GATEWAY_URL/APISIX_CONTROL_URL) and update docs accordingly.
| - "${A6_E2E_GATEWAY_PORT:-19080}:9080" | |
| - "${A6_E2E_CONTROL_PORT:-19090}:9090" | |
| - "${A6_E2E_GATEWAY_PORT:-9080}:9080" | |
| - "${A6_E2E_CONTROL_PORT:-9090}:9090" |
| restart: always | ||
| ports: | ||
| - "8080:8080" | ||
| - "${A6_E2E_HTTPBIN_PORT:-18080}:8080" |
There was a problem hiding this comment.
The httpbin host port default is changed to 18080, but the e2e tests default HTTPBIN_URL is still http://127.0.0.1:8080 (see test/e2e/setup_test.go). This makes local e2e runs against make docker-up use the wrong httpbin port unless users manually override env vars. Align the default mapping back to 8080 or ensure make docker-up/docs set HTTPBIN_URL to match.
| - "${A6_E2E_HTTPBIN_PORT:-18080}:8080" | |
| - "${A6_E2E_HTTPBIN_PORT:-8080}:8080" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
109-123:⚠️ Potential issue | 🟡 MinorPlease document the full E2E env contract (control URL + compose port override vars).
This section is the contract for local E2E setup, but it doesn’t explicitly cover
APISIX_CONTROL_URLor theA6_E2E_*_PORTknobs introduced in compose defaults. Adding them here will prevent setup drift/confusion.Suggested doc addition
**Environment variables (for e2e tests):** | Variable | Default | Purpose | |---|---|---| | `APISIX_ADMIN_URL` | `http://127.0.0.1:9180` | Admin API base URL | | `APISIX_GATEWAY_URL` | `http://127.0.0.1:9080` | Data plane base URL | +| `APISIX_CONTROL_URL` | `http://127.0.0.1:9090` | Control API base URL | | `HTTPBIN_URL` | `http://127.0.0.1:8080` | httpbin upstream URL | +| `A6_E2E_ADMIN_PORT` | `9180` | Host admin port used by `test/e2e/docker-compose.yml` | +| `A6_E2E_GATEWAY_PORT` | `9080` | Host gateway port used by `test/e2e/docker-compose.yml` | +| `A6_E2E_CONTROL_PORT` | `9090` | Host control port used by `test/e2e/docker-compose.yml` | +| `A6_E2E_HTTPBIN_PORT` | `8080` | Host httpbin port used by `test/e2e/docker-compose.yml` |Based on learnings: Every new feature must update the documentation as per
docs/documentation-maintenance.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 109 - 123, The AGENTS.md E2E section lacks the full environment contract: add explicit documentation of the APISIX_CONTROL_URL env var (purpose, default value used by tests, and example like http://127.0.0.1:9180), and document the compose override variables introduced (A6_E2E_APISIX_PORT, A6_E2E_HTTPBIN_PORT, etc. — list each A6_E2E_*_PORT name and its default) and how to override them for local `make docker-up`/CI; place this content into the Local development section near the make commands and link to test/e2e/setup_test.go and test/e2e/suite_test.go as the canonical sources of the defaults, and update the documentation-maintenance note to remind contributors to update AGENTS.md when adding new E2E env knobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/docker-compose.yml`:
- Around line 13-15: Update the docker-compose port default values so they match
the E2E test URL expectations: change the A6_E2E_GATEWAY_PORT default from 19080
to 9080, A6_E2E_CONTROL_PORT default from 19090 to 9090, and the httpbin host
port default (A6_E2E_HTTPBIN_PORT or the 18080 mapping referenced around the
same block) from 18080 to 8080; modify the port mappings in docker-compose.yml
(the lines containing "${A6_E2E_GATEWAY_PORT:...}",
"${A6_E2E_CONTROL_PORT:...}", and the httpbin mapping) so running make docker-up
&& make test-e2e works without manual env overrides.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 109-123: The AGENTS.md E2E section lacks the full environment
contract: add explicit documentation of the APISIX_CONTROL_URL env var (purpose,
default value used by tests, and example like http://127.0.0.1:9180), and
document the compose override variables introduced (A6_E2E_APISIX_PORT,
A6_E2E_HTTPBIN_PORT, etc. — list each A6_E2E_*_PORT name and its default) and
how to override them for local `make docker-up`/CI; place this content into the
Local development section near the make commands and link to
test/e2e/setup_test.go and test/e2e/suite_test.go as the canonical sources of
the defaults, and update the documentation-maintenance note to remind
contributors to update AGENTS.md when adding new E2E env knobs.
🪄 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: 44552063-4ffb-48ba-8551-89f621cbdcb2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
AGENTS.mddocs/testing-strategy.mdgo.modpkg/cmd/route/create/create_test.gopkg/cmd/route/delete/delete_local_test.gopkg/cmd/route/delete/delete_test.gopkg/cmd/route/get/get_local_test.gopkg/cmd/route/get/get_test.gopkg/cmd/route/list/list_test.gopkg/cmd/route/update/update_local_test.gopkg/cmd/route/update/update_test.gopkg/cmd/service/create/create_test.gopkg/cmd/service/export/export_test.gopkg/cmd/service/list/list_test.gopkg/cmd/upstream/create/create_test.gopkg/cmd/upstream/delete/delete_local_test.gopkg/cmd/upstream/delete/delete_test.gopkg/cmd/upstream/export/export_test.gopkg/cmd/upstream/get/get_local_test.gopkg/cmd/upstream/get/get_test.gopkg/cmd/upstream/health/health_local_test.gopkg/cmd/upstream/health/health_test.gopkg/cmd/upstream/list/list_test.gopkg/cmd/upstream/update/update_local_test.gopkg/cmd/upstream/update/update_test.gotest/e2e/docker-compose.ymltest/e2e/route_ginkgo_test.gotest/e2e/scenario_coverage_test.gotest/e2e/service_ginkgo_test.gotest/e2e/suite_test.gotest/e2e/upstream_ginkgo_test.go
💤 Files with no reviewable changes (15)
- pkg/cmd/route/list/list_test.go
- pkg/cmd/upstream/create/create_test.go
- pkg/cmd/service/create/create_test.go
- pkg/cmd/route/get/get_test.go
- pkg/cmd/route/update/update_test.go
- pkg/cmd/upstream/get/get_test.go
- pkg/cmd/service/list/list_test.go
- pkg/cmd/upstream/update/update_test.go
- pkg/cmd/upstream/delete/delete_test.go
- pkg/cmd/service/export/export_test.go
- pkg/cmd/upstream/list/list_test.go
- pkg/cmd/upstream/health/health_test.go
- pkg/cmd/route/create/create_test.go
- pkg/cmd/upstream/export/export_test.go
- pkg/cmd/route/delete/delete_test.go
👮 Files not reviewed due to content moderation or server errors (8)
- pkg/cmd/route/delete/delete_local_test.go
- pkg/cmd/upstream/delete/delete_local_test.go
- pkg/cmd/upstream/health/health_local_test.go
- test/e2e/route_ginkgo_test.go
- docs/testing-strategy.md
- test/e2e/upstream_ginkgo_test.go
- test/e2e/service_ginkgo_test.go
- test/e2e/scenario_coverage_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i := 0; i < 10; i++ { | ||
| resp, reqErr := http.Get(gatewayURL + "/scenario-combo") | ||
| g.Expect(reqErr).NotTo(HaveOccurred()) | ||
|
|
||
| payload, readErr := io.ReadAll(resp.Body) | ||
| g.Expect(resp.Body.Close()).To(Succeed()) | ||
| g.Expect(readErr).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
The retry loop defeats its own purpose by failing immediately on transient request errors (reqErr). This can make the spec flaky when APISIX/gateway isn’t ready yet (or during brief resets). Instead, treat request errors like non-200 responses: record the last error and continue/retry, then assert after the loop that the request eventually succeeded (and include the last error/status in the failure message).
There was a problem hiding this comment.
Updated in d580e2c. The route+service+plugin-config scenario now retries on transient request errors instead of failing immediately, and the final assertion includes the last error/status for easier diagnosis.
| for i := 0; i < 10; i++ { | ||
| resp, reqErr := http.Get(gatewayURL + "/scenario-multi-node") | ||
| g.Expect(reqErr).NotTo(HaveOccurred()) | ||
|
|
||
| payload, readErr := io.ReadAll(resp.Body) | ||
| g.Expect(resp.Body.Close()).To(Succeed()) | ||
| g.Expect(readErr).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Same issue in this retry loop: g.Expect(reqErr).NotTo(HaveOccurred()) will fail on the first transient connection error and prevent retries, making the load-balancing scenario flaky. Consider retrying on request errors (and optionally non-200 status codes) and asserting after the loop with a helpful last-error/last-status message.
There was a problem hiding this comment.
Updated in d580e2c. The multi-node scenario uses the same retry pattern now: transient request errors are retried and the failure message reports the last error/status instead of failing on the first connection issue.
| func expectHeaderContains(g Gomega, raw interface{}, want string) { | ||
| switch v := raw.(type) { | ||
| case string: | ||
| g.Expect(v).To(Equal(want)) | ||
| case []interface{}: | ||
| values := make([]string, 0, len(v)) | ||
| for _, item := range v { | ||
| values = append(values, fmt.Sprintf("%v", item)) | ||
| } | ||
| g.Expect(values).To(ContainElement(want)) |
There was a problem hiding this comment.
expectHeaderContains asserts strict equality for the string case (Equal(want)), which doesn’t match the helper name and is less tolerant of upstream/header formatting differences. Either rename the helper to reflect equality semantics, or change the string assertion to a containment check (and keep the slice case as-is).
There was a problem hiding this comment.
Updated in d580e2c. I changed the string branch in expectHeaderContains to use containment semantics so the helper behavior matches its name.
|
|
||
| E2E tests validate the CLI binary against a real APISIX instance. They live in `test/e2e/` and use the `//go:build e2e` build tag to separate them from unit tests. | ||
| E2E tests validate the CLI binary against a real APISIX instance. They live in `test/e2e/`, use the `//go:build e2e` build tag, and should be written with Ginkgo/Gomega. | ||
|
|
There was a problem hiding this comment.
This section states e2e tests should use Ginkgo/Gomega, but the later “Assertions” section only documents testify. Please clarify the split (e.g., unit tests: testify; e2e specs: gomega/Expect) or update the assertions section to include Gomega examples to avoid confusing contributors.
| Assertion style depends on the test type: | |
| - Unit tests should use the standard `testing` package structure (`func TestXxx(t *testing.T)`) and may use `testify` assertions such as `require.NoError(t, err)` or `assert.Equal(t, expected, actual)`. | |
| - E2E specs in `test/e2e/` should use Ginkgo/Gomega conventions and Gomega assertions such as `Expect(err).NotTo(HaveOccurred())` and `Expect(output).To(ContainSubstring("route"))`. | |
| Do not treat `testify` as the default assertion style for e2e specs; use Gomega `Expect` assertions there so the framework guidance stays consistent. |
| ### Architecture | ||
|
|
||
| The e2e framework: | ||
| 1. Builds the `a6` binary once in `TestMain` (in `setup_test.go`) | ||
| 1. Builds the `a6` binary once per test process | ||
| 2. Waits for APISIX Admin API to become healthy | ||
| 3. Runs tests that invoke the binary via `exec.Command` | ||
| 4. Uses direct Admin API calls for setup/cleanup (not via the CLI) | ||
| 3. Runs specs that invoke the binary via `exec.Command` | ||
| 4. Uses direct Admin API calls only for setup and cleanup | ||
| 5. Organizes scenarios with a suite structure inspired by Kubernetes-style e2e suites | ||
|
|
There was a problem hiding this comment.
The updated e2e strategy (including upstream health checks) now relies on the APISIX Control API as well, but the documented Environment Variables list doesn’t include APISIX_CONTROL_URL (it’s supported by the e2e harness). Please add it to the docs so users know how to override the Control API base URL when running the suite.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/ginkgo_helpers_test.go`:
- Around line 60-74: The four helper functions deleteRouteViaCLIByID,
deleteServiceViaCLIByID, deleteUpstreamViaCLIByID, and
deletePluginConfigViaCLIByID currently discard the error returned by
runA6WithEnv; change each to surface and handle that error instead (e.g., return
error to the caller or call the test fail/assertion helper) so cleanup failures
are not suppressed. Locate runA6WithEnv usage in each function, capture its
error value, and either return it from the helper or call the appropriate
Ginkgo/Gomega fail/assert (so callers can react), and update call sites to
handle the returned error where needed. Ensure log messages include command
context (function name and id) when reporting the error.
- Around line 37-42: The helper deleteResourceViaAdminByID currently only closes
resp.Body when err == nil; change it to always close any non-nil response body
from adminAPI by checking resp != nil and deferring or calling resp.Body.Close()
regardless of err so the body is not leaked (refer to deleteResourceViaAdminByID
and adminAPI response handling: ensure resp != nil then defer resp.Body.Close()
immediately after the call or explicitly close on all return paths).
In `@test/e2e/scenario_coverage_ginkgo_test.go`:
- Around line 176-179: The test handler passed to httptest.NewServer ignores the
error returned by w.Write; update the inline http.HandlerFunc so the write
result is checked (e.g., _, err := w.Write([]byte(name)); if err != nil {
panic(fmt.Errorf("failed to write response: %w", err)) }) to surface failures
during tests; modify the handler inside the httptest.NewServer call accordingly.
- Around line 81-82: The test uses http.Get in polling loops which can block
indefinitely; create a timed HTTP client (client := &http.Client{Timeout: 2 *
time.Second}) and replace the two uses of http.Get(...) with client.Get(...) in
the polling loops (the calls currently using http.Get in the scenario coverage
test), ensuring the client is reused for each request so individual network
stalls will timeout instead of hanging the test.
🪄 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: 621a9714-9409-42d4-b1c3-769d828802e4
📒 Files selected for processing (6)
AGENTS.mdtest/e2e/ginkgo_helpers_test.gotest/e2e/route_ginkgo_test.gotest/e2e/scenario_coverage_ginkgo_test.gotest/e2e/service_ginkgo_test.gotest/e2e/upstream_ginkgo_test.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/service_ginkgo_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- AGENTS.md
- test/e2e/upstream_ginkgo_test.go
- test/e2e/route_ginkgo_test.go
| func deleteResourceViaAdminByID(g Gomega, resourcePath, id string) { | ||
| resp, err := adminAPI("DELETE", resourcePath+"/"+id, nil) | ||
| if err == nil && resp != nil { | ||
| g.Expect(resp.Body.Close()).To(Succeed()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Close Admin API response bodies even on error paths.
On Line 39, resp.Body is closed only when err == nil. If adminAPI returns a non-nil response with an error (e.g., non-2xx), this can leak the body.
Proposed fix
func deleteResourceViaAdminByID(g Gomega, resourcePath, id string) {
resp, err := adminAPI("DELETE", resourcePath+"/"+id, nil)
- if err == nil && resp != nil {
+ if resp != nil {
g.Expect(resp.Body.Close()).To(Succeed())
}
+ g.Expect(err).NotTo(HaveOccurred())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func deleteResourceViaAdminByID(g Gomega, resourcePath, id string) { | |
| resp, err := adminAPI("DELETE", resourcePath+"/"+id, nil) | |
| if err == nil && resp != nil { | |
| g.Expect(resp.Body.Close()).To(Succeed()) | |
| } | |
| } | |
| func deleteResourceViaAdminByID(g Gomega, resourcePath, id string) { | |
| resp, err := adminAPI("DELETE", resourcePath+"/"+id, nil) | |
| if resp != nil { | |
| g.Expect(resp.Body.Close()).To(Succeed()) | |
| } | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/ginkgo_helpers_test.go` around lines 37 - 42, The helper
deleteResourceViaAdminByID currently only closes resp.Body when err == nil;
change it to always close any non-nil response body from adminAPI by checking
resp != nil and deferring or calling resp.Body.Close() regardless of err so the
body is not leaked (refer to deleteResourceViaAdminByID and adminAPI response
handling: ensure resp != nil then defer resp.Body.Close() immediately after the
call or explicitly close on all return paths).
| func deleteRouteViaCLIByID(env []string, id string) { | ||
| _, _, _ = runA6WithEnv(env, "route", "delete", id, "--force") | ||
| } | ||
|
|
||
| func deleteServiceViaCLIByID(env []string, id string) { | ||
| _, _, _ = runA6WithEnv(env, "service", "delete", id, "--force") | ||
| } | ||
|
|
||
| func deleteUpstreamViaCLIByID(env []string, id string) { | ||
| _, _, _ = runA6WithEnv(env, "upstream", "delete", id, "--force") | ||
| } | ||
|
|
||
| func deletePluginConfigViaCLIByID(env []string, id string) { | ||
| _, _, _ = runA6WithEnv(env, "plugin-config", "delete", id, "--force") | ||
| } |
There was a problem hiding this comment.
Do not suppress CLI cleanup errors.
Lines 61/65/69/73 discard cleanup errors entirely. This can mask failed deletions and introduce flaky cross-test interference.
Proposed refactor
-func deleteRouteViaCLIByID(env []string, id string) {
- _, _, _ = runA6WithEnv(env, "route", "delete", id, "--force")
+func deleteRouteViaCLIByID(g Gomega, env []string, id string) {
+ _, stderr, err := runA6WithEnv(env, "route", "delete", id, "--force")
+ g.Expect(err).NotTo(HaveOccurred(), "route cleanup failed: %s", stderr)
}
-func deleteServiceViaCLIByID(env []string, id string) {
- _, _, _ = runA6WithEnv(env, "service", "delete", id, "--force")
+func deleteServiceViaCLIByID(g Gomega, env []string, id string) {
+ _, stderr, err := runA6WithEnv(env, "service", "delete", id, "--force")
+ g.Expect(err).NotTo(HaveOccurred(), "service cleanup failed: %s", stderr)
}
-func deleteUpstreamViaCLIByID(env []string, id string) {
- _, _, _ = runA6WithEnv(env, "upstream", "delete", id, "--force")
+func deleteUpstreamViaCLIByID(g Gomega, env []string, id string) {
+ _, stderr, err := runA6WithEnv(env, "upstream", "delete", id, "--force")
+ g.Expect(err).NotTo(HaveOccurred(), "upstream cleanup failed: %s", stderr)
}
-func deletePluginConfigViaCLIByID(env []string, id string) {
- _, _, _ = runA6WithEnv(env, "plugin-config", "delete", id, "--force")
+func deletePluginConfigViaCLIByID(g Gomega, env []string, id string) {
+ _, stderr, err := runA6WithEnv(env, "plugin-config", "delete", id, "--force")
+ g.Expect(err).NotTo(HaveOccurred(), "plugin-config cleanup failed: %s", stderr)
}As per coding guidelines, **/*.go: Never suppress errors. Always handle and propagate them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/ginkgo_helpers_test.go` around lines 60 - 74, The four helper
functions deleteRouteViaCLIByID, deleteServiceViaCLIByID,
deleteUpstreamViaCLIByID, and deletePluginConfigViaCLIByID currently discard the
error returned by runA6WithEnv; change each to surface and handle that error
instead (e.g., return error to the caller or call the test fail/assertion
helper) so cleanup failures are not suppressed. Locate runA6WithEnv usage in
each function, capture its error value, and either return it from the helper or
call the appropriate Ginkgo/Gomega fail/assert (so callers can react), and
update call sites to handle the returned error where needed. Ensure log messages
include command context (function name and id) when reporting the error.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/scenario_coverage_ginkgo_test.go (2)
83-83:⚠️ Potential issue | 🟠 MajorAdd request timeouts to polling HTTP calls.
Both polling loops still use
http.Getdirectly, so a single stalled request can hang the test even with bounded retries.Proposed fix
@@ - for i := 0; i < 10; i++ { - resp, reqErr := http.Get(gatewayURL + "/scenario-combo") + client := &http.Client{Timeout: 2 * time.Second} + for i := 0; i < 10; i++ { + resp, reqErr := client.Get(gatewayURL + "/scenario-combo") @@ - for i := 0; i < 10; i++ { - resp, reqErr := http.Get(gatewayURL + "/scenario-multi-node") + client := &http.Client{Timeout: 2 * time.Second} + for i := 0; i < 10; i++ { + resp, reqErr := client.Get(gatewayURL + "/scenario-multi-node")Also applies to: 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/scenario_coverage_ginkgo_test.go` at line 83, Replace the raw http.Get calls used in the polling loops (the call producing resp, reqErr when hitting gatewayURL + "/scenario-combo" and the duplicate at the other location) with requests that enforce timeouts: create an http.Client with a sensible Timeout or build each request using context.WithTimeout and use http.NewRequestWithContext, then call client.Do; ensure you handle context/timeouts as errors and close resp.Body in all paths. This change should be applied to both occurrences that reference gatewayURL + "/scenario-combo" so a single stalled request cannot hang the test.
190-193:⚠️ Potential issue | 🟡 MinorDo not suppress
w.Writeerrors in test handler.The handler currently drops the write error, which hides failures.
Proposed fix
func newNamedTestServer(name string) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/plain") - _, _ = w.Write([]byte(name)) + _, err := w.Write([]byte(name)) + Expect(err).NotTo(HaveOccurred()) })) }As per coding guidelines,
**/*.go: Never suppress errors. Always handle and propagate them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/scenario_coverage_ginkgo_test.go` around lines 190 - 193, The handler inside the httptest.NewServer currently discards the result of w.Write; replace the blank identifier with an error check (capture err := w.Write(...)), and handle it instead of suppressing it — e.g., if err != nil { t.Fatalf("failed to write response for %q: %v", name, err) } or otherwise propagate/log/panic appropriately so write failures are not hidden; locate this change in the anonymous http.HandlerFunc used with httptest.NewServer and the call to w.Write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/scenario_coverage_ginkgo_test.go`:
- Around line 104-114: Replace the untyped JSON handling around the local
variable result with a concrete struct that maps the "url" field to string and
"headers" to json.RawMessage (or a typed map[string]string) so you avoid
map[string]interface{} and interface{} usage; unmarshal into that struct, parse
the url string into u as before, and decode the headers RawMessage into a typed
map used by expectHeaderContains when checking the "X-Scenario-Coverage" header.
Update the code paths that reference result["url"], result["headers"], and the
call expectHeaderContains(g, headers["X-Scenario-Coverage"],
"service-route-plugin-config") to use the new struct fields and typed header
value; apply the same replacement for the similar block around lines 196-208.
---
Duplicate comments:
In `@test/e2e/scenario_coverage_ginkgo_test.go`:
- Line 83: Replace the raw http.Get calls used in the polling loops (the call
producing resp, reqErr when hitting gatewayURL + "/scenario-combo" and the
duplicate at the other location) with requests that enforce timeouts: create an
http.Client with a sensible Timeout or build each request using
context.WithTimeout and use http.NewRequestWithContext, then call client.Do;
ensure you handle context/timeouts as errors and close resp.Body in all paths.
This change should be applied to both occurrences that reference gatewayURL +
"/scenario-combo" so a single stalled request cannot hang the test.
- Around line 190-193: The handler inside the httptest.NewServer currently
discards the result of w.Write; replace the blank identifier with an error check
(capture err := w.Write(...)), and handle it instead of suppressing it — e.g.,
if err != nil { t.Fatalf("failed to write response for %q: %v", name, err) } or
otherwise propagate/log/panic appropriately so write failures are not hidden;
locate this change in the anonymous http.HandlerFunc used with
httptest.NewServer and the call to w.Write.
🪄 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: b4338bd2-5b7b-434e-8194-a85b33cbaeac
📒 Files selected for processing (1)
test/e2e/scenario_coverage_ginkgo_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newNamedTestServer(name string) *httptest.Server { | ||
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "text/plain") | ||
| _, err := w.Write([]byte(name)) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| })) |
There was a problem hiding this comment.
newNamedTestServer uses Expect(...) inside the httptest.Server handler goroutine. Gomega/Ginkgo assertions in non-test goroutines should be guarded with defer GinkgoRecover() (or avoid assertions entirely) to prevent panics from bypassing Ginkgo's fail handler and crashing/flaking the suite.
| time.Sleep(3 * time.Second) | ||
| for range 5 { | ||
| resp, reqErr := http.Get(gatewayURL + "/ginkgo-upstream-health/get") | ||
| if reqErr == nil && resp != nil { | ||
| g.Expect(resp.Body.Close()).To(Succeed()) | ||
| } | ||
| time.Sleep(500 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
This probe loop uses http.Get (default client) which has no timeout, so the spec can hang indefinitely if the gateway is slow/unreachable. Prefer an http.Client{Timeout: ...} here (similar to the scenario coverage specs) and treat request errors as retriable to reduce flakiness.
| stdout, stderr, err = runA6WithEnv(env, "service", "list", "--output", "json") | ||
| g.Expect(err).NotTo(HaveOccurred(), "stdout=%s stderr=%s", stdout, stderr) | ||
| g.Expect(json.Valid([]byte(stdout))).To(BeTrue(), stdout) | ||
| g.Expect(stdout).To(ContainSubstring(id1)) | ||
|
|
||
| stdout, stderr, err = runA6WithEnv(env, "service", "list", "--output", "yaml") | ||
| g.Expect(err).NotTo(HaveOccurred(), "stdout=%s stderr=%s", stdout, stderr) | ||
| g.Expect(stdout).To(ContainSubstring("id: " + id1)) |
There was a problem hiding this comment.
In this service list spec you create two services (id1 and id2), but the JSON/YAML assertions only check for id1. This can let regressions slip where non-table output omits entries. Please also assert id2 is present in the unfiltered JSON/YAML outputs (matching the table assertions).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/scenario_coverage_ginkgo_test.go (1)
84-106: Optional: extract shared polling loop to a helper.Both scenarios implement near-identical retry/poll logic; extracting it would reduce duplication and make future tuning safer.
Also applies to: 164-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/scenario_coverage_ginkgo_test.go` around lines 84 - 106, Extract the duplicated polling/retry loop into a helper function (e.g., pollGetUntilOK) that accepts an *http.Client, URL string, maxRetries int and delay time.Duration and returns (body string, status int, err error); replace the inlined loop that uses client := &http.Client{Timeout: 2 * time.Second}, the for i:=0;i<10;i++ retry logic, io.ReadAll(resp.Body)/resp.Body.Close(), lastErr/lastStatus/body handling, and sleeps with a single call to this helper in both places (the block that currently assigns lastErr/lastStatus/body and breaks on http.StatusOK and the similar block at lines 164-188). Ensure the helper closes the response body, returns read errors and non-200 final status as errors, and preserves the existing timeout and retry/delay behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/scenario_coverage_ginkgo_test.go`:
- Around line 84-106: Extract the duplicated polling/retry loop into a helper
function (e.g., pollGetUntilOK) that accepts an *http.Client, URL string,
maxRetries int and delay time.Duration and returns (body string, status int, err
error); replace the inlined loop that uses client := &http.Client{Timeout: 2 *
time.Second}, the for i:=0;i<10;i++ retry logic,
io.ReadAll(resp.Body)/resp.Body.Close(), lastErr/lastStatus/body handling, and
sleeps with a single call to this helper in both places (the block that
currently assigns lastErr/lastStatus/body and breaks on http.StatusOK and the
similar block at lines 164-188). Ensure the helper closes the response body,
returns read errors and non-200 final status as errors, and preserves the
existing timeout and retry/delay behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bcc558e-729f-428d-a3f2-b45a23f0e31d
📒 Files selected for processing (1)
test/e2e/scenario_coverage_ginkgo_test.go
Summary
Validation
Summary by CodeRabbit
New Features
Refactor
Chores