NO-JIRA: Add unit tests across codebase to close coverage gaps#461
Open
sebrandon1 wants to merge 1 commit into
Open
NO-JIRA: Add unit tests across codebase to close coverage gaps#461sebrandon1 wants to merge 1 commit into
sebrandon1 wants to merge 1 commit into
Conversation
Contributor
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (12)
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
Contributor
|
[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
Comment on lines
+199
to
+211
| // Document the bug: when prependErr is non-nil and status update fails, | ||
| // the aggregate should contain prependErr but doesn't due to the bug. | ||
| if tt.checkBuggyAggregate { | ||
| errStr := err.Error() | ||
| // The correct behavior would include "original reconcile error" in the aggregate. | ||
| // Due to the bug (using `err` instead of `prependErr`), it does NOT. | ||
| if strings.Contains(errStr, "original reconcile error") { | ||
| // If this assertion fires, the bug has been fixed -- update this test. | ||
| t.Log("NOTE: updateCondition now correctly includes prependErr in aggregate -- bug is fixed") | ||
| } else { | ||
| t.Log("KNOWN BUG: updateCondition uses `err` instead of `prependErr` in aggregate (istiocsr utils.go line 484)") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
checkBuggyAggregate block never actually asserts anything.
Both branches (205-207 and 208-210) only call t.Log; neither calls t.Error/t.Fatal. The comment says "if this assertion fires, the bug has been fixed -- update this test," but there is no assertion that can fire — the test will pass identically whether or not prependErr ends up in the aggregate. The real signal for this case already comes from the wantErrMsg check at Lines 195-197, so this block adds no regression protection and is misleading about its own purpose.
🐛 Make the documented-bug check actually assert
if tt.checkBuggyAggregate {
errStr := err.Error()
- // The correct behavior would include "original reconcile error" in the aggregate.
- // Due to the bug (using `err` instead of `prependErr`), it does NOT.
- if strings.Contains(errStr, "original reconcile error") {
- // If this assertion fires, the bug has been fixed -- update this test.
- t.Log("NOTE: updateCondition now correctly includes prependErr in aggregate -- bug is fixed")
- } else {
- t.Log("KNOWN BUG: updateCondition uses `err` instead of `prependErr` in aggregate (istiocsr utils.go line 484)")
- }
+ // This currently documents a known bug; once fixed, the aggregate
+ // SHOULD contain the original prependErr. Fail loudly if that
+ // happens so this test gets updated instead of silently passing.
+ if strings.Contains(errStr, "original reconcile error") {
+ t.Fatal("updateCondition now includes prependErr in the aggregate -- bug fixed, update this test's expectations")
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Document the bug: when prependErr is non-nil and status update fails, | |
| // the aggregate should contain prependErr but doesn't due to the bug. | |
| if tt.checkBuggyAggregate { | |
| errStr := err.Error() | |
| // The correct behavior would include "original reconcile error" in the aggregate. | |
| // Due to the bug (using `err` instead of `prependErr`), it does NOT. | |
| if strings.Contains(errStr, "original reconcile error") { | |
| // If this assertion fires, the bug has been fixed -- update this test. | |
| t.Log("NOTE: updateCondition now correctly includes prependErr in aggregate -- bug is fixed") | |
| } else { | |
| t.Log("KNOWN BUG: updateCondition uses `err` instead of `prependErr` in aggregate (istiocsr utils.go line 484)") | |
| } | |
| } | |
| if tt.checkBuggyAggregate { | |
| errStr := err.Error() | |
| // This currently documents a known bug; once fixed, the aggregate | |
| // SHOULD contain the original prependErr. Fail loudly if that | |
| // happens so this test gets updated instead of silently passing. | |
| if strings.Contains(errStr, "original reconcile error") { | |
| t.Fatal("updateCondition now includes prependErr in the aggregate -- bug fixed, update this test's expectations") | |
| } | |
| } |
🤖 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/utils_test.go` around lines 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.
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.
sebrandon1
force-pushed
the
unit-test-coverage-gaps
branch
from
July 24, 2026 02:31
9beb5eb to
f945972
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
@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. |
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
2 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.
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