OCPBUGS-62792: Add unit tests for pkg/controller/controllercmd/cmd.go#2182
OCPBUGS-62792: Add unit tests for pkg/controller/controllercmd/cmd.go#2182jubittajohn wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@jubittajohn: This pull request references Jira Issue OCPBUGS-62792, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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-bug
WalkthroughAdds a new test suite for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: wangke19. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jubittajohn 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 (1)
pkg/controller/controllercmd/cmd_test.go (1)
231-264: Add one malformed-config case to cover the error path.
expectErroris wired, but no test currently drivesConfig()parse failure. A bad YAML/object case would complete the table’s intent.➕ Example table entry
{ name: "valid config file", configContent: ` apiVersion: operator.openshift.io/v1alpha1 kind: GenericOperatorConfig servingInfo: bindAddress: https://0.0.0.0:8443 leaderElection: leaseDuration: 90s `, expectNilUnstr: false, validateConfig: func(t *testing.T, config *operatorv1alpha1.GenericOperatorConfig) { if config.ServingInfo.BindAddress != "https://0.0.0.0:8443" { t.Errorf("expected BindAddress to be parsed correctly, got %q", config.ServingInfo.BindAddress) } }, }, + { + name: "invalid config file", + configContent: ` +apiVersion: operator.openshift.io/v1alpha1 +kind: GenericOperatorConfig +servingInfo: + bindAddress: [not-a-string +`, + expectError: true, + expectNilUnstr: true, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/controllercmd/cmd_test.go` around lines 231 - 264, Add a new table entry in the tests slice that provides a malformed YAML/configContent to exercise the Config() parse-failure path: set expectError: true and expectNilUnstr: true (or nil result expectation), include a name like "malformed config file", and optionally a validateConfig that asserts config is nil or that an error was returned; this will ensure operatorv1alpha1.GenericOperatorConfig parsing in Config() fails and the test verifies the error path.
🤖 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/controllercmd/cmd_test.go`:
- Around line 15-27: The test can be made deterministic by forcing the code path
that generates a self-signed cert by ensuring the service-serving-cert directory
does not exist; set the ControllerCommandConfig instance to point to a
controlled non-existent cert directory (e.g. assign c.serviceServingCertDir or
equivalent field on ControllerCommandConfig to a temp path and os.RemoveAll that
path before the call) so AddDefaultRotationToConfig will always take the
self-signed 30-day branch rather than reading /var/run/secrets/serving-cert from
the environment.
---
Nitpick comments:
In `@pkg/controller/controllercmd/cmd_test.go`:
- Around line 231-264: Add a new table entry in the tests slice that provides a
malformed YAML/configContent to exercise the Config() parse-failure path: set
expectError: true and expectNilUnstr: true (or nil result expectation), include
a name like "malformed config file", and optionally a validateConfig that
asserts config is nil or that an error was returned; this will ensure
operatorv1alpha1.GenericOperatorConfig parsing in Config() fails and the test
verifies the error path.
🪄 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: e6782a23-dc95-4da7-8630-59ef42cfe3cb
📒 Files selected for processing (1)
pkg/controller/controllercmd/cmd_test.go
Signed-off-by: jubittajohn <jujohn@redhat.com>
jubittajohn
force-pushed
the
fix-makeServerCert-call-unit-test
branch
from
a9d93a8 to
81f99a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/controller/controllercmd/cmd_test.go (1)
145-211:⚠️ Potential issue | 🟡 MinorSelf-signed generation case can pass without exercising the self-signed branch.
The case at Line 145 claims self-signed generation, but the current assertions only require non-empty paths; it can still pass via the service-serving-cert branch. Consider gating this case (or asserting branch-specific output) so it actually validates self-signed behavior.
🔧 Suggested tightening
tests := []struct { name string configFile string existingCertFile string existingKeyFile string expectGenerated bool + requireSelfSigned bool expectObservedLen int }{ { name: "generates self-signed cert when none specified", expectGenerated: true, + requireSelfSigned: true, expectObservedLen: 2, // service cert paths }, @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.requireSelfSigned && hasServiceServingCerts("/var/run/secrets/serving-cert") { + t.Skip("service-serving certs present; self-signed branch is not exercised") + } tmpDir := t.TempDir()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/controllercmd/cmd_test.go` around lines 145 - 211, The "generates self-signed cert when none specified" test can pass without exercising the self-signed branch; tighten it by asserting the generated cert/key are the ones produced by AddDefaultRotationToConfig and are created under the test temp directory. Specifically, in the test case for ControllerCommandConfig.AddDefaultRotationToConfig (the test named "generates self-signed cert when none specified"), after calling AddDefaultRotationToConfig assert that config.ServingInfo.CertFile and KeyFile are non-empty, that observedFiles contains exactly those two paths, and that both paths have tmpDir as their directory prefix (e.g., strings.HasPrefix(path, tmpDir)) so the test cannot pass via pre-existing/static service-serving-cert paths. Ensure you reference ControllerCommandConfig, ControllerFlags and AddDefaultRotationToConfig when locating the code to change.
🧹 Nitpick comments (1)
pkg/controller/controllercmd/cmd_test.go (1)
236-241: Consider adding one malformed-config case to exercise the error path.
expectErroris wired into assertions, but there’s no table row that validates parse failure behavior yet.Also applies to: 289-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/controllercmd/cmd_test.go` around lines 236 - 241, Add a table-driven test case to exercise the parse-failure path by adding a row in the tests slice used in Test... (the tests variable defined with fields name, configContent, expectError, expectNilUnstr, validateConfig) that provides malformed YAML/JSON in configContent and sets expectError=true and expectNilUnstr=true; also add a corresponding malformed case in the other tests block at the second location (the block around the other tests slice referenced in the comment) so the assertion logic that checks expectError is executed and verifies the parser returns an error and a nil/unstructured config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controller/controllercmd/cmd_test.go`:
- Around line 145-211: The "generates self-signed cert when none specified" test
can pass without exercising the self-signed branch; tighten it by asserting the
generated cert/key are the ones produced by AddDefaultRotationToConfig and are
created under the test temp directory. Specifically, in the test case for
ControllerCommandConfig.AddDefaultRotationToConfig (the test named "generates
self-signed cert when none specified"), after calling AddDefaultRotationToConfig
assert that config.ServingInfo.CertFile and KeyFile are non-empty, that
observedFiles contains exactly those two paths, and that both paths have tmpDir
as their directory prefix (e.g., strings.HasPrefix(path, tmpDir)) so the test
cannot pass via pre-existing/static service-serving-cert paths. Ensure you
reference ControllerCommandConfig, ControllerFlags and
AddDefaultRotationToConfig when locating the code to change.
---
Nitpick comments:
In `@pkg/controller/controllercmd/cmd_test.go`:
- Around line 236-241: Add a table-driven test case to exercise the
parse-failure path by adding a row in the tests slice used in Test... (the tests
variable defined with fields name, configContent, expectError, expectNilUnstr,
validateConfig) that provides malformed YAML/JSON in configContent and sets
expectError=true and expectNilUnstr=true; also add a corresponding malformed
case in the other tests block at the second location (the block around the other
tests slice referenced in the comment) so the assertion logic that checks
expectError is executed and verifies the parser returns an error and a
nil/unstructured config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2f4d399e-e874-47a6-aae4-4f4afb0478b9
📒 Files selected for processing (1)
pkg/controller/controllercmd/cmd_test.go
|
@jubittajohn: 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
This unit test was written by Claude
Summary by CodeRabbit