test: add E2E tests for manual APIKey approval and rejection workflows#66
Merged
Merged
Conversation
Signed-off-by: emmaaroche <eroche@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds reusable e2e test helpers for Kubernetes operations and failure diagnostics, refactors the automatic approval test to use those helpers, and introduces a new manual approval/rejection e2e test exercising both approval and denial flows. ChangesManual Approval E2E Testing Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/test_helpers.go (1)
91-99: ⚡ Quick winKeep collecting namespace events even when controller pod lookup fails.
Line 93 and Line 98 return early, so owner/consumer events are never logged if pod discovery fails. That removes key diagnostics exactly when failures occur.
Proposed fix
- podOutput, err := utils.Run(cmd) - if err != nil { - _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to get controller pod name: %s\n", err) - return - } - podNames := utils.GetNonEmptyLines(podOutput) - if len(podNames) == 0 { - _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "No controller pod found\n") - return - } - controllerPodName := podNames[0] + podOutput, err := utils.Run(cmd) + controllerPodName := "" + if err != nil { + _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to get controller pod name: %s\n", err) + } else { + podNames := utils.GetNonEmptyLines(podOutput) + if len(podNames) == 0 { + _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "No controller pod found\n") + } else { + controllerPodName = podNames[0] + } + } - ginkgo.By("Fetching controller manager pod logs") - cmd = exec.Command("kubectl", "logs", controllerPodName, "-n", controllerNamespace) - controllerLogs, err := utils.Run(cmd) - if err == nil { - _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Controller logs:\n%s\n", controllerLogs) - } else { - _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to get Controller logs: %s\n", err) - } + if controllerPodName != "" { + ginkgo.By("Fetching controller manager pod logs") + cmd = exec.Command("kubectl", "logs", controllerPodName, "-n", controllerNamespace) + controllerLogs, err := utils.Run(cmd) + if err == nil { + _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Controller logs:\n%s\n", controllerLogs) + } else { + _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to get Controller logs: %s\n", err) + } + } @@ - ginkgo.By("Fetching controller manager pod description") - cmd = exec.Command("kubectl", "describe", "pod", controllerPodName, "-n", controllerNamespace) - podDescription, err := utils.Run(cmd) - if err == nil { - _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Pod description:\n%s\n", podDescription) - } else { - _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to describe controller pod: %s\n", err) - } + if controllerPodName != "" { + ginkgo.By("Fetching controller manager pod description") + cmd = exec.Command("kubectl", "describe", "pod", controllerPodName, "-n", controllerNamespace) + podDescription, err := utils.Run(cmd) + if err == nil { + _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Pod description:\n%s\n", podDescription) + } else { + _, _ = fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to describe controller pod: %s\n", err) + } + }🤖 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/test_helpers.go` around lines 91 - 99, The current code returns early on controller pod lookup failure (the err check and the len(podNames) == 0 check), which prevents later namespace owner/consumer event collection; remove those two early returns and instead only log the error/notice via ginkgo.GinkgoWriter (using the existing fmt.Fprintf calls) and let execution continue so the namespace event collection runs; keep using utils.GetNonEmptyLines(podOutput) and ensure any later code that assumes podNames handles an empty slice gracefully (e.g., skip pod-specific steps but still collect owner/consumer events).
🤖 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/test_helpers.go`:
- Around line 60-71: CleanupNamespaces currently discards errors from utils.Run
when running "kubectl delete ns ..."; update the CleanupNamespaces function to
capture the output and error returned by utils.Run for each kubectl delete
invocation and, on non-nil error, fail the test with clear context (e.g., call
ginkgo.Fail or use ginkgo.Expect/Fail) including the command, stdout/stderr and
the error; reference the kubectl delete invocations inside CleanupNamespaces and
utils.Run to locate the calls to change.
---
Nitpick comments:
In `@test/e2e/test_helpers.go`:
- Around line 91-99: The current code returns early on controller pod lookup
failure (the err check and the len(podNames) == 0 check), which prevents later
namespace owner/consumer event collection; remove those two early returns and
instead only log the error/notice via ginkgo.GinkgoWriter (using the existing
fmt.Fprintf calls) and let execution continue so the namespace event collection
runs; keep using utils.GetNonEmptyLines(podOutput) and ensure any later code
that assumes podNames handles an empty slice gracefully (e.g., skip pod-specific
steps but still collect owner/consumer events).
🪄 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: d2f60552-da78-426c-b1a2-e32a7f68f384
📒 Files selected for processing (3)
test/e2e/automatic_approval_test.gotest/e2e/manual_workflow_test.gotest/e2e/test_helpers.go
Signed-off-by: emmaaroche <eroche@redhat.com>
eguzki
reviewed
May 25, 2026
Contributor
eguzki
left a comment
eguzki
left a comment
There was a problem hiding this comment.
Looks very good
One minor thing commented. This should be good to go anyway.
| func CleanupNamespaces(ownerNamespace, consumerNamespace, kuadrantNamespace string) { | ||
| ginkgo.By("cleaning up kuadrant namespace") | ||
| cmd := exec.Command("kubectl", "delete", "ns", kuadrantNamespace, "--wait=false") | ||
| output, err := utils.Run(cmd) |
Contributor
There was a problem hiding this comment.
Minor: will fail the test if namespace deletion fails. Maybe namespaces are already deleted or in progress.
Signed-off-by: emmaaroche <eroche@redhat.com>
eguzki
approved these changes
May 26, 2026
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
2 participants
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
Add E2E test coverage for manual APIKey approval and rejection workflows.
Both manual workflow tests verify:
approvalMode: manualPending=TruePending=TrueCloses #61
Changes
New Tests (
test/e2e/manual_workflow_test.go):Approved=TruepropagatesDenied=TruepropagatesRefactoring:
CreateHTTPRoute()- HTTPRoute creationCreateAuthPolicy()- AuthPolicy creation + status patchCleanupNamespaces()- namespace cleanupLogDebugInfoOnFailure()- debug logging on test failureautomatic_approval_test.goto use shared helpersVerification Steps
Expected: 6/6 tests pass including new manual approval and rejection tests.
Summary by CodeRabbit