test: migrate runtime coverage toward ginkgo e2e#3
Open
Conversation
📝 WalkthroughWalkthroughMigrates many command-level mocked tests to Ginkgo v2 E2E tests, updates Go toolchain and CI timeout, adds Ginkgo suite entrypoint, introduces new E2E helpers and polling logic, removes numerous httpmock-based command tests, updates docs to require E2E for API-dependent behavior, and adjusts a few CLI command behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
Note
Copilot was unable to run its full agentic suite in this review.
Migrates the E2E test entrypoint to a Ginkgo-based suite, adds a local-stability spec that exercises real EE/gateway workflows, and enforces the updated testing strategy by removing Admin-API-mocking command tests while fixing CLI behavior mismatches.
Changes:
- Add Ginkgo/Gomega E2E suite + “local stability” specs and supporting E2E helpers/cleanup improvements.
- Remove command-level
httpmock/httptest-based CLI tests per updated testing policy and shift remaining verification to E2E + pure logic tests. - Fix CLI behavior:
secret createsupports positionalprovider/id, andstream-route update -fdoesn’t require--upstream-id.
Reviewed changes
Copilot reviewed 88 out of 89 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/stream_route_test.go | Align stream-route E2E CRUD with service-backed model and stronger cleanup. |
| test/e2e/setup_test.go | Introduce testTB interface + update helpers to accept both *testing.T and Ginkgo GinkgoT(). |
| test/e2e/service_test.go | Improve Admin API cleanup semantics and error surfacing for service deletion. |
| test/e2e/secret_test.go | Generalize secret cleanup helper to accept testTB. |
| test/e2e/local_stability_ginkgo_test.go | Add new Ginkgo Ordered E2E “local stability” spec covering route/service, trace, secret, stream-route flows. |
| test/e2e/e2e_suite_test.go | Add Ginkgo suite entrypoint (RunSpecs). |
| test/e2e/consumer_test.go | Add gateway poll helper and adjust key-auth E2E to use service-backed routes and propagation polling. |
| test/e2e/config_sync_test.go | Add dump sanitization/trim helpers to reduce shared-env flakiness and focus roundtrip on owned resources. |
| pkg/cmd/upstream/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/upstream/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/upstream/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/upstream/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/upstream/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/upstream/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/stream-route/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/stream-route/update/update.go | Allow stream-route update -f without requiring --upstream-id. |
| pkg/cmd/stream-route/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/stream-route/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/stream-route/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/stream-route/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/stream-route/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/ssl/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/service/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/service/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/service/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/service/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/service/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/service/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/secret/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/secret/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/secret/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/secret/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/secret/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/secret/create/create.go | Update secret create to accept positional [provider/id]. |
| pkg/cmd/route/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/route/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/route/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/route/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/route/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/proto/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/proto/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/proto/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/proto/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/proto/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/proto/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-metadata/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-metadata/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-metadata/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-metadata/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-config/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-config/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-config/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-config/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-config/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/plugin-config/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/global-rule/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/global-rule/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/global-rule/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/gateway-group/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/credential/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/credential/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/credential/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/credential/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/credential/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/context/create/create_test.go | Remove httptest-based command coverage per new testing policy. |
| pkg/cmd/consumer/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer-group/update/update_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer-group/list/list_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer-group/get/get_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer-group/export/export_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer-group/delete/delete_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/consumer-group/create/create_test.go | Remove httpmock-based command tests per new testing policy. |
| pkg/cmd/config/sync/sync_test.go | Remove httpmock-based config sync command tests per new testing policy. |
| pkg/cmd/config/diff/diff_test.go | Remove httpmock-based config diff command tests per new testing policy. |
| internal/update/github_test.go | Replace httptest server with stub RoundTripper-based tests. |
| go.mod | Bump Go version and add Ginkgo/Gomega dependencies for E2E. |
| docs/testing-strategy.md | Update testing strategy (Ginkgo E2E, avoid Admin API mocks). |
| Makefile | Run E2E via Ginkgo CLI for consistent local/CI behavior. |
| .github/workflows/e2e.yml | Adjust E2E workflow timeout and module download step. |
Comments suppressed due to low confidence (4)
test/e2e/consumer_test.go:1
- In
waitForGatewayStatus, the response body is closed without draining. In tight polling loops this can prevent connection reuse and cause resource/FD churn, especially in CI. Consider draining toio.Discard(up to a reasonable cap) before closing, and optionally handling theClose()error.
test/e2e/stream_route_test.go:1 deleteStreamRouteViaAdminsilently ignores non-nil errors and does not check for non-2xx status codes, which can hide cleanup failures and lead to resource collisions across E2E runs. It would be more robust to mirrordeleteServiceViaAdmin: fail (or at leastErrorf) on request errors and unexpected status codes, while allowing 404 as a no-op, and always closing the body.
test/e2e/setup_test.go:1- This custom
testTBduplicates a subset oftesting.TBand can drift over time (e.g., missing methods that helpers/libraries may start relying on). Prefer taking atesting.TBdirectly where possible; it already includesHelper,TempDir,Cleanup, and skip/fail methods, and improves interoperability with both*testing.Tand Ginkgo'sGinkgoT().
pkg/cmd/secret/create/create.go:1 - With
cobra.MaximumNArgs(1),secret createaccepts zero args even though theUsestring documents a positional[provider/id]. If the command also supports--id, consider adding anArgsvalidator that enforces 'exactly one of positional arg or --id' and produces a clear, up-to-date error message; otherwise, considercobra.ExactArgs(1)to match the documented usage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -18,6 +18,9 @@ jobs: | |||
| with: | |||
| go-version: '1.22' | |||
There was a problem hiding this comment.
The workflow pins Go 1.22, but go.mod now requires Go 1.23.0. This will fail in CI with a 'go.mod requires go >= 1.23' style error. Update actions/setup-go to use Go 1.23.x (or align go.mod back to 1.22) so CI can build and run E2E.
Suggested change
| go-version: '1.22' | |
| go-version: '1.23' |
| module github.com/api7/a7 | ||
|
|
||
| go 1.22.3 | ||
| go 1.23.0 |
There was a problem hiding this comment.
The module now requires Go 1.23.0, which needs to be consistently reflected across CI (and any pinned local dev tooling). Ensure all workflows and release/build pipelines that use go-version or pinned toolchains are updated to Go 1.23.x to avoid build failures.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/e2e.yml (1)
21-29:⚠️ Potential issue | 🟠 MajorWire A7_GATEWAY_URL and HTTPBIN_URL into the E2E workflow.
The
make test-e2estep (lines 26-28) only providesA7_ADMIN_URL,A7_TOKEN, andA7_GATEWAY_GROUP. However,test/e2e/setup_test.goreferencesA7_GATEWAY_URLandHTTPBIN_URLand skips gateway traffic and httpbin-dependent tests when these are absent. Add these to the env block to ensure full test coverage in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 21 - 29, Add the missing environment variables A7_GATEWAY_URL and HTTPBIN_URL to the "Run E2E tests" step so make test-e2e runs with the same inputs expected by test/e2e/setup_test.go; update the env block that currently sets A7_ADMIN_URL, A7_TOKEN, and A7_GATEWAY_GROUP to also set A7_GATEWAY_URL: ${{ secrets.A7_GATEWAY_URL }} and HTTPBIN_URL: ${{ secrets.HTTPBIN_URL }} (or appropriate secret names) so tests that check A7_GATEWAY_URL and HTTPBIN_URL are not skipped.test/e2e/consumer_test.go (1)
59-64:⚠️ Potential issue | 🟠 MajorKeep consumer cleanup idempotent, not silent.
This makes the second delete in
TestConsumer_CRUDharmless, but it also hides real cleanup failures and leaked consumers. Please special-case not-found only and surface the rest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/consumer_test.go` around lines 59 - 64, The deleteConsumerViaAdmin helper currently swallows errors and only closes resp.Body when err==nil, hiding real failures; update it to always close resp.Body when resp is non-nil, and treat HTTP 404 (not found) as a no-op but surface any other non-success status or err via t.Fatalf/t.Errorf; reference the deleteConsumerViaAdmin function and the runtimeAdminAPI call to locate where to check resp.StatusCode and err, close the response body, and fail the test on unexpected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 3: The project declares Go 1.23.0 in go.mod but the E2E workflow still
uses Go 1.22; update the GitHub Actions e2e workflow's setup step (the action
that uses the go-version/go-version matrix entry) to use 1.23.0 (or 1.23) so the
runner's Go matches the module's required version and avoids failing on go mod
download/run; also check any other workflow steps or matrix entries that pin Go
versions and align them to 1.23.0.
In `@pkg/cmd/secret/create/create.go`:
- Around line 39-42: The RunE handler currently unconditionally overwrites
opts.ID with the positional arg (in the RunE func for the cobra.Command),
allowing conflicting sources like `a7 secret create foo --id bar`; change the
logic to detect both sources and reject or require them to match: if args length
> 0 and opts.ID is non-empty, return an error unless args[0] == opts.ID;
otherwise set opts.ID = args[0] only when opts.ID is empty; ensure this
validation happens early in the RunE before proceeding with creation.
In `@test/e2e/config_sync_test.go`:
- Around line 43-48: The test currently replaces cfg["services"] and
cfg["routes"] with filtered slices that may be empty, masking a roundtrip dump
regression; update the test in config_sync_test.go to check the results of
filterResourcesByID (for the variables/services identified by serviceID and
routeID) and fail fast (use t.Fatalf or t.Fatalf-style failure) if the filtered
slice is empty so the test fails when the target service or route is dropped
rather than silently proceeding with an empty collection; keep using the
existing filterResourcesByID helper and the cfg map keys "services" and "routes"
to locate the values.
In `@test/e2e/consumer_test.go`:
- Around line 21-49: The polling helper waitForGatewayStatus currently returns
lastErr on timeout even if an HTTP response was observed earlier, which lets a
transient transport error override a valid lastStatus; change the final return
logic so that a transport error (lastErr) is only returned when no HTTP status
was ever observed (lastStatus == 0). In practice, update the tail of
waitForGatewayStatus to check lastStatus == 0 && lastErr != nil and return the
transport error only in that case, otherwise return the lastStatus with a nil
error so callers like route_test.go and debug_test.go get the last observed HTTP
code.
In `@test/e2e/local_stability_ginkgo_test.go`:
- Around line 101-104: The test currently treats any error from runA7WithEnv
when creating the secret as a skip, which can hide real regressions; change the
logic in the secret creation call (the stdout, stderr, err := runA7WithEnv(env,
"secret", "create", secretID, "-f", tmpFile, "-g", gatewayGroup) block) so you
only call Skip for known environment-capability failures (e.g., detect specific
error strings or a sentinel error returned by runA7WithEnv) and for all other
errors fail the test (use t.Fatalf or t.Error/t.FailNow) with the captured
stdout/stderr to surface regressions; apply the same change to the analogous
runA7WithEnv error handling in the later secret-update/assertion block.
In `@test/e2e/route_test.go`:
- Around line 26-31: deleteRouteViaAdmin currently swallows all errors from
runtimeAdminAPI which hides real failures; change it to call runtimeAdminAPI and
if it returns an error or a non-2xx response, inspect the response (resp != nil)
and only ignore the expected "not found" case (HTTP 404) — for any other error
or status code, report the failure via the test helper (t.Fatalf or t.Errorf) so
leaked routes fail the test; ensure you always close resp.Body when resp is
non-nil. Reference: function deleteRouteViaAdmin and runtimeAdminAPI.
---
Outside diff comments:
In @.github/workflows/e2e.yml:
- Around line 21-29: Add the missing environment variables A7_GATEWAY_URL and
HTTPBIN_URL to the "Run E2E tests" step so make test-e2e runs with the same
inputs expected by test/e2e/setup_test.go; update the env block that currently
sets A7_ADMIN_URL, A7_TOKEN, and A7_GATEWAY_GROUP to also set A7_GATEWAY_URL:
${{ secrets.A7_GATEWAY_URL }} and HTTPBIN_URL: ${{ secrets.HTTPBIN_URL }} (or
appropriate secret names) so tests that check A7_GATEWAY_URL and HTTPBIN_URL are
not skipped.
In `@test/e2e/consumer_test.go`:
- Around line 59-64: The deleteConsumerViaAdmin helper currently swallows errors
and only closes resp.Body when err==nil, hiding real failures; update it to
always close resp.Body when resp is non-nil, and treat HTTP 404 (not found) as a
no-op but surface any other non-success status or err via t.Fatalf/t.Errorf;
reference the deleteConsumerViaAdmin function and the runtimeAdminAPI call to
locate where to check resp.StatusCode and err, close the response body, and fail
the test on unexpected errors.
🪄 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: 3476bfe5-185e-4b9b-9cf0-13ed5291e5e5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
.github/workflows/e2e.ymlMakefiledocs/testing-strategy.mdgo.modinternal/update/github_test.gopkg/api/client_test.gopkg/cmd/config/diff/diff_test.gopkg/cmd/config/dump/dump_test.gopkg/cmd/config/sync/sync_test.gopkg/cmd/consumer-group/create/create_test.gopkg/cmd/consumer-group/delete/delete_test.gopkg/cmd/consumer-group/export/export_test.gopkg/cmd/consumer-group/get/get_test.gopkg/cmd/consumer-group/list/list_test.gopkg/cmd/consumer-group/update/update_test.gopkg/cmd/consumer/create/create_test.gopkg/cmd/consumer/export/export_test.gopkg/cmd/consumer/list/list_test.gopkg/cmd/context/create/create_test.gopkg/cmd/credential/create/create_test.gopkg/cmd/credential/delete/delete_test.gopkg/cmd/credential/get/get_test.gopkg/cmd/credential/list/list_test.gopkg/cmd/credential/update/update_test.gopkg/cmd/debug/trace/trace_test.gopkg/cmd/gateway-group/list/list_test.gopkg/cmd/global-rule/export/export_test.gopkg/cmd/global-rule/get/get_test.gopkg/cmd/global-rule/list/list_test.gopkg/cmd/plugin-config/create/create_test.gopkg/cmd/plugin-config/delete/delete_test.gopkg/cmd/plugin-config/export/export_test.gopkg/cmd/plugin-config/get/get_test.gopkg/cmd/plugin-config/list/list_test.gopkg/cmd/plugin-config/update/update_test.gopkg/cmd/plugin-metadata/create/create_test.gopkg/cmd/plugin-metadata/delete/delete_test.gopkg/cmd/plugin-metadata/get/get_test.gopkg/cmd/plugin-metadata/update/update_test.gopkg/cmd/plugin/get/get_test.gopkg/cmd/proto/create/create_test.gopkg/cmd/proto/delete/delete_test.gopkg/cmd/proto/export/export_test.gopkg/cmd/proto/get/get_test.gopkg/cmd/proto/list/list_test.gopkg/cmd/proto/update/update_test.gopkg/cmd/route/create/create_test.gopkg/cmd/route/delete/delete_test.gopkg/cmd/route/export/export_test.gopkg/cmd/route/get/get_test.gopkg/cmd/route/list/list_test.gopkg/cmd/route/update/update_test.gopkg/cmd/secret/create/create.gopkg/cmd/secret/create/create_test.gopkg/cmd/secret/delete/delete_test.gopkg/cmd/secret/get/get_test.gopkg/cmd/secret/list/list_test.gopkg/cmd/secret/update/update_test.gopkg/cmd/service/create/create_test.gopkg/cmd/service/delete/delete_test.gopkg/cmd/service/export/export_test.gopkg/cmd/service/get/get_test.gopkg/cmd/service/list/list_test.gopkg/cmd/service/update/update_test.gopkg/cmd/ssl/export/export_test.gopkg/cmd/stream-route/create/create_test.gopkg/cmd/stream-route/delete/delete_test.gopkg/cmd/stream-route/export/export_test.gopkg/cmd/stream-route/get/get_test.gopkg/cmd/stream-route/list/list_test.gopkg/cmd/stream-route/update/update.gopkg/cmd/stream-route/update/update_test.gopkg/cmd/upstream/create/create_test.gopkg/cmd/upstream/delete/delete_test.gopkg/cmd/upstream/export/export_test.gopkg/cmd/upstream/get/get_test.gopkg/cmd/upstream/list/list_test.gopkg/cmd/upstream/update/update_test.gotest/e2e/config_sync_test.gotest/e2e/consumer_test.gotest/e2e/debug_test.gotest/e2e/e2e_suite_test.gotest/e2e/local_stability_ginkgo_test.gotest/e2e/route_test.gotest/e2e/secret_test.gotest/e2e/service_test.gotest/e2e/setup_test.gotest/e2e/stream_route_test.go
💤 Files with no reviewable changes (69)
- pkg/cmd/consumer-group/delete/delete_test.go
- pkg/cmd/credential/list/list_test.go
- pkg/cmd/credential/delete/delete_test.go
- pkg/cmd/secret/get/get_test.go
- pkg/cmd/consumer/export/export_test.go
- pkg/cmd/consumer/create/create_test.go
- pkg/cmd/plugin/get/get_test.go
- pkg/cmd/plugin-metadata/get/get_test.go
- pkg/cmd/upstream/get/get_test.go
- pkg/cmd/upstream/export/export_test.go
- pkg/cmd/route/export/export_test.go
- pkg/cmd/plugin-config/update/update_test.go
- pkg/cmd/upstream/create/create_test.go
- pkg/cmd/secret/update/update_test.go
- pkg/cmd/plugin-metadata/update/update_test.go
- pkg/cmd/global-rule/get/get_test.go
- pkg/cmd/credential/create/create_test.go
- pkg/cmd/route/delete/delete_test.go
- pkg/cmd/proto/update/update_test.go
- pkg/cmd/consumer/list/list_test.go
- pkg/cmd/route/list/list_test.go
- pkg/cmd/service/delete/delete_test.go
- pkg/cmd/credential/update/update_test.go
- pkg/cmd/route/update/update_test.go
- pkg/cmd/config/dump/dump_test.go
- pkg/cmd/secret/create/create_test.go
- pkg/cmd/config/sync/sync_test.go
- pkg/cmd/consumer-group/create/create_test.go
- pkg/cmd/route/create/create_test.go
- pkg/cmd/context/create/create_test.go
- pkg/cmd/plugin-config/export/export_test.go
- pkg/cmd/secret/delete/delete_test.go
- pkg/cmd/consumer-group/export/export_test.go
- pkg/cmd/plugin-config/list/list_test.go
- pkg/cmd/proto/create/create_test.go
- pkg/cmd/secret/list/list_test.go
- pkg/cmd/upstream/delete/delete_test.go
- pkg/cmd/proto/get/get_test.go
- pkg/cmd/consumer-group/get/get_test.go
- pkg/cmd/service/create/create_test.go
- pkg/cmd/stream-route/export/export_test.go
- pkg/cmd/service/get/get_test.go
- pkg/cmd/stream-route/delete/delete_test.go
- pkg/cmd/service/list/list_test.go
- pkg/cmd/proto/export/export_test.go
- pkg/cmd/global-rule/export/export_test.go
- pkg/cmd/gateway-group/list/list_test.go
- pkg/cmd/plugin-config/create/create_test.go
- pkg/cmd/consumer-group/list/list_test.go
- pkg/cmd/upstream/list/list_test.go
- pkg/cmd/plugin-config/delete/delete_test.go
- pkg/cmd/service/export/export_test.go
- pkg/cmd/plugin-metadata/create/create_test.go
- pkg/cmd/proto/list/list_test.go
- pkg/cmd/credential/get/get_test.go
- pkg/cmd/stream-route/create/create_test.go
- pkg/cmd/global-rule/list/list_test.go
- pkg/cmd/upstream/update/update_test.go
- pkg/cmd/ssl/export/export_test.go
- pkg/cmd/stream-route/get/get_test.go
- pkg/cmd/plugin-metadata/delete/delete_test.go
- pkg/cmd/stream-route/update/update_test.go
- pkg/cmd/route/get/get_test.go
- pkg/cmd/service/update/update_test.go
- pkg/cmd/config/diff/diff_test.go
- pkg/cmd/stream-route/list/list_test.go
- pkg/cmd/proto/delete/delete_test.go
- pkg/cmd/plugin-config/get/get_test.go
- pkg/cmd/consumer-group/update/update_test.go
Comment on lines
+101
to
+104
| stdout, stderr, err := runA7WithEnv(env, "secret", "create", secretID, "-f", tmpFile, "-g", gatewayGroup) | ||
| if err != nil { | ||
| Skip(fmt.Sprintf("secret create failed in this environment: stdout=%s stderr=%s", stdout, stderr)) | ||
| } |
There was a problem hiding this comment.
Don't turn CLI regressions into skips.
Both branches currently skip on any runA7WithEnv error. That means the positional secret create flow this PR is trying to lock in can regress without failing, and a broken stream-route setup will hide the update assertion entirely. Please skip only on known environment-capability errors and fail everything else.
Also applies to: 131-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/local_stability_ginkgo_test.go` around lines 101 - 104, The test
currently treats any error from runA7WithEnv when creating the secret as a skip,
which can hide real regressions; change the logic in the secret creation call
(the stdout, stderr, err := runA7WithEnv(env, "secret", "create", secretID,
"-f", tmpFile, "-g", gatewayGroup) block) so you only call Skip for known
environment-capability failures (e.g., detect specific error strings or a
sentinel error returned by runA7WithEnv) and for all other errors fail the test
(use t.Fatalf or t.Error/t.FailNow) with the captured stdout/stderr to surface
regressions; apply the same change to the analogous runA7WithEnv error handling
in the later secret-update/assertion block.
Author
|
Addressed the valid review items in
I did not apply the low-confidence suggestion to replace the custom |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 90 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
+30
| func isKnownSecretCapabilityGap(stdout, stderr string) bool { | ||
| combined := stdout + "\n" + stderr | ||
| for _, needle := range []string{ | ||
| "resource not found", | ||
| "secret provider", | ||
| "vault", | ||
| "not configured", | ||
| "unsupported", | ||
| } { | ||
| if strings.Contains(strings.ToLower(combined), needle) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
isKnownSecretCapabilityGap includes the needle "vault", which is likely to appear in many secret-related error messages (and even echoed IDs like "vault/..."), causing the test to Skip on unexpected failures and masking real regressions. Tighten this matcher (remove "vault" or match more specific phrases) so only known environment capability gaps are skipped.
| // - HTTPBIN_URL: httpbin URL (optional — only needed for live traffic | ||
| // forwarding coverage) | ||
| // | ||
| // Run with: go test -v -tags e2e -count=1 -timeout 10m ./test/e2e/... |
There was a problem hiding this comment.
The header comment still suggests running E2E via go test ... ./test/e2e/..., but the Makefile/CI path now uses the Ginkgo CLI runner. Updating this line to the canonical Ginkgo invocation (or pointing to make test-e2e) would avoid confusion for contributors.
| go run github.com/onsi/ginkgo/v2/ginkgo -r --procs=1 --tags=e2e --timeout=45m ./test/e2e/... | ||
|
|
||
| test-e2e-full: | ||
| go run github.com/onsi/ginkgo/v2/ginkgo -r --procs=1 --tags=e2e --timeout=45m ./test/e2e/... |
There was a problem hiding this comment.
test-e2e and test-e2e-full currently run the exact same Ginkgo command, so the additional target is misleading/unused. Either remove test-e2e-full or differentiate it (e.g., focus/label selection or additional env prechecks) so the names reflect distinct behavior.
Suggested change
| go run github.com/onsi/ginkgo/v2/ginkgo -r --procs=1 --tags=e2e --timeout=45m ./test/e2e/... | |
| @set -e; \ | |
| trap '$(MAKE) docker-down' EXIT; \ | |
| $(MAKE) docker-up; \ | |
| $(MAKE) test-e2e |
Comment on lines
+36
to
+47
| Use: "create [provider/id]", | ||
| Short: "Create a secret provider", | ||
| Args: cobra.NoArgs, | ||
| Args: cobra.MaximumNArgs(1), | ||
| RunE: func(c *cobra.Command, args []string) error { | ||
| if len(args) > 0 { | ||
| if opts.ID != "" && opts.ID != args[0] { | ||
| return fmt.Errorf("positional secret provider id %q conflicts with --id %q", args[0], opts.ID) | ||
| } | ||
| if opts.ID == "" { | ||
| opts.ID = args[0] | ||
| } | ||
| } |
There was a problem hiding this comment.
This change introduces new CLI parsing behavior (positional provider/id with conflict detection vs --id), but there is no unit-level test coverage for it in this package after removing the prior command tests. Since the new E2E coverage may be skipped depending on environment capabilities, consider adding a small pure-logic test for the arg/flag resolution and conflict error path to prevent regressions.
Comment on lines
72
to
+98
| @@ -97,6 +93,14 @@ func actionRun(opts *Options) error { | |||
| } | |||
| return cmdutil.NewExporter(format, opts.IO.Out).Write(json.RawMessage(body)) | |||
| } | |||
| if opts.UpstreamID == "" { | |||
| return fmt.Errorf("--upstream-id is required") | |||
| } | |||
There was a problem hiding this comment.
The upstream-id validation behavior changed so file-based updates no longer require --upstream-id, but the previous unit tests for this command were removed and there is no remaining unit-level coverage for the validation branch. Given the E2E scenario may skip when stream routes are disabled, consider adding a small pure-logic unit test around the file vs flag validation decision to lock in the intended behavior.
Comment on lines
+23
to
+35
| func deleteServiceViaAdmin(t testTB, id string) { | ||
| t.Helper() | ||
| resp, err := runtimeAdminAPI("DELETE", fmt.Sprintf("/apisix/admin/services/%s", id), nil) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| if err != nil { | ||
| t.Fatalf("delete service %s via admin API failed: %v", id, err) | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode == 404 { | ||
| return | ||
| } | ||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| body, _ := io.ReadAll(resp.Body) | ||
| t.Fatalf("delete service %s via admin API returned %d: %s", id, resp.StatusCode, string(body)) |
There was a problem hiding this comment.
deleteServiceViaAdmin assumes resp is non-nil and only defers resp.Body.Close() after checking err. http.Client.Do can return a non-nil resp with a non-nil err (and resp may also be nil on success in some edge cases), so this can leak bodies on error or panic on nil resp. Consider mirroring the other cleanup helpers: if resp != nil { defer resp.Body.Close() } before handling err/status codes.
Comment on lines
+31
to
+42
| ctx, cancel := context.WithDeadline(context.Background(), deadline) | ||
| req = req.WithContext(ctx) | ||
| resp, err := insecureClient.Do(req) | ||
| cancel() | ||
| if err != nil { | ||
| lastErr = err | ||
| time.Sleep(500 * time.Millisecond) | ||
| continue | ||
| } | ||
| lastStatus = resp.StatusCode | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| resp.Body.Close() |
There was a problem hiding this comment.
waitForGatewayStatus calls cancel() immediately after insecureClient.Do returns, before draining/closing resp.Body. Canceling the request context can interrupt body reads and prevent connection reuse, which may cause flaky polling and excess connections. Consider deferring cancel until after io.Copy/io.Discard and resp.Body.Close(), and still ensure cancel happens on the error path.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/gateway_group_test.go (1)
17-24:⚠️ Potential issue | 🟡 Minor
deleteGatewayGroupViaAdminstill silently swallows non-2xx responses and transport errors.Every other admin-cleanup helper in this PR (
deleteRouteViaAdmin,deleteConsumerViaAdmin,deleteSecretViaAdmin,deleteStreamRouteViaAdmin) was tightened to fatal on transport errors and non-2xx (treating404as already-deleted). This one was left as-is, and withTestGatewayGroup_CRUDnow opt-in viaA7_E2E_ENABLE_GATEWAY_GROUP_CRUD=1, a leaked gateway group during that opt-in run will silently poison subsequent runs of the same env without any test signal.♻️ Align with the rest of the E2E cleanup helpers
-// deleteGatewayGroupViaAdmin deletes a gateway group via the control-plane API. -func deleteGatewayGroupViaAdmin(t *testing.T, id string) { - t.Helper() - resp, err := adminAPI("DELETE", fmt.Sprintf("/api/gateway_groups/%s", id), nil) - if err == nil { - resp.Body.Close() - } -} +// deleteGatewayGroupViaAdmin deletes a gateway group via the control-plane API. +func deleteGatewayGroupViaAdmin(t testTB, id string) { + t.Helper() + resp, err := adminAPI("DELETE", fmt.Sprintf("/api/gateway_groups/%s", id), nil) + if resp != nil { + defer resp.Body.Close() + } + if err != nil { + t.Fatalf("delete gateway group %s via admin API failed: %v", id, err) + } + if resp.StatusCode == http.StatusNotFound { + return + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + body, _ := io.ReadAll(resp.Body) + t.Fatalf("delete gateway group %s via admin API returned %d: %s", id, resp.StatusCode, string(body)) + } +}(Requires adding
ioandnet/httpto imports.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/gateway_group_test.go` around lines 17 - 24, The helper deleteGatewayGroupViaAdmin currently swallows transport errors and non-2xx responses; update it to call t.Fatalf on transport errors and on non-2xx responses except treat http.StatusNotFound (404) as already-deleted, and ensure resp.Body is fully read and closed (use io.ReadAll or io.Copy to drain) before checking status; reference the deleteGatewayGroupViaAdmin function and adminAPI call and add imports for io and net/http as needed.
🧹 Nitpick comments (2)
test/e2e/consumer_test.go (1)
31-42: Cancel the per-request context after draining the body, not before.The request is bound to
ctxviareq.WithContext(ctx), so callingcancel()on Line 34 beforeio.Copy(io.Discard, resp.Body)on Line 41 can cause the body read to short-circuit with a context-canceled error. The error is swallowed by_, _ = io.Copy(...), but an incompletely drained body prevents the HTTP client from returning the connection to the idle pool — so polling churns through new TCP/TLS handshakes instead of reusing keep-alive connections.♻️ Proposed fix
- ctx, cancel := context.WithDeadline(context.Background(), deadline) - req = req.WithContext(ctx) - resp, err := insecureClient.Do(req) - cancel() - if err != nil { - lastErr = err - time.Sleep(500 * time.Millisecond) - continue - } - lastStatus = resp.StatusCode - _, _ = io.Copy(io.Discard, resp.Body) - resp.Body.Close() + ctx, cancel := context.WithDeadline(context.Background(), deadline) + req = req.WithContext(ctx) + resp, err := insecureClient.Do(req) + if err != nil { + cancel() + lastErr = err + time.Sleep(500 * time.Millisecond) + continue + } + lastStatus = resp.StatusCode + _, _ = io.Copy(io.Discard, resp.Body) + resp.Body.Close() + cancel()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/consumer_test.go` around lines 31 - 42, The per-request context created with context.WithDeadline and attached via req.WithContext(ctx) is being canceled (cancel()) before the response body is drained and closed, which can cause io.Copy(io.Discard, resp.Body) to short-circuit and prevent connection reuse; move the cancel() call to after the body has been fully read and closed (i.e., perform insecureClient.Do(req), then on success call io.Copy(io.Discard, resp.Body), resp.Body.Close(), and only then call cancel()) so connections return to the idle pool; update the request handling around context.WithDeadline, req.WithContext, cancel(), io.Copy and resp.Body.Close accordingly.Makefile (1)
46-50:test-e2eandtest-e2e-fullhave identical recipes.
docs/testing-strategy.mdpositionstest-e2e-fullas the "full local E2E with gateway/data-plane coverage" variant (requiringA7_GATEWAY_URL/HTTPBIN_URL), but the two targets invoke the exact same Ginkgo command. That makes the target pair purely cosmetic today and, more importantly, removes any mechanical way to keep default CI runs narrower than the full local run — which is the stated intent of "narrow default e2e to control-plane coverage" in the commit history.Consider either:
- differentiating
test-e2e-fullvia a Ginkgo label/focus (e.g., add--label-filter='gateway||dataplane'to-fulland--label-filter='!gateway && !dataplane'to the default), and labeling the relevant specs accordingly, or- documenting explicitly that the two are intentional aliases while runtime gating is done solely via env-var skips (e.g.,
requireGatewayURL,requireHTTPBin,A7_E2E_ENABLE_GATEWAY_GROUP_CRUD).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 46 - 50, The Makefile targets test-e2e and test-e2e-full are identical; update the Makefile so test-e2e runs the narrower control-plane suite and test-e2e-full runs the gateway/dataplane-inclusive suite by adding Ginkgo label filters: change the test-e2e recipe to include --label-filter='!gateway && !dataplane' and change test-e2e-full to include --label-filter='gateway||dataplane' (or, if you prefer the env-var gating approach, add a comment in the Makefile stating they are deliberate aliases and ensure test specs use the existing requireGatewayURL/requireHTTPBin/A7_E2E_ENABLE_GATEWAY_GROUP_CRUD guards); refer to the Makefile targets test-e2e and test-e2e-full and to the Ginkgo flag --label-filter and your gateway/dataplane spec labels when making the change.
🤖 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/config_sync_test.go`:
- Around line 93-104: The helper isKnownRoundtripSyncGap is too permissive
because the needle "not found" can mask real regressions; update the function to
remove the generic "not found" entry and replace it with tighter,
control-plane-specific phrases or stable error codes (e.g., the exact structured
prefixes or error codes your control-plane emits) and/or a precise regex match,
so only the intended capability-gap messages (for example the exact "resource
not found" variant or an identified error code) return true; adjust the needle
list used in isKnownRoundtripSyncGap (and the combined variable usage)
accordingly to only match those specific messages.
- Around line 44-57: The test currently only fatals when
filteredServices/filteredRoutes are empty but skips if cfg["services"] or
cfg["routes"] is missing or not a slice; update the helper around cfg,
filterResourcesByID, serviceID and routeID to first assert the presence and type
of cfg["services"] and cfg["routes"] (e.g., check existence and that they are
[]interface{}), and call t.Fatalf with a clear message when a key is missing or
not the expected slice type before proceeding to filterResourcesByID and the
existing empty checks.
---
Outside diff comments:
In `@test/e2e/gateway_group_test.go`:
- Around line 17-24: The helper deleteGatewayGroupViaAdmin currently swallows
transport errors and non-2xx responses; update it to call t.Fatalf on transport
errors and on non-2xx responses except treat http.StatusNotFound (404) as
already-deleted, and ensure resp.Body is fully read and closed (use io.ReadAll
or io.Copy to drain) before checking status; reference the
deleteGatewayGroupViaAdmin function and adminAPI call and add imports for io and
net/http as needed.
---
Nitpick comments:
In `@Makefile`:
- Around line 46-50: The Makefile targets test-e2e and test-e2e-full are
identical; update the Makefile so test-e2e runs the narrower control-plane suite
and test-e2e-full runs the gateway/dataplane-inclusive suite by adding Ginkgo
label filters: change the test-e2e recipe to include --label-filter='!gateway &&
!dataplane' and change test-e2e-full to include
--label-filter='gateway||dataplane' (or, if you prefer the env-var gating
approach, add a comment in the Makefile stating they are deliberate aliases and
ensure test specs use the existing
requireGatewayURL/requireHTTPBin/A7_E2E_ENABLE_GATEWAY_GROUP_CRUD guards); refer
to the Makefile targets test-e2e and test-e2e-full and to the Ginkgo flag
--label-filter and your gateway/dataplane spec labels when making the change.
In `@test/e2e/consumer_test.go`:
- Around line 31-42: The per-request context created with context.WithDeadline
and attached via req.WithContext(ctx) is being canceled (cancel()) before the
response body is drained and closed, which can cause io.Copy(io.Discard,
resp.Body) to short-circuit and prevent connection reuse; move the cancel() call
to after the body has been fully read and closed (i.e., perform
insecureClient.Do(req), then on success call io.Copy(io.Discard, resp.Body),
resp.Body.Close(), and only then call cancel()) so connections return to the
idle pool; update the request handling around context.WithDeadline,
req.WithContext, cancel(), io.Copy and resp.Body.Close accordingly.
🪄 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: 361dfd57-b68a-46d6-b964-196a58779296
📒 Files selected for processing (12)
.github/workflows/e2e.ymlMakefiledocs/testing-strategy.mdpkg/cmd/secret/create/create.gotest/e2e/config_sync_test.gotest/e2e/consumer_test.gotest/e2e/gateway_group_test.gotest/e2e/local_stability_ginkgo_test.gotest/e2e/route_test.gotest/e2e/secret_test.gotest/e2e/setup_test.gotest/e2e/stream_route_test.go
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/e2e.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/setup_test.go
- test/e2e/local_stability_ginkgo_test.go
Comment on lines
+44
to
+57
| if services, ok := cfg["services"].([]interface{}); ok { | ||
| filteredServices := filterResourcesByID(services, serviceID) | ||
| if len(filteredServices) == 0 { | ||
| t.Fatalf("roundtrip dump is missing expected service %q", serviceID) | ||
| } | ||
| cfg["services"] = filteredServices | ||
| } | ||
| if routes, ok := cfg["routes"].([]interface{}); ok { | ||
| filteredRoutes := filterResourcesByID(routes, routeID) | ||
| if len(filteredRoutes) == 0 { | ||
| t.Fatalf("roundtrip dump is missing expected route %q", routeID) | ||
| } | ||
| cfg["routes"] = filteredRoutes | ||
| } |
There was a problem hiding this comment.
Missing services/routes collection still passes silently.
The fatal-on-empty check only runs when the type assertion succeeds. If a future regression causes config dump to omit the services or routes key entirely (or emit it as nil/non-slice), ok is false and the block is skipped — returning to the same "roundtrip proves nothing" failure mode the previous review round was meant to close. Since this helper is called immediately after a dump that is expected to contain both collections (the test just created them), absence should itself be a hard failure.
♻️ Proposed fix
- if services, ok := cfg["services"].([]interface{}); ok {
- filteredServices := filterResourcesByID(services, serviceID)
- if len(filteredServices) == 0 {
- t.Fatalf("roundtrip dump is missing expected service %q", serviceID)
- }
- cfg["services"] = filteredServices
- }
- if routes, ok := cfg["routes"].([]interface{}); ok {
- filteredRoutes := filterResourcesByID(routes, routeID)
- if len(filteredRoutes) == 0 {
- t.Fatalf("roundtrip dump is missing expected route %q", routeID)
- }
- cfg["routes"] = filteredRoutes
- }
+ services, ok := cfg["services"].([]interface{})
+ require.Truef(t, ok, "roundtrip dump missing services collection, got %T", cfg["services"])
+ filteredServices := filterResourcesByID(services, serviceID)
+ require.NotEmptyf(t, filteredServices, "roundtrip dump is missing expected service %q", serviceID)
+ cfg["services"] = filteredServices
+
+ routes, ok := cfg["routes"].([]interface{})
+ require.Truef(t, ok, "roundtrip dump missing routes collection, got %T", cfg["routes"])
+ filteredRoutes := filterResourcesByID(routes, routeID)
+ require.NotEmptyf(t, filteredRoutes, "roundtrip dump is missing expected route %q", routeID)
+ cfg["routes"] = filteredRoutes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/config_sync_test.go` around lines 44 - 57, The test currently only
fatals when filteredServices/filteredRoutes are empty but skips if
cfg["services"] or cfg["routes"] is missing or not a slice; update the helper
around cfg, filterResourcesByID, serviceID and routeID to first assert the
presence and type of cfg["services"] and cfg["routes"] (e.g., check existence
and that they are []interface{}), and call t.Fatalf with a clear message when a
key is missing or not the expected slice type before proceeding to
filterResourcesByID and the existing empty checks.
Comment on lines
+93
to
+104
| func isKnownRoundtripSyncGap(stdout, stderr string) bool { | ||
| combined := strings.ToLower(stdout + "\n" + stderr) | ||
| for _, needle := range []string{ | ||
| "resource not found", | ||
| "not found", | ||
| } { | ||
| if strings.Contains(combined, needle) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
"not found" is too permissive as a skip gate.
isKnownRoundtripSyncGap treats any message containing "not found" (case-insensitive) in combined stdout/stderr as a known environment gap and skips. This also matches benign-looking but real regressions like "upstream http backend not found", "plugin X not found", or "credential not found" that would indicate a genuine sync bug rather than a shared-env capability gap.
Consider narrowing the match to something tighter, e.g. structured error phrases actually emitted by the control-plane on the capability gap you're working around (ideally a stable prefix or error code), so a future real regression in config sync isn't silently skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/config_sync_test.go` around lines 93 - 104, The helper
isKnownRoundtripSyncGap is too permissive because the needle "not found" can
mask real regressions; update the function to remove the generic "not found"
entry and replace it with tighter, control-plane-specific phrases or stable
error codes (e.g., the exact structured prefixes or error codes your
control-plane emits) and/or a precise regex match, so only the intended
capability-gap messages (for example the exact "resource not found" variant or
an identified error code) return true; adjust the needle list used in
isKnownRoundtripSyncGap (and the combined variable usage) accordingly to only
match those specific messages.
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
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.
Summary
secret createnow accepts the documented positional ID form, andstream-route update -fno longer incorrectly requires--upstream-iddocs/testing-strategy.md,Makefile, and the E2E workflow so local and CI execution use the same Ginkgo-based pathValidation
go test ./...go test -c -tags=e2e ./test/e2eNotes
A7_ADMIN_URL,A7_TOKEN,A7_GATEWAY_URL, andHTTPBIN_URLare not set here.test/e2e/*.gocases while making new and modified E2E coverage Ginkgo-based immediately.Refs #1
Summary by CodeRabbit
New Features
Tests
Documentation
Chores