CM-902: Add table-driven unit tests and improve coverage#385
CM-902: Add table-driven unit tests and improve coverage#385anandkuma77 wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThis PR adds comprehensive unit test coverage across multiple Go packages including client management, error handling, utility functions, deployment hooks, object comparison logic, and asset APIs—without modifying any production code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandkuma77 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 |
- Add unit tests for modified functions to improve coverage - Align trustmanager utils tests with table-driven style - Add table-driven tests for refactor safety across controller packages - Update common and deployment test files; remove obsolete istiocsr test Made-with: Cursor
anandkuma77
force-pushed
the
CM-902-add-testcases
branch
from
e98875a to
948bd64
Compare
anandkuma77
changed the title
|
@anandkuma77: This pull request references CM-902 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 story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
@anandkuma77: This pull request references CM-902 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 story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. 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. |
1 similar comment
|
@anandkuma77: This pull request references CM-902 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 story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. 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. |
openshift-ci
bot
removed
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: 2
🧹 Nitpick comments (5)
pkg/operator/applyconfigurations/utils_test.go (2)
54-57: Tautological assertion provides no value.The condition
if !didPanic && conv != nilalready guaranteesconv != nil, soassert.NotNil(t, conv, ...)will always pass. This makes the assertion meaningless and obscures the test's intent.♻️ Proposed fix: simplify to documentation-only
// Document: for nil scheme, either conv is nil (if we returned) or we panicked. - if !didPanic && conv != nil { - assert.NotNil(t, conv, tt.description) + if !didPanic { + t.Logf("nil scheme returned converter (nil=%v): %s", conv == nil, tt.description) }Alternatively, if behavior should be locked in, assert a specific expected outcome rather than a condition that's always true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/applyconfigurations/utils_test.go` around lines 54 - 57, The assertion `assert.NotNil(t, conv, tt.description)` inside the `if !didPanic && conv != nil` branch is tautological and should be removed or replaced; update the test in utils_test.go by either deleting that redundant assertion and leaving a clarifying comment explaining that the branch only runs when `conv` is non-nil, or replace it with a concrete expected-state check (e.g., assert properties of `conv` or assert specific error/return behavior) so the test actually verifies behavior of `conv` rather than restating the if-condition.
16-21: Remove unusedexpectErrorfield.The
expectErrorfield is declared but never referenced in the test logic. This dead code harms maintainability and suggests incomplete test design.♻️ Proposed fix
tests := []struct { name string scheme *runtime.Scheme - expectError bool description string }{Also update the test cases to remove
expectError: false,from each entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/applyconfigurations/utils_test.go` around lines 16 - 21, The tests table in utils_test.go declares an unused field expectError inside the tests slice literal; remove the expectError field from the struct definition and delete any expectError: false entries from each test case so the tests struct only contains name, scheme, and description; update any code referencing expectError (none expected) and run the tests to confirm compilation.pkg/controller/deployment/credentials_request_test.go (1)
118-122: Avoid hardcoding test case names in conditional logic.This check couples test execution logic to the test case name string, making refactoring fragile. Consider adding a dedicated field to the test struct (e.g.,
addDifferentSecret bool) to control this behavior.♻️ Suggested improvement
tests := []struct { name string deploymentName string secretName string secretInStore bool platformType configv1.PlatformType wantErr string wantVolumes int wantMountPath string wantAWSEnv bool noInfra bool + addOtherSecret bool // if true, add a different secret to trigger retry }{Then update the test case:
{ name: "secret not found returns retry error", deploymentName: certmanagerControllerDeployment, secretName: "missing-secret", secretInStore: false, platformType: configv1.AWSPlatformType, wantErr: "Retrying", wantVolumes: 0, + addOtherSecret: true, },And update the setup code:
- if tt.name == "secret not found returns retry error" { - kubeClient = fake.NewSimpleClientset(&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"}, - }) - } + if tt.addOtherSecret { + kubeClient = fake.NewSimpleClientset(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"}, + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/credentials_request_test.go` around lines 118 - 122, The test currently branches on the literal test name string ("secret not found returns retry error"); add a boolean field to the test case struct (e.g., addDifferentSecret bool) and set it on the specific test case instead of matching tt.name, then update the setup to use that field (if tt.addDifferentSecret { kubeClient = fake.NewSimpleClientset(&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"},}) } ). Update any test case definitions to set addDifferentSecret = true for the case that needs the different secret; keep the rest of the setup and assertions unchanged (look for tt, kubeClient, and the fake.NewSimpleClientset call to locate the code to modify).pkg/controller/trustmanager/utils_test.go (1)
240-244: Panic assertion is too permissive and can hide regressions.Allowing any panic containing
"interface"can pass unrelated failure modes. Match only the expected substring for this case.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed tightening
- if !strings.Contains(msg, tt.panicSubstr) && !strings.Contains(msg, "interface") { + if !strings.Contains(msg, tt.panicSubstr) { t.Errorf("panic message = %q, want substring %q", msg, tt.panicSubstr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/utils_test.go` around lines 240 - 244, The panic assertion in the test currently accepts any panic message containing "interface" which is too permissive; update the check in the recovery block (where tt.panicSubstr is used) to require only that the panic message contains tt.panicSubstr (remove the "|| strings.Contains(msg, \"interface\")" fallback) so failures only pass when the expected substring matches; if you intended to special-case a known "interface" panic, make that explicit by setting tt.panicSubstr to "interface" in the test case instead of allowing it implicitly.pkg/controller/istiocsr/utils_test.go (1)
57-69: Remove the duplicated panic scenario from the table.The cases on Line 57–62 and Line 64–69 assert the same input/output behavior (
ClusterRolevsClusterRoleBindingwith"same type"panic). Keeping one avoids redundant maintenance.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/istiocsr/utils_test.go` around lines 57 - 69, There is a duplicated test case asserting a panic for mismatched types (ClusterRole vs ClusterRoleBinding): remove the second entry named "mismatched types (CR vs CRB) panics" so only one table row remains for the ClusterRole vs ClusterRoleBinding panic scenario; update the test table in pkg/controller/istiocsr/utils_test.go (the entries with name "different types panics" and "mismatched types (CR vs CRB) panics") by deleting the redundant block and keep a single canonical case to avoid duplicate assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/common/client_test.go`:
- Around line 51-53: Replace the nil sentinel client with a non-nil sentinel
instance so the test actually verifies wiring through the manager: initialize cl
to a concrete non-nil value that implements client.Client (e.g., a small test
stub instance), set fakeManager.client = cl, call NewClient(...) and assert the
returned client is the exact same instance (pointer/identity) as cl; update any
other test occurrences (the second cl at the other test block and its assertion)
to use the same non-nil sentinel to prevent NewClient from silently returning
nil and to strengthen the regression check.
In `@pkg/controller/trustmanager/utils_test.go`:
- Around line 155-187: Add a positive test case to
TestValidateTrustManagerConfig that constructs a valid v1alpha1.TrustManager
(populate Spec.TrustManagerConfig with fields that meet
validateTrustManagerConfig requirements) and set wantErr: false; run the
existing assertions against that case so validateTrustManagerConfig is exercised
on a success path. Reference TestValidateTrustManagerConfig and the
validateTrustManagerConfig call, and use v1alpha1.TrustManager /
v1alpha1.TrustManagerSpec / v1alpha1.TrustManagerConfig when building the valid
input.
---
Nitpick comments:
In `@pkg/controller/deployment/credentials_request_test.go`:
- Around line 118-122: The test currently branches on the literal test name
string ("secret not found returns retry error"); add a boolean field to the test
case struct (e.g., addDifferentSecret bool) and set it on the specific test case
instead of matching tt.name, then update the setup to use that field (if
tt.addDifferentSecret { kubeClient =
fake.NewSimpleClientset(&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name:
"other", Namespace: "cert-manager"},}) } ). Update any test case definitions to
set addDifferentSecret = true for the case that needs the different secret; keep
the rest of the setup and assertions unchanged (look for tt, kubeClient, and the
fake.NewSimpleClientset call to locate the code to modify).
In `@pkg/controller/istiocsr/utils_test.go`:
- Around line 57-69: There is a duplicated test case asserting a panic for
mismatched types (ClusterRole vs ClusterRoleBinding): remove the second entry
named "mismatched types (CR vs CRB) panics" so only one table row remains for
the ClusterRole vs ClusterRoleBinding panic scenario; update the test table in
pkg/controller/istiocsr/utils_test.go (the entries with name "different types
panics" and "mismatched types (CR vs CRB) panics") by deleting the redundant
block and keep a single canonical case to avoid duplicate assertions.
In `@pkg/controller/trustmanager/utils_test.go`:
- Around line 240-244: The panic assertion in the test currently accepts any
panic message containing "interface" which is too permissive; update the check
in the recovery block (where tt.panicSubstr is used) to require only that the
panic message contains tt.panicSubstr (remove the "|| strings.Contains(msg,
\"interface\")" fallback) so failures only pass when the expected substring
matches; if you intended to special-case a known "interface" panic, make that
explicit by setting tt.panicSubstr to "interface" in the test case instead of
allowing it implicitly.
In `@pkg/operator/applyconfigurations/utils_test.go`:
- Around line 54-57: The assertion `assert.NotNil(t, conv, tt.description)`
inside the `if !didPanic && conv != nil` branch is tautological and should be
removed or replaced; update the test in utils_test.go by either deleting that
redundant assertion and leaving a clarifying comment explaining that the branch
only runs when `conv` is non-nil, or replace it with a concrete expected-state
check (e.g., assert properties of `conv` or assert specific error/return
behavior) so the test actually verifies behavior of `conv` rather than restating
the if-condition.
- Around line 16-21: The tests table in utils_test.go declares an unused field
expectError inside the tests slice literal; remove the expectError field from
the struct definition and delete any expectError: false entries from each test
case so the tests struct only contains name, scheme, and description; update any
code referencing expectError (none expected) and run the tests to confirm
compilation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9605116-c267-4862-9294-9bdc97489909
📒 Files selected for processing (12)
pkg/controller/common/client_test.gopkg/controller/common/errors_test.gopkg/controller/common/fakes/fake_ctrl_client_test.gopkg/controller/common/utils_test.gopkg/controller/deployment/credentials_request_test.gopkg/controller/deployment/deployment_overrides_test.gopkg/controller/istiocsr/utils_test.gopkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/utils_test.gopkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.gopkg/operator/applyconfigurations/utils_test.gopkg/operator/assets/bindata_test.go
| var cl client.Client = nil | ||
| mgr := &fakeManager{client: cl} | ||
|
|
There was a problem hiding this comment.
Use a non-nil client sentinel; current “same client” assertion is too weak.
Line 51 and Line 93 set cl to nil, so Line 100 can pass even if NewClient stops wiring through the manager client and always returns nil. This reduces regression protection for the main behavior these tests are documenting.
Proposed test hardening
func TestNewClient(t *testing.T) {
- var cl client.Client = nil
+ cl := &struct{ client.Client }{} // non-nil sentinel implementing client.Client
mgr := &fakeManager{client: cl}
@@
func TestNewClient_GetClientReturnsSameClient(t *testing.T) {
- var cl client.Client = nil
+ cl := &struct{ client.Client }{} // non-nil sentinel implementing client.Client
mgr := &fakeManager{client: cl}
@@
- assert.Equal(t, cl, impl.Client, "client must be the manager's client")
+ assert.Same(t, cl, impl.Client, "client must be the manager's client instance")
}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 93-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/common/client_test.go` around lines 51 - 53, Replace the nil
sentinel client with a non-nil sentinel instance so the test actually verifies
wiring through the manager: initialize cl to a concrete non-nil value that
implements client.Client (e.g., a small test stub instance), set
fakeManager.client = cl, call NewClient(...) and assert the returned client is
the exact same instance (pointer/identity) as cl; update any other test
occurrences (the second cl at the other test block and its assertion) to use the
same non-nil sentinel to prevent NewClient from silently returning nil and to
strengthen the regression check.
| func TestValidateTrustManagerConfig(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| tm *v1alpha1.TrustManager | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "empty config returns error", | ||
| tm: &v1alpha1.TrustManager{ | ||
| Spec: v1alpha1.TrustManagerSpec{ | ||
| TrustManagerConfig: v1alpha1.TrustManagerConfig{}, | ||
| }, | ||
| }, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "zero value config returns error", | ||
| tm: &v1alpha1.TrustManager{Spec: v1alpha1.TrustManagerSpec{}}, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := validateTrustManagerConfig(tt.tm) | ||
| if (err != nil) != tt.wantErr { | ||
| t.Errorf("validateTrustManagerConfig() error = %v, wantErr %v", err, tt.wantErr) | ||
| } | ||
| if err != nil && err.Error() == "" { | ||
| t.Error("error message should be non-empty") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a success-path case to TestValidateTrustManagerConfig.
All current rows assert wantErr: true, so an implementation that always returns an error would still pass this test suite. Add at least one known-valid config case with wantErr: false to make this test actually guard behavior.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/trustmanager/utils_test.go` around lines 155 - 187, Add a
positive test case to TestValidateTrustManagerConfig that constructs a valid
v1alpha1.TrustManager (populate Spec.TrustManagerConfig with fields that meet
validateTrustManagerConfig requirements) and set wantErr: false; run the
existing assertions against that case so validateTrustManagerConfig is exercised
on a success path. Reference TestValidateTrustManagerConfig and the
validateTrustManagerConfig call, and use v1alpha1.TrustManager /
v1alpha1.TrustManagerSpec / v1alpha1.TrustManagerConfig when building the valid
input.
|
@anandkuma77: 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. |
2 participants
What
Adds or expands _test.go coverage for controller common utilities (client helpers, errors, utils) including fake controller-runtime client tests.
Adds tests for deployment flows (credentials_request, deployment_overrides).
Adds istiocsr utils tests and trustmanager utils / controller tests, using table-driven patterns where appropriate.
Adds operator tests for applyconfigurations (certmanagerconfig, utils) and assets/bindata validation.
Why
Increases confidence when refactoring controllers and shared helpers by locking in current behavior with explicit, maintainable tests.
Table-driven tests make edge cases and regressions easier to see and extend.
Improves coverage in areas that were previously untested or lightly tested, reducing risk for future CM-902–related and general maintenance work.