Add cmd ut #1765
Add cmd ut #1765
Conversation
| var i net.IP | ||
| for _, ip := range hosts { | ||
| for _, i = range ip.IPS { | ||
| } |
There was a problem hiding this comment.
The code has been updated
| "net" | ||
| "testing" | ||
|
|
||
| "github.com/sealerio/sealer/cmd/sealer/cmd/types" |
There was a problem hiding this comment.
usually, there should be three groups separated by blank for go-imports
the golang official package
the package for the current project
the others
cmd/sealer/cmd/utils/cluster_test.go
Outdated
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if res := removeIPList(tt.clusterIPList, tt.toBeDeletedIPList); (res != nil) != tt.wantErr { |
There was a problem hiding this comment.
I don't check the "removeIPList".
But I guess it aims to remove "toBeDeletedIPList" from "clusterIPList", and return the left "ip list"?
I think we could make the test more readable like this:
struct {
name string
IPList []net.IP
IPListToDelete []net.IP
IPListExpected []net.IP
}
Compare the "res := removeIPList" and "IPListExpected". Otherwise the test case is hard to be understood.
cmd/sealer/cmd/utils/cluster_test.go
Outdated
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if res := removeIPList(tt.clusterIPList, tt.toBeDeletedIPList); (res != nil) != tt.wantErr { | ||
| fmt.Println(res) |
cmd/sealer/cmd/utils/cluster_test.go
Outdated
| if res := removeIPList(tt.clusterIPList, tt.toBeDeletedIPList); (res != nil) != tt.wantErr { | ||
| fmt.Println(res) | ||
| } | ||
| logrus.Error("is empty") |
There was a problem hiding this comment.
t.Error here, and make the error message more detailed.
cmd/sealer/cmd/utils/cluster_test.go
Outdated
| assert.NotNil(t, res) | ||
| result := assert.Equal(t, tt.IPListExpected, res) | ||
| if !result { | ||
| t.Errorf("The filtered ip address is different from the expected value") |
cmd/sealer/cmd/utils/cluster_test.go
Outdated
| false, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if res := removeIPList(tt.clusterIPList, tt.toBeDeletedIPList); (res != nil) != tt.wantErr { | ||
| fmt.Println(res) | ||
| assert.NotNil(t, res) |
There was a problem hiding this comment.
"(res != nil) != tt.wantErr"
"assert.NotNil(t, res)"
Are them duplicated?
IMO, both of them are unnecessary.
Just do "assert.Equal(t, tt.IPListExpected, res)".
In addition, I guess "if !result " will not be executed if "assert" error happened?
There was a problem hiding this comment.
Yes, the judgment does repeat itself
Make changes to the reviewed questions: modify the return error message function
Stevent-fei
force-pushed
the
new_sealer
branch
from
c340207 to
759aa84
Compare
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
It's not simple enough. Let's finish the last change.
IMO, "(res != nil) != tt.wantErr" means if res != nil. In this case, wantErr should be removed.
And let's do like this:
- remove wantErr
res := removeIPList(tt.clusterIPList, tt.toBeDeletedIPList)
if res == nil {
continue
}
assert.Equal(t, tt.IPListExpected, res)
Or make the expected IPList to be nil
Then you don't have to make sure the res != nil.
There was a problem hiding this comment.
This judgment is inside a for loop, so you can't use continue?
Or make the expected IPList to be nil
Then you don't have to make sure the res ! = nil.
The meaning of this sentence is not quite understood
But my understanding is this:
If expected IPList is empty, an error will be reported when the res and expected IPList functions are detected
Codecov ReportBase: 19.89% // Head: 20.68% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
==========================================
+ Coverage 19.89% 20.68% +0.78%
==========================================
Files 69 69
Lines 6478 6478
==========================================
+ Hits 1289 1340 +51
+ Misses 5012 4957 -55
- Partials 177 181 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Describe what this PR does / why we need it
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews