test: migrate runtime coverage toward ginkgo e2e#3
Open
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 33 minutes and 21 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 (8)
📝 WalkthroughWalkthroughThis PR migrates the test suite from mocked HTTP-based unit tests to an E2E framework using Ginkgo v2. Changes include updating Go dependencies, rewriting testing documentation to mandate E2E coverage for API-dependent features, adding Ginkgo test infrastructure, deleting numerous command-level mocked tests, implementing new E2E test cases with real gateway interactions, and updating Makefile and CI workflows to execute E2E tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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.
.github/workflows/e2e.yml
Outdated
| @@ -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 |
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
Release Notes
New Features
Tests
Documentation
Chores