CNTRLPLANE-3899: Add shared cmd/ unit tests for create nodepool agent - #9162
CNTRLPLANE-3899: Add shared cmd/ unit tests for create nodepool agent#9162mehabhalodiya 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
|
@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactored agent nodepool create command construction so Possibly related PRs
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mehabhalodiya 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9162 +/- ##
==========================================
+ Coverage 44.62% 44.80% +0.18%
==========================================
Files 775 775
Lines 97105 97213 +108
==========================================
+ Hits 43329 43555 +226
+ Misses 50781 50657 -124
- Partials 2995 3001 +6
... and 20 files with indirect coverage changes
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.
The Agent nodepool create has its own platform-specific options and validation logic that is completely untested. Signed-off-by: mehabhalodiya <mehabhalodiya@gmail.com>
64bfa61 to
673b347
Compare
|
@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. |
3 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