CNF-26153: Add unit tests across codebase to close coverage gaps - #461
CNF-26153: Add unit tests across codebase to close coverage gaps#461sebrandon1 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request explicitly references no 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. |
WalkthroughAdds broad unit-test coverage for cert-manager, common controller utilities, IstioCSR, trust-manager, feature gates, operator client behavior, finalizers, status handling, and cache selector configuration. ChangesCert-manager controller coverage
Common controller coverage
IstioCSR coverage
Trust-manager coverage
Feature and operator coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 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 |
![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
🧹 Nitpick comments (2)
pkg/controller/certmanager/deployment_helper_test.go (1)
946-1377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTriplicated informer/watch-reactor setup across three tests.
TestGetOverrideArgsFor,TestGetOverrideEnvFor, andTestGetOverridePodLabelsForeach re-implement the same ~90-line fake-clientset/watch-reactor/informer/channel setup and create-wait-assert-delete-wait loop. Extracting a shared helper would remove significant duplication and make future additions (e.g., a 4th override type) cheaper.🤖 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 `@pkg/controller/certmanager/deployment_helper_test.go` around lines 946 - 1377, The three override tests duplicate identical informer setup and create/delete event synchronization. Extract the shared fake client, watch reactor, informer, event channel, and test-case execution loop into a reusable helper, then have TestGetOverrideArgsFor, TestGetOverrideEnvFor, and TestGetOverridePodLabelsFor provide only their test data and override assertion logic while preserving existing error and cleanup behavior.pkg/controller/certmanager/related_images_test.go (1)
68-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
t.Setenvoveros.Setenv/defer os.Unsetenv.
t.Setenvauto-restores the env var even on failure and is the idiomatic Go testing pattern for this.♻️ Example for one case
- os.Setenv("RELATED_IMAGE_CERT_MANAGER_WEBHOOK", tt.envVarValue) - defer os.Unsetenv("RELATED_IMAGE_CERT_MANAGER_WEBHOOK") + t.Setenv("RELATED_IMAGE_CERT_MANAGER_WEBHOOK", tt.envVarValue)Also applies to: 99-100, 130-131, 141-142
🤖 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 `@pkg/controller/certmanager/related_images_test.go` around lines 68 - 69, Replace the os.Setenv/defer os.Unsetenv calls in the related image test cases with t.Setenv, including the cases around the existing environment-variable setup blocks, so each test automatically restores RELATED_IMAGE_CERT_MANAGER_WEBHOOK.
🤖 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 `@pkg/controller/istiocsr/utils_test.go`:
- Around line 199-211: Remove the non-asserting checkBuggyAggregate logging
block, or replace it with a real assertion that verifies the aggregate includes
“original reconcile error” when prependErr is set and the status update fails.
Keep the existing wantErrMsg validation as the primary regression check and
eliminate misleading t.Log-only behavior.
---
Nitpick comments:
In `@pkg/controller/certmanager/deployment_helper_test.go`:
- Around line 946-1377: The three override tests duplicate identical informer
setup and create/delete event synchronization. Extract the shared fake client,
watch reactor, informer, event channel, and test-case execution loop into a
reusable helper, then have TestGetOverrideArgsFor, TestGetOverrideEnvFor, and
TestGetOverridePodLabelsFor provide only their test data and override assertion
logic while preserving existing error and cleanup behavior.
In `@pkg/controller/certmanager/related_images_test.go`:
- Around line 68-69: Replace the os.Setenv/defer os.Unsetenv calls in the
related image test cases with t.Setenv, including the cases around the existing
environment-variable setup blocks, so each test automatically restores
RELATED_IMAGE_CERT_MANAGER_WEBHOOK.
🪄 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: 695c0df9-44c0-4988-8185-da9798463447
📒 Files selected for processing (18)
pkg/controller/certmanager/cert_manager_networkpolicy_test.gopkg/controller/certmanager/default_cert_manager_controller_test.gopkg/controller/certmanager/deployment_helper_test.gopkg/controller/certmanager/deployment_log_level_test.gopkg/controller/certmanager/deployment_overrides_test.gopkg/controller/certmanager/related_images_test.gopkg/controller/common/client_test.gopkg/controller/common/reconcile_result_test.gopkg/controller/common/utils_test.gopkg/controller/common/validation_test.gopkg/controller/istiocsr/networkpolicies_test.gopkg/controller/istiocsr/utils_test.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/utils_test.gopkg/controller/trustmanager/webhooks_test.gopkg/features/features_test.gopkg/operator/operatorclient/operatorclient_test.gopkg/operator/setup_manager_test.go
9beb5eb to
f945972
Compare
openshift-ci
Bot
removed
the
needs-rebase
sebrandon1
changed the title
|
@sebrandon1: This pull request references CNF-26153 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 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. |
f945972 to
25a087d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/controller/istiocsr/networkpolicies_test.go (1)
75-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for
istioCSRCreateRecon: true.Every case in
TestCreateOrUpdateNetworkPolicyleavesistioCSRCreateReconat its zero value (false). Add a case withistioCSRCreateRecon: trueto cover the reconciliation-creation code path ofcreateOrUpdateNetworkPolicy, if that flag affects behavior.🤖 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 `@pkg/controller/istiocsr/networkpolicies_test.go` around lines 75 - 208, Add a table entry in TestCreateOrUpdateNetworkPolicy with istioCSRCreateRecon set to true, configuring the fake client and assertions to exercise the reconciliation-creation path of createOrUpdateNetworkPolicy. Verify the expected create/update behavior for this flag while preserving the existing cases.pkg/controller/common/client_test.go (1)
143-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared
mockClient/mockStatusClienttest doubles.The graph context shows an essentially identical
mockClient/mockStatusClientpair (sameGet,Create,Update,Delete,List,Patch,Statusstructure) already exists inpkg/controller/certmanager/deployment_helper_test.go. Duplicating this scaffolding across packages increases the maintenance burden: any change to theclient.Clientinterface signature requires updating every copy.Move
mockClientandmockStatusClientinto a shared internal test helper package (for examplepkg/controller/common/testutilor a smallfakespackage) that bothpkg/controller/certmanagerandpkg/controller/commontests import.🤖 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 `@pkg/controller/common/client_test.go` around lines 143 - 231, Extract the duplicated mockClient and mockStatusClient implementations from pkg/controller/common and certmanager tests into a shared internal test helper or fakes package. Export the helper types and preserve their configurable callbacks and Client/Status method behavior, then update newCtrlClient and both test suites to use the shared definitions and remove the local copies.
🤖 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 `@pkg/controller/certmanager/deployment_helper_test.go`:
- Around line 950-1015: Refactor TestGetOverrideArgsFor, TestGetOverrideEnvFor,
and TestGetOverridePodLabelsFor to reuse setupSyncedFakeCertManagerInformer and
withFakeCertManagerForTest instead of creating watchers, event handlers, and
create/delete flows inline. Remove the duplicated informer setup and channel
handling, preserving each test’s existing assertions and test data while relying
on the helpers’ checked AddEventHandler registration and tombstone-safe
DeleteFunc.
In `@pkg/operator/operatorclient/operatorclient_test.go`:
- Around line 124-126: Update the test comparison around json.Marshal to check
both error returns immediately, failing the test with the relevant error before
comparing got and want. Do not discard marshal errors, while preserving the
existing JSON string comparison in operatorclient tests.
---
Nitpick comments:
In `@pkg/controller/common/client_test.go`:
- Around line 143-231: Extract the duplicated mockClient and mockStatusClient
implementations from pkg/controller/common and certmanager tests into a shared
internal test helper or fakes package. Export the helper types and preserve
their configurable callbacks and Client/Status method behavior, then update
newCtrlClient and both test suites to use the shared definitions and remove the
local copies.
In `@pkg/controller/istiocsr/networkpolicies_test.go`:
- Around line 75-208: Add a table entry in TestCreateOrUpdateNetworkPolicy with
istioCSRCreateRecon set to true, configuring the fake client and assertions to
exercise the reconciliation-creation path of createOrUpdateNetworkPolicy. Verify
the expected create/update behavior for this flag while preserving the existing
cases.
🪄 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: 1bdc7251-3f53-4af1-bbe9-62ef5e2e8fa0
📒 Files selected for processing (18)
pkg/controller/certmanager/cert_manager_networkpolicy_test.gopkg/controller/certmanager/default_cert_manager_controller_test.gopkg/controller/certmanager/deployment_helper_test.gopkg/controller/certmanager/deployment_log_level_test.gopkg/controller/certmanager/deployment_overrides_test.gopkg/controller/certmanager/related_images_test.gopkg/controller/common/client_test.gopkg/controller/common/reconcile_result_test.gopkg/controller/common/utils_test.gopkg/controller/common/validation_test.gopkg/controller/istiocsr/networkpolicies_test.gopkg/controller/istiocsr/utils_test.gopkg/controller/trustmanager/deployments_test.gopkg/controller/trustmanager/utils_test.gopkg/controller/trustmanager/webhooks_test.gopkg/features/features_test.gopkg/operator/operatorclient/operatorclient_test.gopkg/operator/setup_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/controller/certmanager/deployment_overrides_test.go
- pkg/features/features_test.go
- pkg/controller/certmanager/default_cert_manager_controller_test.go
- pkg/controller/trustmanager/webhooks_test.go
- pkg/controller/trustmanager/deployments_test.go
- pkg/operator/setup_manager_test.go
- pkg/controller/common/reconcile_result_test.go
- pkg/controller/trustmanager/utils_test.go
- pkg/controller/certmanager/related_images_test.go
- pkg/controller/common/utils_test.go
- pkg/controller/certmanager/deployment_log_level_test.go
- pkg/controller/istiocsr/utils_test.go
Full-codebase unit test audit identified 42 coverage gaps across 12
packages. This commit addresses the high and medium priority gaps
that can be covered without modifying production code.
New test files (11):
- pkg/controller/certmanager: network policy validation, default
CertManager controller, deployment log level hook
- pkg/controller/common: client methods (Exists, Get, Create, Update,
UpdateWithRetry, Patch, StatusUpdate), HandleReconcileResult,
validation functions, utility functions
- pkg/controller/istiocsr: network policy CRUD, validateIstioCSRConfig,
updateCondition
- pkg/operator/operatorclient: GetOperatorState, EnsureFinalizer,
RemoveFinalizer, ApplyOperatorStatus, GetUnsupportedConfigOverrides
- pkg/operator: buildCacheObjectList, addControllerCacheConfig,
findExistingCacheEntry
Modified test files (7):
- pkg/controller/certmanager: deployment helper override functions
(args, env, labels), unsupported overrides invalid JSON, related
images for webhook/cainjector/acmesolver
- pkg/controller/trustmanager: managedAnnotationsModified,
addFinalizer/removeFinalizer edge cases, updateStatus retry,
containerPortsMatch, readinessProbeModified, webhook rules and
AdmissionReviewVersions drift
- pkg/features: IsIstioCSRFeatureGateEnabled, SetupWithFlagValue
invalid flag
Notable finding: istiocsr utils.go:484 updateCondition aggregates
{err, errUpdate} instead of {prependErr, errUpdate}, losing the
original reconcile error when both fail. The http01proxy version
correctly uses prependErr. Test documents this bug.
25a087d to
8cedfe4
Compare
|
/retest |
|
@sebrandon1: The following test failed, say
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
Summary
Jira: CNF-26153
Full-codebase unit test audit identified 42 coverage gaps across 12 packages. This PR addresses the high and medium priority gaps, adding 3,337 lines of new test code across 18 files (11 new, 7 modified) without touching any production code.
New test coverage by package
Bug found
pkg/controller/istiocsr/utils.go:484--updateConditionaggregates{err, errUpdate}instead of{prependErr, errUpdate}, losing the original reconcile error when both the reconcile and the status update fail. The http01proxy version of the same function correctly usesprependErr. A test documents this bug.Test plan
make lintpasses (no new lint issues in test files)