-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🌱 (cli; alpha commands): Add unit tests for alpha commands [WiP] #4931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
🌱 (cli; alpha commands): Add unit tests for alpha commands [WiP] #4931
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cmallikarjunah The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @cmallikarjunah! |
Hi @cmallikarjunah. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
||
When("NewScaffoldCommand", func() { | ||
It("Testing the NewScaffoldCommand", func() { | ||
cmd := NewScaffoldCommand() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alpha update command internal code it might be nice if we could cover something
However, if you find it too complicated, I suggest skipping pkg/cli/alpha and proceeding to the following directory. ;-)
We have a lot inside that we can add more to, and it is not alpha/experimental staff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I am working on the update internal code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tip:
You can validate the code locally with:
- make lint -> for linter check
- you can run make lint-fix : to fix automaticly issues ( not all is fixed but is very helpful )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you finish this one please remove WIP from the title so that we are aware of and we can review/get merged get done ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cmallikarjunah, I'm currently working on alpha internal update package, are you working on the same or is it different?. I wanted to clarify so that we dont duplicate the work :-)
https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/cli/alpha/internal/update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya it's the same @mayuka-c.
Was working on functions of prepare.go yesterday. Planning to continue.
There was no WiP or status for it so I picked it upon Camila's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no worries, you can continue
7c5b2d9
to
c072261
Compare
@@ -75,7 +75,7 @@ func (opts *Update) defineFromVersion(config store.Store) (string, error) { | |||
|
|||
func (opts *Update) defineToVersion() string { | |||
if len(opts.ToVersion) != 0 { | |||
if !strings.HasPrefix(opts.FromVersion, "v") { | |||
if !strings.HasPrefix(opts.ToVersion, "v") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please rebase with master?
So that we have no this change it is fixed and merged now 👍
Entry("options", &Update{ToVersion: "v1.1.0"}), | ||
Entry("options", &Update{FromVersion: "1.0.0"}), | ||
Entry("options", &Update{ToVersion: "1.1.0"}), | ||
Entry("options", &Update{}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reduce the number of test cases here?
Do we really need all of them to cover the intended scenarios?
It seems like we’re primarily validating support for:
- FromVersion and ToVersion with and without the v prefix
- Presence or absence of FromBranch
We’re not actually validating SemVer parsing here, so duplicating every combination may be unnecessary.
Maybe we could simplify this to focus on meaningful permutations only?
Issue: 4925
Adding unit tests