CNTRLPLANE-2589: Migrate test/e2e-encryption-perf to OpenShift Tests Extension framework#926
Open
ropatil010 wants to merge 1 commit into
Open
CNTRLPLANE-2589: Migrate test/e2e-encryption-perf to OpenShift Tests Extension framework#926ropatil010 wants to merge 1 commit into
ropatil010 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe AES-CBC encryption performance test logic is extracted from ChangesEncryption perf test refactor and OTE registration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 3 warnings)
✅ Passed checks (9 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: 2
🤖 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-encryption-perf/encryption_perf.go`:
- Around line 56-67: The AssertDBPopulatedFunc closure is not validating the
errorStore parameter, allowing the test to pass even when DB loader workers
report failures. Add an assertion early in the AssertDBPopulatedFunc that checks
if errorStore contains any entries, and if it does, fail the test using
require.Fail or similar with a message that includes the collected errors so
loader failures are caught instead of silently ignored.
In `@test/library/encryption/scenarios.go`:
- Around line 46-57: The hardcoded 30 second timeout in the time.After() call
within the select statement can cause premature failures on slow clusters even
when the system is functioning correctly. Replace the fixed duration of 30 *
time.Second with a reference to the migration watcher's configured timeout value
to ensure the channel wait window is properly aligned with the watcher's actual
timeout configuration instead of using an arbitrary fixed duration.
🪄 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: 1bbedebe-122c-4fce-a0aa-369d62586ef2
📒 Files selected for processing (6)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.gotest/library/encryption_wrappers.go
✅ Action performedFull review finished. |
Contributor
Author
|
/test e2e-agnostic-upgrade |
ropatil010
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@ropatil010: This pull request references CNTRLPLANE-2589 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 epic to target the "5.0.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. |
Contributor
Author
|
/assign @liouk
PASS logs for migrated case |
Contributor
Author
|
/retest |
liouk
suggested changes
Jun 26, 2026
liouk
left a comment
liouk
left a comment
Member
There was a problem hiding this comment.
Overall, it looks like we're copying and adapting quite a lot of functions from library-go mainly because we're changing *testing.T to testing.TB. Is there a plan to eventually update these funcs in library-go as well, so that we don't have to copy code over for this change? If we're doing a global OTE migration, this should definitely be part of it (I'd even argue that we should have started there). This way we can reduce code duplication just for plumbing needs.
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ropatil010
pushed a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Jun 26, 2026
This commit addresses all reviewer suggestions for PR openshift#926: 1. Context Injection and Propagation: - Add context.Context parameter to Ginkgo tests - Use parent context instead of context.Background() - Ginkgo now injects context: g.It(..., func(ctx context.Context)) - Standard tests use tt.Context() - Context flows properly: Ginkgo -> test -> library functions 2. Migration Watcher Check: - Remove overly-restrictive cond.Reason == "Migrating" check - Now matches library-go behavior (only checks Status) - Prevents false failures if controller uses different reason strings 3. Remove Unnecessary Wrapper File: - Delete test/library/encryption_wrappers.go (39 lines) - Update caller to use local implementation directly - No functionality lost - wrappers were just pass-throughs 4. Improved Assertion Pattern: - Replace error creation + require.NoError() with direct assertions - Use require.True() and require.GreaterOrEqual() - More readable and follows Go testing best practices - Remove unused errors import 5. Dual-Mode Support: - Both standard Go test and Ginkgo modes work correctly - Standard: TestPerfEncryptionTypeAESCBC(tt.Context(), tt) - Ginkgo: g.It(..., func(ctx context.Context)) - Shared implementation uses testing.TB interface Changes: - Modified: 4 files - Deleted: 1 file - Net: -47 lines (simplified code) All changes verified: - Compiles in both test modes - No go vet issues - Proper context propagation - Consistent application of patterns Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/library/encryption/perf_helpers.go (1)
43-47: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the test-process clock for the value sent on
migrationStartedCh.Line 47 sends the operator condition timestamp, but
scenarios.gosubtracts it from a locally captured end timestamp. If the operator/test clocks differ, the perf duration can be negative or inflated. Send a local observation timestamp here, or compute both endpoints from the same clock source.Suggested direction
if cond.Type == "EncryptionMigrationControllerProgressing" && cond.Status == operatorv1.ConditionTrue && cond.LastTransitionTime.Time.After(testStartTime) { - t.Logf("EncryptionMigrationControllerProgressing condition observed at %v with reason %q", cond.LastTransitionTime, cond.Reason) - migrationStartedCh <- cond.LastTransitionTime.Time + observedAt := time.Now() + t.Logf("EncryptionMigrationControllerProgressing condition observed at %v with reason %q; local observation time %v", cond.LastTransitionTime, cond.Reason, observedAt) + migrationStartedCh <- observedAt return true, nil }🤖 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/library/encryption/perf_helpers.go` around lines 43 - 47, The timestamp sent on migrationStartedCh in the condition-watching helper should come from the test-process clock, not from cond.LastTransitionTime, because scenarios.go later compares it to a locally captured end time. Update the logic around the EncryptionMigrationControllerProgressing check so it logs the operator timestamp for diagnostics if needed, but sends a local observation time from the test clock on migrationStartedCh to keep both endpoints on the same clock source.
🤖 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.
Outside diff comments:
In `@test/library/encryption/perf_helpers.go`:
- Around line 43-47: The timestamp sent on migrationStartedCh in the
condition-watching helper should come from the test-process clock, not from
cond.LastTransitionTime, because scenarios.go later compares it to a locally
captured end time. Update the logic around the
EncryptionMigrationControllerProgressing check so it logs the operator timestamp
for diagnostics if needed, but sends a local observation time from the test
clock on migrationStartedCh to keep both endpoints on the same clock source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e3e9b695-94e4-499f-9a3b-dacbd0879ece
📒 Files selected for processing (4)
test/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e-encryption-perf/encryption_perf.go
- test/e2e-encryption-perf/encryption_perf_test.go
Contributor
Author
Thanks for the feedback, how ever i have copied the same contents of PR: #859 in this PR without any modifications. Initiated discussion here: https://redhat-internal.slack.com/archives/C07RDCVEYJG/p1782475409270789 |
openshift-merge-bot Bot
pushed a commit
to openshift/library-go
that referenced
this pull request
Jun 30, 2026
Change TestPerfEncryption and runTestEncryption to accept testing.TB interface instead of *testing.T concrete type. This enables these functions to be used with both standard Go tests and Ginkgo/OTE framework tests. The testing.TB interface is implemented by both *testing.T and *testing.B, as well as Ginkgo's GinkgoTB. This change is 100% backwards compatible - all existing callers passing *testing.T will continue to work. This eliminates the need for downstream operators to duplicate these functions when migrating to the OpenShift Tests Extension (OTE) framework. Currently, operators must copy ~400+ lines of code from library-go just to change *testing.T to testing.TB. Benefits: - Enables OTE framework migration across OpenShift operators - Reduces code duplication in downstream repos - Maintains full backwards compatibility - No behavior changes, only signature widening Related: openshift/cluster-authentication-operator#926 Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ropatil010
pushed a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Jun 30, 2026
Migrate encryption performance tests to support both standard Go tests
and Ginkgo/OTE framework, addressing reviewer feedback to eliminate
code duplication.
Changes:
1. Add Ginkgo test wrapper for OTE compatibility
2. Refactor to use testing.TB interface for dual-mode support
3. Update library-go to v0.0.0-20260630085645 (includes testing.TB support)
4. Remove duplicated library-go functions (432 lines):
- test/library/encryption/scenarios.go (247 lines)
- test/library/encryption/perf_helpers.go (185 lines)
5. Use library.TestPerfEncryption directly instead of local copy
6. Update vendor dependencies from library-go update
The test now works in both modes:
- Standard Go: TestPerfEncryptionTypeAESCBC(t *testing.T)
- Ginkgo/OTE: g.It(..., func(ctx) { testPerfEncryptionTypeAESCBC(ctx, g.GinkgoTB()) })
Addresses reviewer feedback from PR openshift#926 by removing code duplication
and using library-go's testing.TB-compatible functions.
Depends-on: openshift/library-go#2335
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci
Bot
added
the
needs-rebase
ropatil010
pushed a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Jun 30, 2026
Migrate encryption performance tests to support both standard Go tests
and Ginkgo/OTE framework, addressing reviewer feedback to eliminate
code duplication.
Changes:
1. Add Ginkgo test wrapper for OTE compatibility
2. Refactor to use testing.TB interface for dual-mode support
3. Update library-go to v0.0.0-20260630085645 (includes testing.TB support)
4. Remove duplicated library-go functions (432 lines):
- test/library/encryption/scenarios.go (247 lines)
- test/library/encryption/perf_helpers.go (185 lines)
5. Use library.TestPerfEncryption directly instead of local copy
6. Update vendor dependencies from library-go update
The test now works in both modes:
- Standard Go: TestPerfEncryptionTypeAESCBC(t *testing.T)
- Ginkgo/OTE: g.It(..., func(ctx) { testPerfEncryptionTypeAESCBC(ctx, g.GinkgoTB()) })
Addresses reviewer feedback from PR openshift#926 by removing code duplication
and using library-go's testing.TB-compatible functions.
Depends-on: openshift/library-go#2335
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
needs-rebase
ropatil010
pushed a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Jun 30, 2026
Migrate encryption performance tests to support both standard Go tests
and Ginkgo/OTE framework, addressing reviewer feedback to eliminate
code duplication.
Changes:
1. Add Ginkgo test wrapper for OTE compatibility
2. Refactor to use testing.TB interface for dual-mode support
3. Update library-go to v0.0.0-20260630085645 (includes testing.TB support)
4. Remove duplicated library-go functions (432 lines):
- test/library/encryption/scenarios.go (247 lines)
- test/library/encryption/perf_helpers.go (185 lines)
5. Use library.TestPerfEncryption directly instead of local copy
6. Update vendor dependencies from library-go update
The test now works in both modes:
- Standard Go: TestPerfEncryptionTypeAESCBC(t *testing.T)
- Ginkgo/OTE: g.It(..., func(ctx) { testPerfEncryptionTypeAESCBC(ctx, g.GinkgoTB()) })
Addresses reviewer feedback from PR openshift#926 by removing code duplication
and using library-go's testing.TB-compatible functions.
Depends-on: openshift/library-go#2335
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ropatil010
pushed a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Jun 30, 2026
Migrate encryption performance tests to support both standard Go tests
and Ginkgo/OTE framework, addressing reviewer feedback to eliminate
code duplication.
Changes:
1. Add Ginkgo test wrapper for OTE compatibility
2. Refactor to use testing.TB interface for dual-mode support
3. Update library-go to v0.0.0-20260630085645 (includes testing.TB support)
4. Remove duplicated library-go functions (432 lines):
- test/library/encryption/scenarios.go (247 lines)
- test/library/encryption/perf_helpers.go (185 lines)
5. Use library.TestPerfEncryption directly instead of local copy
6. Update vendor dependencies from library-go update
The test now works in both modes:
- Standard Go: TestPerfEncryptionTypeAESCBC(t *testing.T)
- Ginkgo/OTE: g.It(..., func(ctx) { testPerfEncryptionTypeAESCBC(ctx, g.GinkgoTB()) })
Addresses reviewer feedback from PR openshift#926 by removing code duplication
and using library-go's testing.TB-compatible functions.
Depends-on: openshift/library-go#2335
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Migrate encryption performance tests to support both standard Go tests
and Ginkgo/OTE framework, addressing reviewer feedback to eliminate
code duplication.
Changes:
1. Add Ginkgo test wrapper for OTE compatibility
2. Refactor to use testing.TB interface for dual-mode support
3. Update library-go to v0.0.0-20260630085645 (includes testing.TB support)
4. Remove duplicated library-go functions (432 lines):
- test/library/encryption/scenarios.go (247 lines)
- test/library/encryption/perf_helpers.go (185 lines)
5. Use library.TestPerfEncryption directly instead of local copy
6. Update vendor dependencies from library-go update
The test now works in both modes:
- Standard Go: TestPerfEncryptionTypeAESCBC(t *testing.T)
- Ginkgo/OTE: g.It(..., func(ctx) { testPerfEncryptionTypeAESCBC(ctx, g.GinkgoTB()) })
Addresses reviewer feedback from PR openshift#926 by removing code duplication
and using library-go's testing.TB-compatible functions.
Depends-on: openshift/library-go#2335
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
Hi @liouk Merged the dependency PR: #2335 |
Contributor
|
@ropatil010: 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. |
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
3 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.
Hi Team,
Splitting the original PR: #859 into smaller once to better debug and merge the PR's easily.
PR about:
This commit migrates the encryption performance tests to support both traditional go test and the OpenShift Tests Extension (OTE) framework, following the dual-mode pattern established in PR #859.
Changes:
The tests now use testing.TB interface for framework compatibility and context.Background() instead of t.Context() to avoid Ginkgo panics.
Suite details:
Test: [sig-auth] authentication operator [Encryption][Serial] TestPerfEncryptionTypeAESCBC
Summary by CodeRabbit
New Features
Bug Fixes