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
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.
9beb5eb to
f945972
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@sebrandon1: 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. |
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. |
2 participants
Summary
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 - updateCondition aggregates {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 uses prependErr. A test documents this bug.
Test plan
Summary by CodeRabbit