test(e2e): add a7 CLI permutation / combination stability suite#58
Closed
shreemaan-abhishek wants to merge 2 commits into
Closed
test(e2e): add a7 CLI permutation / combination stability suite#58shreemaan-abhishek wants to merge 2 commits into
shreemaan-abhishek wants to merge 2 commits into
Conversation
A new layered combinatorial test that drives the real ./bin/a7 binary
against a live API7 EE 3.9 instance. ~290 cases run end-to-end in
~2 seconds; ginkgo signals overall pass/fail and the suite writes a
paste-able test/e2e/_artifacts/permutation-report.{json,md} artifact
listing every case with its command line, exit code, stdout/stderr
digest, and duration.
Coverage tiers:
1. Help integrity — every parent + every leaf, --help and -h
2. Version / completion — version, completion {bash,zsh,fish,ps1}
3. Context lifecycle — create/list/current/use/delete in isolated A7_CONFIG_DIR
4. Output-format matrix — every read-only verb x {table, json, yaml, invalid}
5. Auth-source precedence — flag vs env vs context-file
6. CRUD round-trip — per-resource walker for service, route, consumer,
ssl (with runtime-generated ECDSA cert), stream-route,
proto, plus dedicated global-rule (upsert via update)
and secret (capability-gap probe)
7. Declarative config — dump -> validate -> diff (clean)
8. Negative / error — missing flag, bad ID, malformed file, wrong token,
wrong server, unsupported declarative sections
9. Debug commands smoke
10. Unsupported commands — assert upstream, consumer-group, service-template,
plugin-config still error as unknown
Implementation notes:
- All resources use the a7-perm-<resource>-<nanoseconds> id prefix; per-case
defers clean up; an AfterSuite sweep backstops any leftover ids.
- Sweep silently skips resource types where listing requires a parent id or
the endpoint is not exposed on this EE build.
- Ginkgo container is Ordered+ContinueOnFailure so a failure in one tier does
not skip the rest.
- Runs via the new `make test-e2e-permutation` target (ginkgo --focus).
Run with:
A7_ADMIN_URL=https://localhost:7443 \
A7_TOKEN=a7ee-xxxx \
A7_GATEWAY_GROUP=default \
make test-e2e-permutation
Adds test/e2e/_artifacts/ to .gitignore so per-run report files do not get
committed.
|
Warning Review limit reached
More reviews will be available in 15 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive end-to-end Ginkgo test suite that exercises the ChangesE2E Permutation Test Suite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/permutation_cleanup_test.go`:
- Line 90: Replace the ignored error from io.ReadAll(resp.Body) with proper
error handling: change occurrences like body, _ := io.ReadAll(resp.Body) to
capture the error (body, err := io.ReadAll(resp.Body)) and on err != nil fail
the test or propagate the error (e.g., t.Fatalf("reading response body failed:
%v", err)) so malformed/partial responses are surfaced; update all similar sites
(the instances at the noted lines) that read resp.Body to follow this pattern
and ensure resp.Body is still closed as currently done.
In `@test/e2e/permutation_matrix_test.go`:
- Around line 597-599: The artifactDir helper currently ignores os.Getwd()
errors; update the artifactDir function to return (string, error) instead of
just string, check the error from os.Getwd() and return a wrapped error (e.g.,
fmt.Errorf("getting working dir: %w", err)) when it fails, and only
construct/return filepath.Join(wd, "_artifacts") on success; then update all
callers of artifactDir (tests in this package) to handle the returned error
(fail the test or propagate) instead of assuming a valid path.
In `@test/e2e/permutation_report_test.go`:
- Around line 56-58: The permutation report is storing raw Args and EnvOverrides
which may include sensitive tokens; create and use a redaction helper (e.g.,
redactSensitive) to sanitize slices before assignment so Args:
redactSensitive(append([]string(nil), args...)) and EnvOverrides:
redactSensitive(append([]string(nil), envOverrides...)); redactSensitive should
detect patterns like "--token", "-token", "A7_TOKEN", "API_KEY", both forms
key=value and separate-arg values and replace the secret portion with a fixed
placeholder (e.g., "<redacted>"); apply the same redaction at the other
occurrences mentioned (around lines 184-187) so no raw tokens are stored or
printed.
In `@test/e2e/permutation_test.go`:
- Around line 103-106: The deferred anonymous function currently swallows errors
from spec.cleanup(id, parentID); change it to capture and handle the returned
error instead of discarding it — e.g., inside the defer replace "_ =
spec.cleanup(id, parentID)" with "if err := spec.cleanup(id, parentID); err !=
nil { t.Fatalf(\"cleanup failed: %v\", err) }" or use your test helper
(require.NoError/require.NoErrorf) to fail the test on cleanup errors; apply the
same fix for the other deferred cleanup sites that call spec.cleanup.
🪄 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: 3d1a77b3-e9f5-4d0d-b3e0-c23cc69e8b28
📒 Files selected for processing (6)
.gitignoreMakefiletest/e2e/permutation_cleanup_test.gotest/e2e/permutation_matrix_test.gotest/e2e/permutation_report_test.gotest/e2e/permutation_test.go
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
| body, _ := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
Handle io.ReadAll errors in cleanup HTTP paths.
These reads currently ignore failures, which can hide malformed/partial responses and break unsupported-endpoint detection or delete diagnostics.
Proposed fix pattern
-body, _ := io.ReadAll(resp.Body)
+body, readErr := io.ReadAll(resp.Body)
+if readErr != nil {
+ return nil, fmt.Errorf("read %s response body: %w", path, readErr)
+}-body, _ := io.ReadAll(resp.Body)
+body, readErr := io.ReadAll(resp.Body)
+if readErr != nil {
+ return fmt.Errorf("read DELETE %s response body: %w", withForce, readErr)
+}As per coding guidelines **/*.go: Never suppress errors. Always handle and propagate errors explicitly.
Also applies to: 106-106, 184-184, 225-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/permutation_cleanup_test.go` at line 90, Replace the ignored error
from io.ReadAll(resp.Body) with proper error handling: change occurrences like
body, _ := io.ReadAll(resp.Body) to capture the error (body, err :=
io.ReadAll(resp.Body)) and on err != nil fail the test or propagate the error
(e.g., t.Fatalf("reading response body failed: %v", err)) so malformed/partial
responses are surfaced; update all similar sites (the instances at the noted
lines) that read resp.Body to follow this pattern and ensure resp.Body is still
closed as currently done.
Comment on lines
+597
to
+599
| wd, _ := os.Getwd() | ||
| // test/e2e is the working dir under ginkgo; resolve to ./_artifacts inside. | ||
| return filepath.Join(wd, "_artifacts") |
There was a problem hiding this comment.
Handle os.Getwd() failure when resolving artifact output path.
artifactDir() currently ignores the os.Getwd() error, which can silently produce an incorrect output location.
Proposed fix
func artifactDir() string {
- wd, _ := os.Getwd()
+ wd, err := os.Getwd()
+ if err != nil {
+ return "_artifacts"
+ }
// test/e2e is the working dir under ginkgo; resolve to ./_artifacts inside.
return filepath.Join(wd, "_artifacts")
}As per coding guidelines **/*.go: Never suppress errors. Always handle and propagate errors explicitly.
📝 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.
Suggested change
| wd, _ := os.Getwd() | |
| // test/e2e is the working dir under ginkgo; resolve to ./_artifacts inside. | |
| return filepath.Join(wd, "_artifacts") | |
| wd, err := os.Getwd() | |
| if err != nil { | |
| return "_artifacts" | |
| } | |
| // test/e2e is the working dir under ginkgo; resolve to ./_artifacts inside. | |
| return filepath.Join(wd, "_artifacts") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/permutation_matrix_test.go` around lines 597 - 599, The artifactDir
helper currently ignores os.Getwd() errors; update the artifactDir function to
return (string, error) instead of just string, check the error from os.Getwd()
and return a wrapped error (e.g., fmt.Errorf("getting working dir: %w", err))
when it fails, and only construct/return filepath.Join(wd, "_artifacts") on
success; then update all callers of artifactDir (tests in this package) to
handle the returned error (fail the test or propagate) instead of assuming a
valid path.
Comment on lines
+56
to
+58
| Args: append([]string(nil), args...), | ||
| EnvOverrides: append([]string(nil), envOverrides...), | ||
| StdoutSHA256: sha256hex(stdout), |
There was a problem hiding this comment.
Redact token-bearing args/env before storing or rendering permutation reports.
The recorder currently stores and prints raw command args/env overrides, which can include real --token and A7_TOKEN values from auth-tier cases.
Proposed fix
res := permResult{
Tier: tier,
TierName: tierName(tier),
Name: name,
- Args: append([]string(nil), args...),
- EnvOverrides: append([]string(nil), envOverrides...),
+ Args: redactArgs(args),
+ EnvOverrides: redactEnvOverrides(envOverrides),
...
}+func redactArgs(args []string) []string {
+ out := append([]string(nil), args...)
+ for i := 0; i < len(out); i++ {
+ switch out[i] {
+ case "--token", "--api-key":
+ if i+1 < len(out) {
+ out[i+1] = "<redacted>"
+ }
+ }
+ }
+ return out
+}
+
+func redactEnvOverrides(env []string) []string {
+ out := make([]string, 0, len(env))
+ for _, kv := range env {
+ if strings.HasPrefix(kv, "A7_TOKEN=") || strings.HasPrefix(kv, "AUTHORIZATION=") || strings.HasPrefix(kv, "X-API-KEY=") || strings.HasPrefix(kv, "COOKIE=") {
+ k := strings.SplitN(kv, "=", 2)[0]
+ out = append(out, k+"=<redacted>")
+ continue
+ }
+ out = append(out, kv)
+ }
+ return out
+}As per coding guidelines **/*.{js,ts,tsx,go}: Do not log, serialize, or return API keys, tokens, or authentication headers without redaction.
Also applies to: 184-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/permutation_report_test.go` around lines 56 - 58, The permutation
report is storing raw Args and EnvOverrides which may include sensitive tokens;
create and use a redaction helper (e.g., redactSensitive) to sanitize slices
before assignment so Args: redactSensitive(append([]string(nil), args...)) and
EnvOverrides: redactSensitive(append([]string(nil), envOverrides...));
redactSensitive should detect patterns like "--token", "-token", "A7_TOKEN",
"API_KEY", both forms key=value and separate-arg values and replace the secret
portion with a fixed placeholder (e.g., "<redacted>"); apply the same redaction
at the other occurrences mentioned (around lines 184-187) so no raw tokens are
stored or printed.
Comment on lines
+103
to
+106
| defer func() { | ||
| if spec.cleanup != nil { | ||
| _ = spec.cleanup(id, parentID) | ||
| } |
There was a problem hiding this comment.
Stop swallowing deferred cleanup errors in tier workflows.
These deferred deletes currently suppress errors. When cleanup fails, the suite can leak resources and make later cases fail nondeterministically.
Proposed fix pattern
defer func() {
if spec.cleanup != nil {
- _ = spec.cleanup(id, parentID)
+ if err := spec.cleanup(id, parentID); err != nil {
+ GinkgoWriter.Printf("cleanup failed for %s/%s: %v\n", spec.name, id, err)
+ }
}
}()-defer func() { _ = deleteGlobalRuleByID(pluginID) }()
+defer func() {
+ if err := deleteGlobalRuleByID(pluginID); err != nil {
+ GinkgoWriter.Printf("cleanup failed for global-rule/%s: %v\n", pluginID, err)
+ }
+}()-defer func() { _ = deleteSecretByID(id) }()
+defer func() {
+ if err := deleteSecretByID(id); err != nil {
+ GinkgoWriter.Printf("cleanup failed for secret/%s: %v\n", id, err)
+ }
+}()As per coding guidelines **/*.go: Never suppress errors. Always handle and propagate errors explicitly.
Also applies to: 291-291, 360-360
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/permutation_test.go` around lines 103 - 106, The deferred anonymous
function currently swallows errors from spec.cleanup(id, parentID); change it to
capture and handle the returned error instead of discarding it — e.g., inside
the defer replace "_ = spec.cleanup(id, parentID)" with "if err :=
spec.cleanup(id, parentID); err != nil { t.Fatalf(\"cleanup failed: %v\", err)
}" or use your test helper (require.NoError/require.NoErrorf) to fail the test
on cleanup errors; apply the same fix for the other deferred cleanup sites that
call spec.cleanup.
…gaps The first CI run on this branch surfaced two problems: 1. The suite ran inside the regular `make test-e2e` target. That target is the existing CI gate; bringing 290+ permutation cases into it both inflates the run time and tied PR merge to the suite reporting clean — which it does not yet, because three real CLI inconsistencies are still open (fixed in PR #56 and PR #57). The permutation matrix is meant to be a manual triage tool, not a CI gate. Fix: tag the Describe with Label("permutation") and update test-e2e / test-e2e-full to skip it via --label-filter='!permutation'. The dedicated test-e2e-permutation target inverts the filter to include only the permutation specs. Net effect: regular CI no longer sees the suite; `make test-e2e-permutation` is unchanged. 2. The CI EE rejected `stream-route create` with `Error: API error (status 400): can not create a Stream Route to the HTTP Service`. Stream routes need an L4 service on that deployment; my local EE accepted them attached to an HTTP service. The walker then cascaded 7 failures (get / list / export / delete all error with "resource not found"). Fix: detect known capability-gap stderr patterns after the create step and downgrade the rest of the walker to a single informational "skipped" record. Patterns cover stream-route on HTTP-only EEs plus the existing secret-provider / vault gaps already handled by local_stability. The walker now also stops short on any create failure (capability gap or otherwise) rather than producing cascade noise from "resource not found" on every follow-up step.
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
1 participant
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
Adds a new layered combinatorial test layer that drives the real `./bin/a7` binary against a live API7 EE 3.9 instance. ~290 cases run end-to-end in roughly 2 seconds; ginkgo signals overall pass/fail and the suite writes a paste-able `test/e2e/_artifacts/permutation-report.{json,md}` artifact listing every case with its command line, exit code, stdout/stderr digest, and duration.
Coverage tiers
How to run
```
A7_ADMIN_URL=https://localhost:7443 \
A7_TOKEN=a7ee-xxxx \
A7_GATEWAY_GROUP=default \
make test-e2e-permutation
```
Optional: set `A7_GATEWAY_URL` to also exercise the `debug trace` smoke.
Findings from the first three runs
Three CLI inconsistencies surfaced. Each is fixed in a separate small PR; once those merge, this suite goes 292/292 green:
Design choices
Test plan
Out of scope
Summary by CodeRabbit