CNTRLPLANE-2161: test/e2e: migrate serving-cert-annotation test for OTE compatibility#297
Conversation
WalkthroughAdds a Ginkgo v2-based E2E test suite and test registration file, updates go.mod to require Ginkgo and replace it with an OpenShift fork, and consolidates/refactors existing e2e test helpers into the new test implementation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
wangke19
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![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: Organization 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/service-ca-operator-tests-ext/main.go(1 hunks)go.mod(1 hunks)test/e2e/serving_cert.go(1 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:
cmd/service-ca-operator-tests-ext/main.gotest/e2e/serving_cert.gogo.mod
🧬 Code graph analysis (1)
test/e2e/serving_cert.go (1)
pkg/controller/api/api.go (1)
ServingCertSecretAnnotation(47-47)
🔇 Additional comments (8)
go.mod (1)
8-8: LGTM! Ginkgo dependency correctly configured.The changes properly move
ginkgo/v2from an indirect to a direct dependency and add a replace directive pointing to the OpenShift fork, which aligns with the OTE framework requirements mentioned in the PR objectives.Also applies to: 129-129
test/e2e/serving_cert.go (6)
1-21: LGTM! Imports are well-organized.The imports are appropriate for a Kubernetes end-to-end test using Ginkgo. The alias
gfor Ginkgo is a common convention that improves readability.
27-34: LGTM! Test structure follows Ginkgo best practices.The test correctly uses closure variable capture (
headlessValue := headless) to avoid common loop variable issues in goroutines. The test tags[Operator][Serial]properly align with the suite configuration incmd/service-ca-operator-tests-ext/main.go.Also applies to: 131-134
33-44: LGTM! Client setup correctly uses BuildConfigFromFlags.The use of
clientcmd.BuildConfigFromFlagsaligns with the PR objective to avoid stdout pollution that would interfere with OTE JSON parsing. The fallback to/tmp/admin.confis appropriate for test environments.
46-59: LGTM! Namespace lifecycle management is appropriate.The test correctly creates an isolated namespace with a random suffix and ensures cleanup via defer. Logging cleanup errors without failing the test is a reasonable approach, as namespace deletion failures shouldn't mask the actual test results.
61-105: LGTM! Service creation and secret polling logic is correct.The test properly configures both standard and headless service scenarios. The polling logic correctly distinguishes between
NotFounderrors (which should continue polling) and other errors (which should fail immediately). The 10-second timeout is reasonable for certificate provisioning.
107-130: LGTM! Secret validation thoroughly checks certificate data.The validation correctly verifies:
- Secret contains exactly 2 entries (certificate and private key)
- Both
TLSCertKeyandTLSPrivateKeyKeyare present- Certificate data is valid PEM-encoded X.509 format
cmd/service-ca-operator-tests-ext/main.go (1)
23-24: LGTM! Correct pattern for registering Ginkgo tests.The anonymous import is the standard Go idiom to ensure the
init()functions in thetest/e2epackage execute, which registers the Ginkgo test suites defined intest/e2e/serving_cert.go.
wangke19
force-pushed
the
ote-migrate-serving-cert-annotation
branch
from
4913c1b to
cdd5f7e
Compare
Add Ginkgo wrapper for OTE framework while keeping original test unchanged. Changes: - Add test/e2e/serving_cert.go with Ginkgo wrapper and shared function - Move helper functions to serving_cert.go with testing.TB signatures - Simplify e2e_test.go to call shared function - Add cmd/service-ca-operator-tests-ext/dependencymagnet.go for test registration - Remove blank import from main.go (moved to dependencymagnet.go) - Use dependencymagnet.go pattern following OpenShift operator best practices Both frameworks tested successfully: - Standard go test: PASSED (1.94s, 1.84s) - OTE framework: PASSED (2.39s, 1.99s) Files changed: 4 (serving_cert.go, e2e_test.go, main.go, dependencymagnet.go)
wangke19
force-pushed
the
ote-migrate-serving-cert-annotation
branch
from
cdd5f7e to
17bf176
Compare
wangke19
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
wangke19
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@wangke19: This pull request references CNTRLPLANE-2161 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 task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
Remove extra blank lines in e2e_test.go to satisfy gofmt verification. This fixes the CI failure: verify-gofmt failed - please run `make update-gofmt`
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization 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 (2)
test/e2e/e2e_test.gotest/e2e/serving_cert.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/serving_cert.go
🧰 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/e2e_test.go
🔇 Additional comments (1)
test/e2e/e2e_test.go (1)
1054-1060: LGTM! Clean refactoring to shared test function.The migration to call
testServingCertAnnotation(t, headless)simplifies the test structure and enables reuse across both standard go test and Ginkgo/OTE frameworks. The approach follows established patterns and maintains test coverage for both headless and non-headless service scenarios.
|
@wangke19 better to keep same filename like serving-cert.go to e2e.go. |
I perfer to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/e2e/sa_operator.go (3)
24-27: Unused constants.
pollIntervalandpollTimeoutare declared but never referenced in this file.pollForServiceServingSecretat line 178 uses hardcodedtime.Secondand10*time.Secondinstead. Consider removing these constants or using them consistently.
31-33:rand.Seedis deprecated.Since Go 1.20, the global random source is automatically seeded. This
init()function can be removed entirely.🔎 Proposed fix
-func init() { - rand.Seed(time.Now().UnixNano()) -}
146-174: Duplicated constant.
owningHeadlessServiceLabelNameis defined locally here but also exists as a package-level constant ine2e_test.go(line 57). Since both files are in the same package, consider removing this local declaration to use the existing constant.🔎 Proposed fix
func createServingCertAnnotatedService(client *kubernetes.Clientset, secretName, serviceName, namespace string, headless bool) error { - const owningHeadlessServiceLabelName = "owning-headless-service" service := &v1.Service{
📜 Review details
Configuration used: Organization 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 (2)
test/e2e/e2e_test.gotest/e2e/sa_operator.go
🧰 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/sa_operator.gotest/e2e/e2e_test.go
🧬 Code graph analysis (1)
test/e2e/sa_operator.go (1)
pkg/controller/api/api.go (1)
ServingCertSecretAnnotation(47-47)
🔇 Additional comments (9)
test/e2e/sa_operator.go (7)
35-44: LGTM!The Ginkgo test definition correctly captures the loop variable and uses
GinkgoTB()for dual-framework compatibility.
46-86: LGTM!The shared test function is well-documented and properly structured for dual-framework compatibility. The cleanup is correctly deferred, and error handling is appropriate.
88-129: LGTM!The kubeconfig handling provides clear, actionable error messages that distinguish between different failure scenarios. The comment at line 118 about avoiding stdout output for OTE compatibility is helpful context.
131-144: LGTM!The namespace creation with cleanup function pattern is appropriate. The cleanup correctly logs rather than fails on deletion errors.
176-188: LGTM!Polling logic is correct. The NotFound case properly continues polling while other errors propagate.
190-216: LGTM!The TLS validation logic correctly verifies the secret structure and certificate validity. The strict
len(sss.Data) != 2check is appropriate for TLS secrets.
218-225: LGTM!The random string generation is appropriate for test resource name uniqueness.
test/e2e/e2e_test.go (2)
1016-1042: LGTM!The improved KUBECONFIG handling now provides clear, actionable feedback. The warning log at line 1027 and the distinct skip messages properly address the concern about misleading error messages when
KUBECONFIGis explicitly set to an invalid path.
1063-1069: LGTM!The refactoring to use the shared
testServingCertAnnotationfunction reduces duplication and aligns with the OTE migration pattern.
|
/test okd-scos-images |
|
CI job okd-scos-images ran into known issue: https://redhat-internal.slack.com/archives/CBN38N3MW/p1765791339549229 |
|
/test okd-scos-images |
| func TestE2E(t *testing.T) { | ||
| // use /tmp/admin.conf (placed by ci-operator) or KUBECONFIG env | ||
| confPath := "/tmp/admin.conf" | ||
| // Check for kubeconfig in this order: |
There was a problem hiding this comment.
could we move test/e2e: improve kubeconfig detection and error handling commit to a new PR ?
|
/override ci/prow/okd-scos-images |
|
@wangke19: wangke19 unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. DetailsIn 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 kubernetes-sigs/prow repository. |
- Rename test/e2e/sa_operator.go → test/e2e/e2e.go per reviewer feedback - Add comment to t.Run section explaining duplication is temporary - The duplication exists until we fully migrate to OTE framework
|
@wangke19: This pull request references CNTRLPLANE-2161 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 task to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
Go 1.20+ automatically seeds the global rand, making the manual rand.Seed(time.Now().UnixNano()) call in init() unnecessary. This project uses Go 1.24.0. See: https://go.dev/doc/go1.20#math/rand
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/e2e.go (1)
24-27: Unused constants; consider removal or usage.The constants
pollIntervalandpollTimeoutare defined but never used. Line 156 inpollForServiceServingSecrethardcodes different values (time.Secondand10*time.Second). Either remove these unused constants or refactor line 156 to use them.🔎 Recommended approach
Option 1: Remove unused constants
-const ( - pollInterval = 5 * time.Second - pollTimeout = 60 * time.Second -) - var characters = []rune("abcdefghijklmnopqrstuvwxyz0123456789")Option 2: Use the constants in pollForServiceServingSecret
func pollForServiceServingSecret(client *kubernetes.Clientset, secretName, namespace string) error { - return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { + return wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) { _, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
📜 Review details
Configuration used: Organization 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 (1)
test/e2e/e2e.go
🧰 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/e2e.go
|
/jira refresh |
|
@wangke19: This pull request references CNTRLPLANE-2161 which is a valid jira issue. DetailsIn 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. |
|
/payload-job-with-prs pull-ci-openshift-service-ca-operator-main-okd-scos-images coreos/rhel-coreos-config#134 |
|
@wangke19: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
ci/prow/okd-scos-images is fixed by openshift/release#72984 |
|
/test okd-scos-images |
1 similar comment
|
/test okd-scos-images |
|
/test okd-scos-images |
|
@wangke19: all tests passed! 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr, wangke19 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 |
|
/verified by CI tests |
openshift-ci-robot
added
the
verified
|
@wangke19: This PR has been marked as verified by DetailsIn 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. |
…ice-ca-operator pattern This commit implements the same dual-compatibility pattern as service-ca-operator PR openshift#297, allowing tests to run both as standard Go tests and through the OTE framework. Files added: - cmd/run-once-duration-override-operator-tests-ext/dependencymagnet.go: Imports test packages to ensure they're included in the build and register Ginkgo tests with the OTE framework - test/e2e/operator.go: Ginkgo test specs using g.Describe() with shared test functions that accept testing.TB interface for dual compatibility - test/e2e/operator_test.go: Standard Go test wrapper that calls the shared functions from operator.go - test/e2e/helpers.go: Kubernetes client helper functions and Init() function to ensure package-level Ginkgo registrations are executed Key features: - Uses testing.TB interface for all test functions (setupOperator, testActiveDeadlineSecondsWebhook, cleanupTestNamespace) - Ginkgo specs use g.GinkgoTB() to call shared functions - Standard Go tests use *testing.T to call the same functions - Follows the exact pattern from service-ca-operator PR openshift#297 Reference: openshift/service-ca-operator#297 Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Implements dual-compatibility pattern from service-ca-operator PR openshift#297, allowing tests to run both as standard Go tests and through OTE framework. Changes: - Add cmd/run-once-duration-override-operator-tests-ext/dependencymagnet.go: Imports test packages to register Ginkgo tests with OTE framework - Add test/e2e/operator.go: Ginkgo test specs with shared test functions using testing.TB interface for dual compatibility - Replace test/e2e/operator_test.go: New standard Go test wrapper that calls shared functions from operator.go - Add test/e2e/helpers.go: Kubernetes client helpers and Init() function Key features: - Uses testing.TB interface (setupOperator, testActiveDeadlineSecondsWebhook, cleanupTestNamespace) - Ginkgo specs use g.GinkgoTB(), standard Go tests use *testing.T - Matches service-ca-operator PR openshift#297 pattern exactly Reference: openshift/service-ca-operator#297 Co-Authored-By: Rohit Patil <ropatil@redhat.com>
4 participants
Summary
Migrate the
serving-cert-annotationtest to support OTE framework execution using shared function pattern withtesting.TBinterface.Changes
test/e2e/e2e.go: Ginkgo wrapper + shared test function acceptingtesting.TBtest/e2e/e2e_test.go: Inline test now calls shared function (126 lines removed)cmd/service-ca-operator-tests-ext/dependencymagnet.go: Test registration following OpenShift operator best practicescmd/service-ca-operator-tests-ext/main.go: Remove blank import (moved to dependencymagnet.go)e2e.gowithtesting.TBsignatures for dual-compatibilityApproach
Uses shared function pattern with
testing.TBinterface:testServingCertAnnotation(t testing.TB, headless bool)works with both frameworks*testing.TGinkgoTB()testing.TBinstead of*testing.Tcluster-kube-apiserver-operatorfor test registrationTest Results
Standard Go Test Framework
$ KUBECONFIG=/home/kewang/work/openshift/env420a/kubeconfig go test -v ./test/e2e -run TestE2E/serving-cert-annotation === RUN TestE2E/serving-cert-annotation === RUN TestE2E/serving-cert-annotation/headless=false --- PASS: TestE2E/serving-cert-annotation/headless=false (1.94s) === RUN TestE2E/serving-cert-annotation/headless=true --- PASS: TestE2E/serving-cert-annotation/headless=true (1.84s) --- PASS: TestE2E (3.44s) PASSOTE Framework
Files Changed
Test Tags
[sig-service-ca],[Operator],[Serial]Related