Added e2e test cases from openshift test private.#160
Open
siddhibhor-56 wants to merge 2 commits into
Open
Conversation
WalkthroughAdds a new Ginkgo E2E test suite ( ChangesE2E Provider Test Suite with AWS and Generator Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🧹 Nitpick comments (2)
test/e2e/helpers_test.go (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the typed phase constant over the string literal.
Comparing
ns.Status.Phaseagainst the literal"Terminating"works but is fragile to typos and refactors. Use thecorev1.NamespaceTerminatingconstant fromk8s.io/api/core/v1.♻️ Proposed change
- if ns.Status.Phase != "Terminating" { + if ns.Status.Phase != corev1.NamespaceTerminating { return nil }Ensure
corev1 "k8s.io/api/core/v1"is imported.🤖 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/helpers_test.go` at line 75, The namespace phase check in the helper test is using a fragile string literal instead of the typed Kubernetes constant. Update the comparison in the namespace status assertion to use corev1.NamespaceTerminating from k8s.io/api/core/v1, and ensure the test file imports corev1 so the phase check stays consistent with the API types.test/e2e/provider_e2e_test.go (1)
273-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Fail()instead ofExpect(false).To(BeTrue())for the missing-condition case.
g.Expect(false).To(BeTrue(), ...)works but is less idiomatic;Fail("expected Ready condition with ownership error")(orStopTrying) reads more clearly when the conditions loop finds noReadycondition.🤖 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/provider_e2e_test.go` around lines 273 - 289, The missing-condition fallback in the ownership-error check is using a non-idiomatic false expectation; update the `Eventually` block in `provider_e2e_test.go` so that the `Ready` condition search in the `conds` loop uses `Fail("expected Ready condition with ownership error")` (or `StopTrying`) when no matching condition is found, rather than `g.Expect(false).To(BeTrue(), ...)`.
🤖 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/provider_e2e_test.go`:
- Around line 91-101: Add meaningful failure messages to the suite-setup
assertions in provider_e2e_test.go so Ginkgo failures are easier to diagnose.
Update the key Expect(...).To(Succeed()) calls around namespace creation,
VerifyPodsReadyByPrefix, and ensureExternalSecretsConfigReady in the setup flow
to include short, specific messages. Apply the same pattern consistently to
other resource-creation and readiness checks in this file, using the surrounding
helpers and setup steps as anchors.
- Around line 592-593: The AWS credential copy step is discarding the result of
CopyAWSCredsToNamespace, which can hide setup failures and cause later
SecretStore/PushSecret checks to fail confusingly. Update the provider_e2e_test
flow to assert that CopyAWSCredsToNamespace succeeds, matching the earlier AWS
Parameter Store setup pattern, and keep the failure close to the credential-copy
step for clearer test diagnostics.
---
Nitpick comments:
In `@test/e2e/helpers_test.go`:
- Line 75: The namespace phase check in the helper test is using a fragile
string literal instead of the typed Kubernetes constant. Update the comparison
in the namespace status assertion to use corev1.NamespaceTerminating from
k8s.io/api/core/v1, and ensure the test file imports corev1 so the phase check
stays consistent with the API types.
In `@test/e2e/provider_e2e_test.go`:
- Around line 273-289: The missing-condition fallback in the ownership-error
check is using a non-idiomatic false expectation; update the `Eventually` block
in `provider_e2e_test.go` so that the `Ready` condition search in the `conds`
loop uses `Fail("expected Ready condition with ownership error")` (or
`StopTrying`) when no matching condition is found, rather than
`g.Expect(false).To(BeTrue(), ...)`.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70d9ba79-663d-4969-821c-d2d4ca8627c2
⛔ Files ignored due to path filters (76)
test/go.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go/service/ssm/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/ssm/waiters.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/PATENTSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/client.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/socks.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/direct.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/per_host.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/socks5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/upgrade.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/transport.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/third_party/forked/golang/netutil/addr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/errorstream.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/fallback.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/reader.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/remotecommand.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/resize.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v1.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v2.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v3.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v4.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/websocket.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/spdy/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/websocket/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/util/exec/exec.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
test/e2e/helpers_test.gotest/e2e/provider_e2e_test.gotest/go.modtest/utils/aws_provider_resources.gotest/utils/generator_resources.go
Comment on lines
+91
to
+101
| got, err := providerClientset.CoreV1().Namespaces().Create(ctx, namespace, metav1.CreateOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| providerTestNamespace = got.GetName() | ||
|
|
||
| By("Waiting for operator pod to be ready") | ||
| Expect(utils.VerifyPodsReadyByPrefix(ctx, providerClientset, operatorNamespace, []string{ | ||
| operatorPodPrefix, | ||
| })).To(Succeed()) | ||
|
|
||
| By("Ensuring ExternalSecretsConfig cluster CR exists and is Ready") | ||
| Expect(ensureExternalSecretsConfigReady(ctx)).To(Succeed()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add failure messages to key suite-setup assertions.
Several setup Expect(...).To(Succeed()) calls (e.g., namespace creation, operator/operand readiness, ensureExternalSecretsConfigReady) lack messages, making BeforeAll/BeforeEach failures harder to triage. Adding short messages like "failed to create provider test namespace" improves diagnosability. This pattern recurs throughout the file (e.g., resource creation and readiness waits).
As per coding guidelines: "Flag Ginkgo test assertions that lack meaningful failure messages."
🤖 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/provider_e2e_test.go` around lines 91 - 101, Add meaningful failure
messages to the suite-setup assertions in provider_e2e_test.go so Ginkgo
failures are easier to diagnose. Update the key Expect(...).To(Succeed()) calls
around namespace creation, VerifyPodsReadyByPrefix, and
ensureExternalSecretsConfigReady in the setup flow to include short, specific
messages. Apply the same pattern consistently to other resource-creation and
readiness checks in this file, using the surrounding helpers and setup steps as
anchors.
Source: Coding guidelines
Comment on lines
+592
to
+593
| By("Copying AWS credentials") | ||
| _ = utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Swallowed error on credential copy may surface as a confusing downstream failure.
Unlike the AWS Parameter Store context (line 131) which asserts success, here the result of CopyAWSCredsToNamespace is discarded. If the copy fails, the SecretStore/PushSecret steps later in this test will fail with a less obvious error. Prefer asserting success.
🛡️ Proposed fix
- By("Copying AWS credentials")
- _ = utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName)
+ By("Copying AWS credentials")
+ Expect(utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName)).
+ To(Succeed(), "failed to copy AWS credentials into test namespace")📝 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
| By("Copying AWS credentials") | |
| _ = utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName) | |
| By("Copying AWS credentials") | |
| Expect(utils.CopyAWSCredsToNamespace(ctx, providerClientset, providerTestNamespace, awsCredsLocalName)). | |
| To(Succeed(), "failed to copy AWS credentials into test namespace") |
🤖 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/provider_e2e_test.go` around lines 592 - 593, The AWS credential
copy step is discarding the result of CopyAWSCredsToNamespace, which can hide
setup failures and cause later SecretStore/PushSecret checks to fail
confusingly. Update the provider_e2e_test flow to assert that
CopyAWSCredsToNamespace succeeds, matching the earlier AWS Parameter Store setup
pattern, and keep the failure close to the credential-copy step for clearer test
diagnostics.
|
@siddhibhor-56: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Added test cases for :-
Summary by CodeRabbit