test: align E2E coverage with current EE models#2
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (1)
📝 WalkthroughWalkthroughA test assertion in the secret creation test file was updated to expect a new, more descriptive validation error string. No other test logic or behavior was modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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
Updates the Go E2E test suite to match the current API7 EE data model where routes reference services via service_id, and improves gateway-facing assertions to be propagation-aware.
Changes:
- Refactors route E2E tests to create a service first and then create routes with
service_id(removing inlineupstreamfrom route fixtures). - Reworks debug trace E2E coverage to use the current route model (
paths+service_id). - Aligns key-auth E2E to create a consumer + credential, and polls the gateway until expected status codes appear.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/e2e/route_test.go | Rewrites route CRUD/export/label/traffic tests to use service-backed routes and adds propagation-aware gateway polling. |
| test/e2e/debug_test.go | Introduces a helper to create traceable routes using service_id + paths, updating all debug trace tests accordingly. |
| test/e2e/consumer_test.go | Adds a gateway status polling helper and updates key-auth E2E to use consumer + credential creation and propagation-aware assertions. |
Comments suppressed due to low confidence (1)
test/e2e/route_test.go:49
createTestRouteViaCLIis currently unused within this file (no call sites) and duplicates the existing shared helpercreateTestRouteWithServiceViaCLIinsetup_test.go. Consider removing this dead code, or reusing/adjusting the shared helper to avoid divergent route fixture formats across the E2E suite.
func createTestRouteViaCLI(t *testing.T, env []string, id, serviceID string) string {
t.Helper()
routeJSON := fmt.Sprintf(`{
"id": %q,
"name": "e2e-route-%s",
"service_id": %q,
"paths": ["/test-%s"],
"methods": ["GET"]
}`, id, id, serviceID, id)
tmpFile := filepath.Join(t.TempDir(), "route.json")
require.NoError(t, os.WriteFile(tmpFile, []byte(routeJSON), 0644))
stdout, stderr, err := runA7WithEnv(env, "route", "create", "-f", tmpFile, "-g", gatewayGroup)
require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr)
return id
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func waitForGatewayStatus(url string, buildRequest func() (*http.Request, error), want func(int) bool, timeout time.Duration) (int, error) { | ||
| deadline := time.Now().Add(timeout) | ||
| lastStatus := 0 | ||
| var lastErr error | ||
| for time.Now().Before(deadline) { | ||
| req, err := buildRequest() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| resp, err := insecureClient.Do(req) | ||
| if err != nil { | ||
| lastErr = err | ||
| time.Sleep(500 * time.Millisecond) | ||
| continue | ||
| } | ||
| lastStatus = resp.StatusCode | ||
| resp.Body.Close() | ||
| if want(resp.StatusCode) { | ||
| return resp.StatusCode, nil | ||
| } | ||
| time.Sleep(500 * time.Millisecond) | ||
| } | ||
| if lastErr != nil { | ||
| return lastStatus, lastErr | ||
| } | ||
| return lastStatus, nil | ||
| } |
There was a problem hiding this comment.
waitForGatewayStatus is now used across multiple test files (e.g., route and consumer tests), but it’s defined in consumer_test.go, which creates a non-obvious cross-file dependency. Consider moving it to the shared test utilities (e.g., setup_test.go) and either remove the unused url parameter or use it (callers currently pass it but it’s ignored).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/debug_test.go (1)
32-51:⚠️ Potential issue | 🟠 MajorAdd a sync check before running
debug traceto prevent race conditions on route propagation.The
debug tracecommand atpkg/cmd/debug/trace/trace.gomakes a single unretried HTTP request with a 15-second timeout and does not include retry logic. These tests create routes and immediately invokedebug tracewithout waiting for the route to reach the data plane first. Other live-gateway tests in this PR usewaitForGatewayStatusto poll for readiness. Add the same synchronization here (either by reusingwaitForGatewayStatusor a minimal check against the gateway URL) before callingdebug traceto avoid transient 404s during route sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/debug_test.go` around lines 32 - 51, The test TestDebugTrace_JSONOutput must wait for the route to be propagated before invoking debug trace; add a synchronization call (reuse waitForGatewayStatus used in other tests) after createDebugTraceRoute and before calling runA7WithEnv so the data plane has the route available (use gatewayGroup and gatewayURL as arguments to waitForGatewayStatus or the minimal gateway URL check your test suite provides) to avoid transient 404s when runA7WithEnv executes the debug trace.
🤖 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/consumer_test.go`:
- Around line 17-42: The loop in waitForGatewayStatus can block on
insecureClient.Do because buildRequest returns a request without a bound
context; update waitForGatewayStatus to create a per-iteration context tied to
the remaining deadline (use ctx, cancel :=
context.WithDeadline(context.Background(), deadline) or
context.WithTimeout(context.Background(), time.Until(deadline))), attach it to
the request via req = req.WithContext(ctx) before calling insecureClient.Do,
defer cancel() (or call cancel after the request completes) to avoid leaks, and
ensure you still handle lastErr, resp.Body.Close(), and the
want(resp.StatusCode) check as before.
In `@test/e2e/route_test.go`:
- Around line 75-80: The cleanup currently calls deleteServiceViaAdmin(t, svcID)
but that helper silently ignores non-2xx responses elsewhere, which can leak a
fixed svcID and break subsequent runs; update the service cleanup to assert
success (or allow only 404) by changing deleteServiceViaAdmin to return an
error/status and making the t.Cleanup caller (or the helper itself) fail the
test on unexpected responses, or modify the helper to treat any non-2xx other
than 404 as a test failure so createTestServiceViaCLI can safely reuse fixed
IDs; locate and update the deleteServiceViaAdmin helper and the t.Cleanup
invocation around createTestServiceViaCLI to enforce this behavior.
---
Outside diff comments:
In `@test/e2e/debug_test.go`:
- Around line 32-51: The test TestDebugTrace_JSONOutput must wait for the route
to be propagated before invoking debug trace; add a synchronization call (reuse
waitForGatewayStatus used in other tests) after createDebugTraceRoute and before
calling runA7WithEnv so the data plane has the route available (use gatewayGroup
and gatewayURL as arguments to waitForGatewayStatus or the minimal gateway URL
check your test suite provides) to avoid transient 404s when runA7WithEnv
executes the debug trace.
🪄 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: 4060788d-c895-49eb-b178-90ef239f1d9c
📒 Files selected for processing (3)
test/e2e/consumer_test.gotest/e2e/debug_test.gotest/e2e/route_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/config_sync_test.go`:
- Around line 146-152: The test removes secrets via sanitizeDumpForSync but then
calls runA7WithEnv to execute the "config sync" command (in the test's call
site) without disabling deletions; update the test to call runA7WithEnv with the
sync flag that disables deletions (e.g., add "--delete=false" or equivalent) so
remote secrets not present in the sanitized dump are not removed, referencing
the sanitizeDumpForSync and runA7WithEnv call sites and the "config sync"
invocation to locate where to add the flag.
In `@test/e2e/consumer_test.go`:
- Around line 34-49: The polling helper currently leaves lastErr set when a
transient transport error occurs, making that error sticky; modify the loop so
that when a request succeeds (err == nil) you clear lastErr (set lastErr = nil)
before assigning lastStatus and closing resp.Body, ensuring subsequent
successful HTTP statuses (checked via want(resp.StatusCode)) aren't shadowed by
an earlier transport error; update the code around the resp handling (variables
lastErr, lastStatus, resp, want) to clear lastErr on success.
🪄 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: d56fb339-4b02-47a0-8461-ebcbe18552b5
📒 Files selected for processing (5)
test/e2e/config_sync_test.gotest/e2e/consumer_test.gotest/e2e/debug_test.gotest/e2e/route_test.gotest/e2e/service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/debug_test.go
| if err != nil { | ||
| lastErr = err | ||
| time.Sleep(500 * time.Millisecond) | ||
| continue | ||
| } | ||
| lastStatus = resp.StatusCode | ||
| resp.Body.Close() | ||
| if want(resp.StatusCode) { | ||
| return resp.StatusCode, nil | ||
| } | ||
| time.Sleep(500 * time.Millisecond) | ||
| } | ||
| if lastErr != nil { | ||
| return lastStatus, lastErr | ||
| } | ||
| return lastStatus, nil |
There was a problem hiding this comment.
Clear lastErr after a successful poll.
Line 35 makes transient transport failures sticky for the rest of the wait. If the gateway briefly refuses connections and then only returns 404 until timeout, this helper returns that old error, so the callers fail before reaching their status == 404 skip path.
🛠 Suggested fix
resp, err := insecureClient.Do(req)
cancel()
if err != nil {
lastErr = err
time.Sleep(500 * time.Millisecond)
continue
}
+ lastErr = nil
lastStatus = resp.StatusCode
resp.Body.Close()📝 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.
| if err != nil { | |
| lastErr = err | |
| time.Sleep(500 * time.Millisecond) | |
| continue | |
| } | |
| lastStatus = resp.StatusCode | |
| resp.Body.Close() | |
| if want(resp.StatusCode) { | |
| return resp.StatusCode, nil | |
| } | |
| time.Sleep(500 * time.Millisecond) | |
| } | |
| if lastErr != nil { | |
| return lastStatus, lastErr | |
| } | |
| return lastStatus, nil | |
| if err != nil { | |
| lastErr = err | |
| time.Sleep(500 * time.Millisecond) | |
| continue | |
| } | |
| lastErr = nil | |
| lastStatus = resp.StatusCode | |
| resp.Body.Close() | |
| if want(resp.StatusCode) { | |
| return resp.StatusCode, nil | |
| } | |
| time.Sleep(500 * time.Millisecond) | |
| } | |
| if lastErr != nil { | |
| return lastStatus, lastErr | |
| } | |
| return lastStatus, nil |
🤖 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 34 - 49, The polling helper currently
leaves lastErr set when a transient transport error occurs, making that error
sticky; modify the loop so that when a request succeeds (err == nil) you clear
lastErr (set lastErr = nil) before assigning lastStatus and closing resp.Body,
ensuring subsequent successful HTTP statuses (checked via want(resp.StatusCode))
aren't shadowed by an earlier transport error; update the code around the resp
handling (variables lastErr, lastStatus, resp, want) to clear lastErr on
success.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/stream_route_test.go (1)
47-48: Consider unique test IDs to reduce cross-run collisions.Lines 47-48 use fixed IDs. If a prior run leaves stale resources (or tests run concurrently), this can cause nondeterministic create failures.
Optional hardening
+import "time" + - svcID := "e2e-stream-route-svc" - srID := "e2e-stream-route-crud" + suffix := fmt.Sprintf("%d", time.Now().UnixNano()) + svcID := "e2e-stream-route-svc-" + suffix + srID := "e2e-stream-route-crud-" + suffix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/stream_route_test.go` around lines 47 - 48, Replace the fixed test IDs svcID and srID in test/e2e/stream_route_test.go with uniquely generated values to avoid collisions (e.g., append a timestamp, test name, or random/UUID suffix); update all uses of svcID and srID in the test to reference the new unique values (for example derive a suffix via time.Now().UnixNano() or uuid.NewString() or t.Name()) so each test run gets distinct resource names and prevents nondeterministic create failures.
🤖 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/stream_route_test.go`:
- Around line 49-52: The cleanup currently hides failures from
deleteStreamRouteViaAdmin; change the helper to return an error instead of
swallowing transport/status errors (update deleteStreamRouteViaAdmin signature
to return error and propagate underlying HTTP/transport error details), then
modify the t.Cleanup closure in the test to call the new
deleteStreamRouteViaAdmin(t, srID) and check/handle its returned error (e.g.,
call t.Fatalf or t.Errorf with the error details) before calling
deleteServiceViaAdmin(t, svcID) so route-delete failures are surfaced and not
masked by service-delete errors.
---
Nitpick comments:
In `@test/e2e/stream_route_test.go`:
- Around line 47-48: Replace the fixed test IDs svcID and srID in
test/e2e/stream_route_test.go with uniquely generated values to avoid collisions
(e.g., append a timestamp, test name, or random/UUID suffix); update all uses of
svcID and srID in the test to reference the new unique values (for example
derive a suffix via time.Now().UnixNano() or uuid.NewString() or t.Name()) so
each test run gets distinct resource names and prevents nondeterministic create
failures.
🪄 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: 2f3f852a-75af-43e7-b2ba-ee1e86b9ddc3
📒 Files selected for processing (2)
test/e2e/config_sync_test.gotest/e2e/stream_route_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/config_sync_test.go
| t.Cleanup(func() { | ||
| deleteStreamRouteViaAdmin(t, srID) | ||
| deleteServiceViaAdmin(t, svcID) | ||
| }) |
There was a problem hiding this comment.
Cleanup currently masks stream-route deletion failures.
Line 50 calls deleteStreamRouteViaAdmin, but that helper swallows transport/status errors. With the new service dependency, hidden route-delete failures make cleanup diagnosis harder and can surface as misleading service-delete failures.
Suggested fix (tighten helper failure handling)
+import "io"
+
func deleteStreamRouteViaAdmin(t *testing.T, id string) {
t.Helper()
resp, err := runtimeAdminAPI("DELETE", fmt.Sprintf("/apisix/admin/stream_routes/%s", id), nil)
- if err == nil {
- resp.Body.Close()
- }
+ if err != nil {
+ t.Fatalf("delete stream route %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 stream route %s via admin API returned %d: %s", id, resp.StatusCode, string(body))
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/stream_route_test.go` around lines 49 - 52, The cleanup currently
hides failures from deleteStreamRouteViaAdmin; change the helper to return an
error instead of swallowing transport/status errors (update
deleteStreamRouteViaAdmin signature to return error and propagate underlying
HTTP/transport error details), then modify the t.Cleanup closure in the test to
call the new deleteStreamRouteViaAdmin(t, srID) and check/handle its returned
error (e.g., call t.Fatalf or t.Errorf with the error details) before calling
deleteServiceViaAdmin(t, svcID) so route-delete failures are surfaced and not
masked by service-delete errors.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func deleteServiceViaAdmin(t *testing.T, 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 now calls t.Fatalf on Admin API errors / non-2xx responses. Since this function is primarily used from t.Cleanup, a transient Admin API issue (or an already-deleted resource) will fail the whole test run during cleanup. This is inconsistent with other delete*ViaAdmin helpers in this package, which treat cleanup as best-effort. Consider returning silently on errors (or using t.Logf) and only enforcing strict status checks in the main test flow, not in cleanup.
| // waitForGatewayStatus polls the gateway until the desired status is observed | ||
| // or the timeout expires. Each request is bound to the remaining deadline so a | ||
| // stalled HTTP call cannot outlive the caller-provided timeout. | ||
| func waitForGatewayStatus(url string, buildRequest func() (*http.Request, error), want func(int) bool, timeout time.Duration) (int, error) { | ||
| deadline := time.Now().Add(timeout) | ||
| lastStatus := 0 |
There was a problem hiding this comment.
waitForGatewayStatus takes a url parameter but never uses it (all requests come from buildRequest). This is misleading for callers and makes it easier to pass inconsistent values. Consider removing the url parameter, or using it inside the helper (e.g., as the default request URL and/or in a timeout error message).
| // waitForGatewayStatus polls the gateway until the desired status is observed | ||
| // or the timeout expires. Each request is bound to the remaining deadline so a | ||
| // stalled HTTP call cannot outlive the caller-provided timeout. | ||
| func waitForGatewayStatus(url string, buildRequest func() (*http.Request, error), want func(int) bool, timeout time.Duration) (int, error) { | ||
| deadline := time.Now().Add(timeout) | ||
| lastStatus := 0 | ||
| var lastErr error | ||
| for time.Now().Before(deadline) { | ||
| req, err := buildRequest() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| 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 | ||
| resp.Body.Close() | ||
| if want(resp.StatusCode) { | ||
| return resp.StatusCode, nil | ||
| } | ||
| time.Sleep(500 * time.Millisecond) | ||
| } | ||
| if lastErr != nil { | ||
| return lastStatus, lastErr | ||
| } | ||
| return lastStatus, nil | ||
| } |
There was a problem hiding this comment.
This polling helper is defined in consumer_test.go but is used by other test files (e.g. route/debug tests). That cross-file dependency is easy to miss and can cause accidental breakage when refactoring tests. Consider moving waitForGatewayStatus to a shared test helper file (e.g. setup_test.go or a new helpers_test.go) alongside other shared E2E utilities.
| username := "e2e-consumer-keyauth" | ||
| svcID := "e2e-service-keyauth" | ||
| routeID := "e2e-route-keyauth" | ||
| credID := "e2e-cred-keyauth" | ||
| t.Cleanup(func() { | ||
| deleteRouteViaAdmin(t, routeID) | ||
| deleteServiceViaAdmin(t, svcID) | ||
| deleteConsumerViaAdmin(t, username) | ||
| }) | ||
|
|
||
| // Create consumer (no auth plugins — API7 EE requires credentials). | ||
| createTestConsumerViaCLI(t, env, username) | ||
| createTestServiceViaCLI(t, env, svcID) | ||
|
|
||
| // Create credential with key-auth plugin. | ||
| credJSON := fmt.Sprintf(`{ | ||
| "name": %q, | ||
| "plugins": { | ||
| "key-auth": { | ||
| "key": "e2e-key-%s" | ||
| } | ||
| } | ||
| }`, username) | ||
| }`, credID, username) | ||
| credFile := filepath.Join(t.TempDir(), "credential.json") | ||
| require.NoError(t, os.WriteFile(credFile, []byte(credJSON), 0644)) | ||
| _, stderr, err := runA7WithEnv(env, "credential", "create", credID, | ||
| stdout, stderr, err := runA7WithEnv(env, "credential", "create", credID, | ||
| "--consumer", username, "-f", credFile, "-g", gatewayGroup) | ||
| if err != nil { | ||
| t.Skipf("credential create failed: %s", stderr) | ||
| } | ||
| require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) |
There was a problem hiding this comment.
TestConsumer_WithKeyAuth creates a credential (credential create) but cleanup only deletes the route/service/consumer. If credentials are not automatically deleted when the consumer is deleted, this will leak test data in shared EE environments. Consider adding explicit credential cleanup (via a7 credential delete ... --consumer <username> --force) in the t.Cleanup block.
| func sanitizeDumpForSync(t *testing.T, file string) { | ||
| t.Helper() | ||
| data, err := os.ReadFile(file) | ||
| require.NoError(t, err) | ||
|
|
||
| var cfg map[string]interface{} | ||
| require.NoError(t, yaml.Unmarshal(data, &cfg)) | ||
| delete(cfg, "secrets") | ||
|
|
||
| updated, err := yaml.Marshal(cfg) | ||
| require.NoError(t, err) | ||
| require.NoError(t, os.WriteFile(file, updated, 0644)) | ||
| } | ||
|
|
There was a problem hiding this comment.
sanitizeDumpForSync is introduced but not used anywhere in this test file. If it’s no longer needed, consider removing it to avoid dead code; if it is intended for the roundtrip sync flow, consider calling it (or folding it into trimDumpForRoundtrip).
| func sanitizeDumpForSync(t *testing.T, file string) { | |
| t.Helper() | |
| data, err := os.ReadFile(file) | |
| require.NoError(t, err) | |
| var cfg map[string]interface{} | |
| require.NoError(t, yaml.Unmarshal(data, &cfg)) | |
| delete(cfg, "secrets") | |
| updated, err := yaml.Marshal(cfg) | |
| require.NoError(t, err) | |
| require.NoError(t, os.WriteFile(file, updated, 0644)) | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| trimDumpForRoundtrip(t, dumpFile, svcID, routeID) | ||
|
|
||
| stdout, stderr, err = runA7WithEnv(env, "config", "sync", "-f", dumpFile, "-g", gatewayGroup) | ||
| stdout, stderr, err := runA7WithEnv(env, "config", "diff", "-f", dumpFile, "-g", gatewayGroup) | ||
| require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) | ||
|
|
||
| stdout, stderr, err = runA7WithEnv(env, "config", "sync", "-f", dumpFile, "--delete=false", "-g", gatewayGroup) |
There was a problem hiding this comment.
config diff exits with an error when differences are found (see pkg/cmd/config/diff/diff.go), so running it after trimDumpForRoundtrip will almost always fail because the trimmed file intentionally omits other remote resources (which show up as deletes). Consider moving the diff call back to before trimming (diffing the unmodified dump against remote), or explicitly tolerating the expected "differences found" error when diffing the trimmed file.
Summary
Validation
FAIL github.com/api7/a7/test/e2e 0.867s
FAIL
FAIL github.com/api7/a7/test/e2e 0.609s
FAIL
Refs #1
Summary by CodeRabbit