-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Strip the version prefix before calling semver #7054
Strip the version prefix before calling semver #7054
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afbjorklund The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #7054 +/- ##
==========================================
+ Coverage 36.94% 37.24% +0.29%
==========================================
Files 146 146
Lines 9094 9094
==========================================
+ Hits 3360 3387 +27
+ Misses 5322 5292 -30
- Partials 412 415 +3
|
Test seems to be working as intended, catches the previous error which was the idea:
It doesn't really verify the repository though, since it could still be defined as empty: // ImageRepositories contains all known image repositories
ImageRepositories = map[string][]string{
"global": {""},
"cn": {"registry.cn-hangzhou.aliyuncs.com/google_containers"},
} |
for _, test := range tests { | ||
t.Run(test.description, func(t *testing.T) { | ||
cmd := &cobra.Command{} | ||
viper.SetDefault(imageRepository, test.imageRepository) |
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.
Calling viper.Set in unit test has un-wanted side effects, such as setting the user 's settings and also can not run in parallel safely
Could we break down generateCfgFromFlags func so we can test only the mirror func without need to pass cobra ?
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.
No idea, think I only copied what the test above was doing or something
viper.SetDefault(kubernetesVersion, test.paramVersion)
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.
I think we can refactor our tests not to use viper as much as possible, we could parameterize the funcs into small independent testable units. what do u think? maybe good opprotunity to refactor this part of the code?
Needs a mirror country unit test, apparently.
Closes #7052