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 7 commits intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds many new Go unit tests across controller and operator packages covering client wrapping, error classification/conversion, fake client behavior, utility helpers, deployment hooks, resource-diff logic, trustmanager helpers, apply-configuration helpers, and embedded asset APIs; also updates Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 121-124: The test currently expects a panic for a nil manager
which locks in crash-on-nil behavior; update the test cases in
pkg/controller/common/client_test.go (the cases referencing m: nil and
expectPanic) to instead expect an error from NewClient/GetClient and assert the
returned error is non-nil and no panic occurs, and in production ensure
NewClient (or the GetClient method on the client factory) defensively checks for
a nil manager and returns a descriptive error (e.g., "nil manager") rather than
panicking so the test and code rely on returned errors not panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38d701bb-dc31-4e06-b884-df97dc6093a1
📒 Files selected for processing (2)
pkg/controller/common/client_test.gopkg/controller/trustmanager/utils_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/trustmanager/utils_test.go
|
/re-test |
|
/lgtm no user facing changes, PR is for improving the unit test coverage. Hence adding other required labels. |
openshift-ci
bot
added
docs-approved
|
/retest |
|
@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
added
the
needs-rebase
openshift-ci
bot
added
lgtm
- 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
- Use a non-nil sentinel client.Client in client tests so happy path asserts manager client is wired by pointer identity, not nil==nil. - Add a valid TrustManagerConfig case to TestValidateTrustManagerConfig to exercise validateTrustManagerConfig success path. Made-with: Cursor
anandkuma77
force-pushed
the
CM-902-add-testcases
branch
from
5be0f10 to
5a89fe2
Compare
openshift-ci
bot
removed
lgtm
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/controller/common/client_test.go (1)
121-145:⚠️ Potential issue | 🟠 MajorDon’t lock
NewClient(nil)panic into the contract.Line 121 and Line 128-135 make panic the expected behavior for nil manager input. That cements crash behavior and blocks a safer
error-return contract, whilepkg/controller/common/client.gocurrently has no nil guard.Suggested adjustment
--- a/pkg/controller/common/client_test.go +++ b/pkg/controller/common/client_test.go @@ - expectPanic bool + expectErr bool @@ - name: "nil manager - GetClient panics", + name: "nil manager returns error", m: nil, - expectPanic: true, + expectErr: true, @@ - if tt.expectPanic { - defer func() { - r := recover() - if r == nil { - t.Fatal("expected panic but got none") - } - t.Logf("NewClient(nil) panicked as expected: %v", r) - }() - } got, err := NewClient(tt.m) - if !tt.expectPanic { - require.NoError(t, err) - require.NotNil(t, got) - ... - } + if tt.expectErr { + require.Error(t, err) + require.Nil(t, got) + return + } + require.NoError(t, err) + require.NotNil(t, got) + ...// pkg/controller/common/client.go func NewClient(m manager.Manager) (CtrlClient, error) { if m == nil { return nil, fmt.Errorf("nil manager") } return &ctrlClientImpl{Client: m.GetClient()}, nil }#!/bin/bash set -euo pipefail # Verify NewClient currently dereferences manager without nil guard. rg -n --type=go 'func\s+NewClient\s*\(m manager\.Manager\)' pkg/controller/common/client.go -A8 -B2 # Verify the test currently expects panic for nil manager. rg -n --type=go 'expectPanic|recover\(|nil manager - GetClient panics' pkg/controller/common/client_test.go -A4 -B4As 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/common/client_test.go` around lines 121 - 145, The test and implementation should not require NewClient(m manager.Manager) to panic on nil; instead add a nil-guard in NewClient that returns (nil, fmt.Errorf("nil manager")) when m == nil and otherwise returns &ctrlClientImpl{Client: m.GetClient()}, nil; then update the test case currently named "nil manager - GetClient panics" to expect an error (require.Error) and a nil result rather than deferring a recover, while leaving the non-nil assertions unchanged (require.NoError, require.NotNil, type assert to *ctrlClientImpl and verify the wrapped Client equals m.GetClient()).
🧹 Nitpick comments (3)
pkg/operator/assets/bindata_test.go (2)
59-64: Duplicate test case.This case tests the exact same input (
tokenRequestRBAsset) as the first test case at lines 29-34 with only a differentexpectedMinthreshold. Consider removing it or testing a distinct scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/assets/bindata_test.go` around lines 59 - 64, The test suite contains a duplicate test entry that uses the same input tokenRequestRBAsset as the earlier case but only changes expectedMin, so remove or replace this duplicate case: either delete the test case object that has name "boundary - single known asset from AssetNames" or change it to exercise a distinct scenario (e.g., use a different input variable than tokenRequestRBAsset or alter expectedMin and the input to validate the different threshold). Locate the duplicated test entry in the table of test cases (the struct literal containing name, input: tokenRequestRBAsset, expectedMin, expectError) and update or remove it accordingly so each test case is unique.
231-238: Unsafe type assertion may cause secondary panic.
r.(string)will panic if the recovered value is not a string. Usefmt.Sprintf("%v", r)or a type switch for robustness.♻️ Proposed fix
t.Run("panic on unknown asset", func(t *testing.T) { defer func() { r := recover() require.NotNil(t, r, "MustAsset must panic when asset not found") - assert.Contains(t, r.(string), "not found") + assert.Contains(t, fmt.Sprintf("%v", r), "not found") }() _ = MustAsset("nonexistent.yaml") })Note: Add
"fmt"to imports if applying this fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/assets/bindata_test.go` around lines 231 - 238, The test's deferred recover uses an unsafe type assertion r.(string) which can panic if the recovered value isn't a string; update the panic check in the t.Run "panic on unknown asset" to format the recovered value safely (e.g. use fmt.Sprintf("%v", r) or a type switch) when asserting the message contains "not found" and add "fmt" to imports if you choose fmt.Sprintf; keep the existing require.NotNil(t, r, ...) and assert.Contains call but pass the formatted string instead of r.(string), referencing the MustAsset call that triggers the panic.pkg/controller/deployment/credentials_request_test.go (1)
118-122: Avoid name-coupled test setup logicUsing
tt.name == "secret not found returns retry error"to drive fixture behavior is brittle and easy to break during case renames. Prefer an explicit scenario flag in the test table.Proposed refactor
tests := []struct { name string deploymentName string secretName string secretInStore bool + seedOtherSecret bool platformType configv1.PlatformType wantErr string @@ { name: "secret not found returns retry error", deploymentName: certmanagerControllerDeployment, secretName: "missing-secret", secretInStore: false, + seedOtherSecret: true, platformType: configv1.AWSPlatformType, wantErr: "Retrying", wantVolumes: 0, }, @@ - if tt.name == "secret not found returns retry error" { + if tt.seedOtherSecret { kubeClient = fake.NewSimpleClientset(&corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"}, }) }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/deployment/credentials_request_test.go` around lines 118 - 122, The test currently uses a brittle name check (tt.name == "secret not found returns retry error") to drive fixture setup; change the table-driven tests to include an explicit scenario flag (e.g., tt.secretMissing or tt.setupSecret func) and use that flag to decide the kubeClient fixture creation instead of comparing tt.name, update each test case to set the new flag for the secret-not-found scenario, and adjust the setup code that creates kubeClient (the fake.NewSimpleClientset(...) branch) to read tt.secretMissing/tt.setupSecret so renaming tt.name no longer affects behavior.
🤖 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/utils_test.go`:
- Line 124: The test uses an undefined function HasObjectChanged causing a
compile error; replace each call to HasObjectChanged(...) in
pkg/controller/common/utils_test.go with the existing helper
ObjectMetadataModified(...) (preserving the same arguments, e.g., change
HasObjectChanged(sa, cm) to ObjectMetadataModified(sa, cm)) for all occurrences
reported so the code references the defined function.
In `@pkg/controller/deployment/credentials_request_test.go`:
- Around line 190-194: The test currently skips asserting when tt.wantMountPath
is set but no VolumeMounts exist; update the test in credentials_request_test.go
to explicitly fail when wantMountPath != "" and
deployment.Spec.Template.Spec.Containers[0].VolumeMounts is empty (e.g., call
t.Fatalf or t.Errorf with a clear message about missing VolumeMount), then
proceed to check
deployment.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath ==
tt.wantMountPath as before; reference the tt.wantMountPath variable and the
deployment.Spec.Template.Spec.Containers[0].VolumeMounts slice when adding the
presence assertion.
- Around line 171-177: The test currently treats any NotFound error as
acceptable for all failure cases, which can mask incorrect behavior; update the
assertion logic in the test block that checks tt.wantErr to only accept
apierrors.IsNotFound when the test case explicitly expects a NotFound (e.g., add
a test-case field like wantNotFound or encode "NotFound" in tt.wantErr),
otherwise require the returned error string to contain tt.wantErr; modify the
conditional that references err, tt.wantErr and apierrors.IsNotFound so that
apierrors.IsNotFound is only considered a pass when the test case indicates
NotFound is the expected outcome (locate and change the assertion around
tt.wantErr, err and apierrors.IsNotFound in credentials_request_test.go).
In `@pkg/controller/trustmanager/utils_test.go`:
- Around line 215-227: The test case uses tm as &v1alpha1.TrustManager but the
table expects a *trustManagerBuilder and later calls tt.tm.Build(), and wantErr
is declared as a string but given a bool; fix by replacing the literal tm value
with a call to testTrustManager(...) that returns a *trustManagerBuilder (so
tt.tm is the correct type and tt.tm.Build() compiles), and change the wantErr
field to the correct bool type (or adjust the table's wantErr to bool) so the
test case types match the struct declaration; ensure references to Build() and
the trustManagerBuilder type remain consistent.
---
Duplicate comments:
In `@pkg/controller/common/client_test.go`:
- Around line 121-145: The test and implementation should not require
NewClient(m manager.Manager) to panic on nil; instead add a nil-guard in
NewClient that returns (nil, fmt.Errorf("nil manager")) when m == nil and
otherwise returns &ctrlClientImpl{Client: m.GetClient()}, nil; then update the
test case currently named "nil manager - GetClient panics" to expect an error
(require.Error) and a nil result rather than deferring a recover, while leaving
the non-nil assertions unchanged (require.NoError, require.NotNil, type assert
to *ctrlClientImpl and verify the wrapped Client equals m.GetClient()).
---
Nitpick comments:
In `@pkg/controller/deployment/credentials_request_test.go`:
- Around line 118-122: The test currently uses a brittle name check (tt.name ==
"secret not found returns retry error") to drive fixture setup; change the
table-driven tests to include an explicit scenario flag (e.g., tt.secretMissing
or tt.setupSecret func) and use that flag to decide the kubeClient fixture
creation instead of comparing tt.name, update each test case to set the new flag
for the secret-not-found scenario, and adjust the setup code that creates
kubeClient (the fake.NewSimpleClientset(...) branch) to read
tt.secretMissing/tt.setupSecret so renaming tt.name no longer affects behavior.
In `@pkg/operator/assets/bindata_test.go`:
- Around line 59-64: The test suite contains a duplicate test entry that uses
the same input tokenRequestRBAsset as the earlier case but only changes
expectedMin, so remove or replace this duplicate case: either delete the test
case object that has name "boundary - single known asset from AssetNames" or
change it to exercise a distinct scenario (e.g., use a different input variable
than tokenRequestRBAsset or alter expectedMin and the input to validate the
different threshold). Locate the duplicated test entry in the table of test
cases (the struct literal containing name, input: tokenRequestRBAsset,
expectedMin, expectError) and update or remove it accordingly so each test case
is unique.
- Around line 231-238: The test's deferred recover uses an unsafe type assertion
r.(string) which can panic if the recovered value isn't a string; update the
panic check in the t.Run "panic on unknown asset" to format the recovered value
safely (e.g. use fmt.Sprintf("%v", r) or a type switch) when asserting the
message contains "not found" and add "fmt" to imports if you choose fmt.Sprintf;
keep the existing require.NotNil(t, r, ...) and assert.Contains call but pass
the formatted string instead of r.(string), referencing the MustAsset call that
triggers the panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52f733c1-5fd5-4277-9e5d-0eae7c9f6657
📒 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
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/applyconfigurations/utils_test.go
- pkg/controller/deployment/deployment_overrides_test.go
…lder - Use ObjectMetadataModified in common utils tests (HasObjectChanged does not exist). - Fix ValidateTrustManagerConfig case to use trustManagerBuilder and drop invalid wantErr. Made-with: Cursor
Return (nil, error) from NewClient when manager is nil instead of panicking. Tighten withCloudCredentials tests: decoy-secret flag, NotFound only when expected, required VolumeMounts when wantMountPath is set. Remove duplicate bindata Asset case; format MustAsset panic recovery safely. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/deployment/credentials_request_test.go (1)
127-140: Decouple fixture setup from expected outcomeUsing
tt.wantErr != ""to decide informer seeding/sync makes test setup implicit and brittle. Prefer explicit fixture flags (e.g.,seedSecretInformer,waitForSecretSync) so new error cases don’t accidentally get a pre-seeded secret.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/deployment/credentials_request_test.go` around lines 127 - 140, The test currently uses tt.wantErr != "" to decide seeding and cache sync which couples expected outcome to fixture setup; update the table-driven tests to include explicit boolean flags (e.g., seedSecretInformer and waitForSecretSync) and replace uses of tt.wantErr != "" in the setup block and cache sync condition with those flags; specifically modify the setup around kubeInformers.Core().V1().Secrets().Informer().GetStore().Add(...) and the cache.WaitForCacheSync(...) call so they check tt.seedSecretInformer and tt.waitForSecretSync respectively, and ensure the decoySecretOnly logic still applies when seedSecretInformer is true.
🤖 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/deployment/credentials_request_test.go`:
- Around line 178-185: The test currently treats any NotFound error as
acceptable when tt.wantNotFoundOK is true, which can mask unrelated NotFounds;
update the logic in the error-checking block (where matchSubstring,
matchNotFound, apierrors.IsNotFound(err), tt.wantNotFoundOK and tt.wantErr are
referenced) to require both apierrors.IsNotFound(err) AND
strings.Contains(err.Error(), tt.wantErr) when tt.wantNotFoundOK is true (i.e.,
set matchNotFound = tt.wantNotFoundOK && apierrors.IsNotFound(err) &&
matchSubstring or adjust the overall if logic accordingly), and update the
t.Errorf messages to reflect that the test expects a NotFound with the specified
substring.
---
Nitpick comments:
In `@pkg/controller/deployment/credentials_request_test.go`:
- Around line 127-140: The test currently uses tt.wantErr != "" to decide
seeding and cache sync which couples expected outcome to fixture setup; update
the table-driven tests to include explicit boolean flags (e.g.,
seedSecretInformer and waitForSecretSync) and replace uses of tt.wantErr != ""
in the setup block and cache sync condition with those flags; specifically
modify the setup around
kubeInformers.Core().V1().Secrets().Informer().GetStore().Add(...) and the
cache.WaitForCacheSync(...) call so they check tt.seedSecretInformer and
tt.waitForSecretSync respectively, and ensure the decoySecretOnly logic still
applies when seedSecretInformer is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f5fbb24-94ee-4da1-bcc4-40a5ee1b7157
📒 Files selected for processing (4)
pkg/controller/common/client.gopkg/controller/common/client_test.gopkg/controller/deployment/credentials_request_test.gopkg/operator/assets/bindata_test.go
When wantNotFoundOK is set, pass only if apierrors.IsNotFound(err) and the error message contains wantErr, avoiding unrelated NotFound matches. Made-with: Cursor
There was a problem hiding this comment.
Do we need to concern UT coverage under path pkg/operator/applyconfigurations/ or pkg/operator/assets/ at all? They are auto-generated code
There was a problem hiding this comment.
I have removed the testcases for these.
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
/retest |
Drop tests that only exercised go-bindata and applyconfiguration-gen output. Coverage should focus on hand-written operator logic; generated packages are not maintained as test targets. Made-with: Cursor
|
/lgtm |
openshift-ci
bot
added
lgtm
The manager is always non-nil when controllers construct the client; removing the guard simplifies the code path for test coverage. Made-with: Cursor
anandkuma77
force-pushed
the
CM-902-add-testcases
branch
from
1b9bc8a to
c09cebd
Compare
|
@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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anandkuma77, arun717, bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/label px-approved |
|
/label docs-approved |
|
/px-approved |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
5 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.