CNTRLPLANE-3899: Add shared cmd/ unit tests for create nodepool agent - #9162
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mehabhalodiya: This pull request references CNTRLPLANE-3899 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 task 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. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughRefactored agent nodepool create command construction so Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (1)
cmd/nodepool/agent/create_test.go (1)
24-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required test-description format consistently.
cmd/nodepool/agent/create_test.go#L24-L37: rename each table case to “When … it should …”.cmd/nodepool/agent/create_test.go#L78-L105: rename each table case to “When … it should …”.cmd/nodepool/agent/create_test.go#L158-L158: rename the test to include a “When … it should …” clause.As per coding guidelines, “Always use "When ... it should ..." format for describing test cases when creating unit tests.”
🤖 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 `@cmd/nodepool/agent/create_test.go` around lines 24 - 37, Rename every table-driven test case in cmd/nodepool/agent/create_test.go at lines 24-37 and 78-105 to consistently follow the “When … it should …” format, describing the setup and expected behavior. Rename the test at line 158 to include the same “When … it should …” clause; no behavioral changes are needed.Source: Coding guidelines
🤖 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 `@cmd/nodepool/agent/create_test.go`:
- Around line 135-155: Update
TestUpdateNodePool_When_invalid_label_selector_is_provided_it_should_panic to
capture the error returned by AgentPlatformCreateOptions.UpdateNodePool instead
of discarding it, and include that error in the failure report when recover
returns nil; preserve the existing panic assertion behavior.
- Around line 43-51: Update the test around NewCreateCommand so it uses the
AgentPlatformCreateOptions instance wired by the command instead of constructing
a separate instance and manually copying agentLabelSelector. Ensure the test
parses the bound Cobra flags and validates the resulting command options,
exercising NewCreateCommand’s actual flag wiring.
---
Nitpick comments:
In `@cmd/nodepool/agent/create_test.go`:
- Around line 24-37: Rename every table-driven test case in
cmd/nodepool/agent/create_test.go at lines 24-37 and 78-105 to consistently
follow the “When … it should …” format, describing the setup and expected
behavior. Rename the test at line 158 to include the same “When … it should …”
clause; no behavioral changes are needed.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3dfa70b5-073b-4694-8c14-1a74f79e06d0
⛔ Files ignored due to path filters (3)
cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_empty_label_selector.yamlis excluded by!**/testdata/**cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_multi_label_selector.yamlis excluded by!**/testdata/**cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_single_label_selector.yamlis excluded by!**/testdata/**
📒 Files selected for processing (1)
cmd/nodepool/agent/create_test.go
openshift-ci
Bot
added
area/cli
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9162 +/- ##
==========================================
+ Coverage 45.74% 45.77% +0.03%
==========================================
Files 781 781
Lines 97858 97861 +3
==========================================
+ Hits 44765 44796 +31
+ Misses 50024 49996 -28
Partials 3069 3069
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
8c36439 to
64bfa61
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/nodepool/agent/create_test.go (1)
23-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required subtest description format.
Rename these cases—and the cases in the selector-translation table—to
When ... it should ...descriptions.As per coding guidelines, “Always use
When ... it should ...format for describing test cases when creating unit tests.”🤖 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 `@cmd/nodepool/agent/create_test.go` around lines 23 - 35, Rename the test cases in the selector-related table, including the selector-translation table, to follow the required “When ... it should ...” description format. Update each existing case description while preserving its arguments and expected behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cmd/nodepool/agent/create_test.go`:
- Around line 23-35: Rename the test cases in the selector-related table,
including the selector-translation table, to follow the required “When ... it
should ...” description format. Update each existing case description while
preserving its arguments and expected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0d47374c-1515-49d0-9532-5e2049d6b0a0
⛔ Files ignored due to path filters (3)
cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_empty_label_selector.yamlis excluded by!**/testdata/**cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_multi_label_selector.yamlis excluded by!**/testdata/**cmd/nodepool/agent/testdata/zz_fixture_TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool_single_label_selector.yamlis excluded by!**/testdata/**
📒 Files selected for processing (1)
cmd/nodepool/agent/create_test.go
There was a problem hiding this comment.
HyperShift Staff Engineer Agent Review
Standards: 4 hard violations (test naming x3, flag wiring bypass) + 2 judgement calls.
Spec: 1 partial (template deviation) + 1 questionable (panic enshrinement).
Worst issue on Standards: the flag-wiring test bypasses the actual binding and would miss real regressions. Worst issue on Spec: the fixture test only partially follows the template pattern.
Details in inline comments below.
64bfa61 to
673b347
Compare
673b347 to
e51854d
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/test e2e-v2-gke |
|
/verified by unit-tests |
openshift-ci-robot
added
the
verified
|
@mehabhalodiya: This PR has been marked as verified by 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. |
|
@bryan-cox all test passed! PTAL! Thank you! |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Thanks for adding coverage here — the Agent create path genuinely needed it, and the test cases themselves (empty/equality/set-based/invalid selectors, Type()) are the right scenarios with real-world values. Two axes of feedback:
Standards (TESTING.md)
- Test function names carry a
When…, it should…clause, which the standard reserves for test cases. Function names must beTest<FunctionName>mapping 1:1 to the function under test. Please collapse toTestNewCreateCommand, a table-drivenTestUpdateNodePool(selector + panic cases), andTestType. TestNewCreateCommand_When_flags_are_parsed…actually exercisesUpdateNodePool(it asserts onnodePool.Spec.Platform.Agent), so the name points at the wrong function. Move it underTestUpdateNodePool.- Minor: the two
UpdateNodePool-driven tests duplicate identical setup — a sharednodePoolfixture helper would remove the repetition.
Spec (CNTRLPLANE-3899 / PR description)
- The AC names test #1
TestCreateNodePool_…("the exact function name from the acceptance criteria") and test #4TestType_it_should_return_AgentPlatform; the committed names differ, and the testdata fixtures encode the divergent name. Please reconcile the code with the AC (or update the AC if it's stale). - Scope vs. description: the PR says "4 functions, 7 subtests" and "New file: create_test.go", but the diff has 5 functions / 6 subtests and also refactors
create.go(extractingnewCreateCommandWithOpts()). The refactor is a fine testability seam — just call it out in the description so scope matches.
Behaviorally the tests look correct; the requested changes are naming/scope alignment rather than logic. Happy to re-review once the function names follow TESTING.md and the description/AC are reconciled.
| return cmd | ||
| } | ||
|
|
||
| func newCreateCommandWithOpts() (*cobra.Command, *AgentPlatformCreateOptions) { |
There was a problem hiding this comment.
Reasonable seam to make the command testable — no change requested. One note: the UpdateNodePool-driven tests reach through this helper mainly to re-assert coverage that a direct TestUpdateNodePool already gives. If you consolidate the tests as suggested, TestNewCreateCommand only needs to cover command wiring (Use/RunE/flag registration), so this split stays justified and minimal.
The Agent nodepool create has its own platform-specific options and validation logic that is completely untested. Signed-off-by: mehabhalodiya <mehabhalodiya@gmail.com>
e51854d to
a881c6a
Compare
openshift-ci-robot
removed
the
verified
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, mehabhalodiya 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 |
openshift-ci
Bot
added
the
approved
|
/test e2e-v2-gke |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/verified by unit tests |
openshift-ci-robot
added
the
verified
|
@mehabhalodiya: This PR has been marked as verified by 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. |
|
@mehabhalodiya: 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. |
4 participants
The Agent nodepool create has its own platform-specific options and validation logic that is completely untested.
What this PR does / why we need it:
New file:
cmd/nodepool/agent/create_test.goTests added (4 functions, 7 subtests):
TestCreateNodePool_When_flags_are_parsed_it_should_generate_correct_nodepool— fixture-based test with 3 cases (empty/single/multi label selector), matching the exact function name from the acceptance criteriaTestUpdateNodePool_When_label_selector_is_parsed_it_should_set_agent_platform— validates the parsed LabelSelector struct for equality-based, set-based, and empty selectorsTestUpdateNodePool_When_invalid_label_selector_is_provided_it_should_panic— tests the validation path (panic on invalid selector)TestType_it_should_return_AgentPlatform— verifies Type() returns AgentPlatformWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3899
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
notin), and invalid label selectors.