[WIP]CNTRLPLANE-2072:Migrating TestTokenRequestAndReview to ginkgo #1978
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors bound service account token tests by extracting shared test logic into a reusable testTokenRequestAndReview function and TestingT interface. Adds E2E test file with Ginkgo-based test suite. Updates main package to register E2E tests via blank import. Simplifies existing test file to delegate to shared logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
gangwgr
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr The full list of commands accepted by this bot can be found here.
Needs 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
cmd/cluster-kube-apiserver-operator-tests-ext/main.go(1 hunks)test/e2e/bound_sa_token.go(1 hunks)test/e2e/bound_sa_token_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/bound_sa_token_test.gotest/e2e/bound_sa_token.gocmd/cluster-kube-apiserver-operator-tests-ext/main.go
🧬 Code graph analysis (1)
test/e2e/bound_sa_token.go (1)
test/library/client.go (1)
NewClientConfigForTest(12-20)
🔇 Additional comments (5)
cmd/cluster-kube-apiserver-operator-tests-ext/main.go (1)
22-23: LGTM!The blank import with explanatory comment correctly registers the E2E tests with the test extension framework via side-effect initialization.
test/e2e/bound_sa_token.go (3)
17-24: LGTM!The
TestingTinterface correctly captures the minimal common interface betweentesting.TBandginkgo.GinkgoTInterface, enabling code reuse across both test frameworks.
26-30: LGTM!The Ginkgo test registration with
[Operator][Serial]tags correctly matches the suite qualifier defined in the test extension registry.
57-83: LGTM!The test logic correctly exercises the TokenRequest and TokenReview APIs with appropriate assertions. The service account creation, token request with default audience, and authentication validation are all properly implemented.
test/e2e/bound_sa_token_test.go (1)
169-177: LGTM!Clean delegation to the shared
testTokenRequestAndReviewfunction with a clear comment explaining the dual-use pattern. The*testing.Ttype satisfies theTestingTinterface, enabling seamless reuse.
| defer func() { | ||
| err := corev1client.Namespaces().Delete(context.TODO(), ns.Name, metav1.DeleteOptions{}) | ||
| require.NoError(t, err) | ||
| }() |
There was a problem hiding this comment.
Avoid require in defer for cleanup.
Using require.NoError (which calls FailNow()) inside a defer can cause issues—if the test already failed, it may produce confusing output, and FailNow() in a defer can behave unexpectedly in some contexts.
Consider logging the error instead or using assert.NoError which doesn't abort:
defer func() {
err := corev1client.Namespaces().Delete(context.TODO(), ns.Name, metav1.DeleteOptions{})
- require.NoError(t, err)
+ if err != nil {
+ t.Errorf("failed to delete namespace %s: %v", ns.Name, err)
+ }
}()📝 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.
| defer func() { | |
| err := corev1client.Namespaces().Delete(context.TODO(), ns.Name, metav1.DeleteOptions{}) | |
| require.NoError(t, err) | |
| }() | |
| defer func() { | |
| err := corev1client.Namespaces().Delete(context.TODO(), ns.Name, metav1.DeleteOptions{}) | |
| if err != nil { | |
| t.Errorf("failed to delete namespace %s: %v", ns.Name, err) | |
| } | |
| }() |
🤖 Prompt for AI Agents
In test/e2e/bound_sa_token.go around lines 51 to 54, the deferred cleanup
currently calls require.NoError which invokes FailNow inside a defer—replace it
so deferred cleanup does not abort the test: capture the delete error and either
call assert.NoError(t, err) (non-fatal) or log it with t.Logf/t.Errorf if
non-nil; alternatively move the namespace deletion into t.Cleanup and use
require.NoError there (since t.Cleanup runs outside a defer), ensuring the
cleanup path never calls FailNow from within a defer.
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-2072 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@gangwgr: all tests passed! Full PR test history. Your PR dashboard. Instructions 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. |
2 participants
Migrate TestTokenRequestAndReview to dual-compatible test framework
This commit migrates the TestTokenRequestAndReview test to support both
Ginkgo (OTE framework) and standard Go test execution using a
dual-compatibility approach.
Changes:
Created test/e2e/bound_sa_token.go with:
and ginkgo.GinkgoTInterface implement
Updated test/e2e/bound_sa_token_test.go:
shared testTokenRequestAndReview(t) function
Replaced Gomega assertions (o.Expect) with require package assertions
(require.NoError, require.Empty, require.True) which work with the
minimal TestingT interface
Benefits:
Helper methods (compatible with both testing.TB and GinkgoTInterface)