test: add test cases for diskquota_test.go #2638
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2638 +/- ##
=========================================
Coverage ? 43.38%
=========================================
Files ? 277
Lines ? 18720
Branches ? 0
=========================================
Hits ? 8121
Misses ? 9353
Partials ? 1246
|
apis/opts/diskquota.go
Outdated
| @@ -9,17 +9,21 @@ import ( | |||
| func ParseDiskQuota(quotas []string) (map[string]string, error) { | |||
| var quotaMaps = make(map[string]string) | |||
|
|
|||
| if quotas == nil { | |||
There was a problem hiding this comment.
Actually quotas is a slice of string, then I think using len(quota) == 0 will be better. Since then it will cover two cases:
- first, the slice is a nil;
- second, the slice is not nil, but an empty slice.
WDYT? @hellolijj
There was a problem hiding this comment.
A very good advise, i totally agree
apis/opts/diskquota.go
Outdated
| @@ -9,17 +9,21 @@ import ( | |||
| func ParseDiskQuota(quotas []string) (map[string]string, error) { | |||
| var quotaMaps = make(map[string]string) | |||
|
|
|||
| if quotas == nil { | |||
| return nil, fmt.Errorf("invalid format for disk quota: %s", quotas) | |||
There was a problem hiding this comment.
We are trying to build PouchContainer a user-friendly, operator-friendly software, so when returning error we should try to make the error message be much clearer. For this case, operators can only get the error with the message invalid, but he has no idea why this is invalid.
Thus from my point of view, maybe fmt.Errorf("invalid format for disk quota: quotas cannot be empty", quotas) will be better. In addition, we try to make quotas to be formatted is meaningless, because it is already nil. WDYT? @hellolijj
There was a problem hiding this comment.
Yeah, that's right!
apis/opts/diskquota.go
Outdated
| for _, quota := range quotas { | ||
| if quota == "" { | ||
| if strings.TrimSpace(quota) == "" { | ||
| return nil, fmt.Errorf("invalid format for disk quota: %s", quota) |
There was a problem hiding this comment.
same problem, we should add more concrete invalid reason here.
apis/opts/diskquota.go
Outdated
| case 2: | ||
| quotaMaps[parts[0]] = parts[1] | ||
| quotaMaps[strings.TrimSpace(parts[0])] = strings.TrimSpace(parts[1]) | ||
| default: | ||
| return nil, fmt.Errorf("invalid format for disk quota: %s", quota) |
There was a problem hiding this comment.
same problem, we should add more concrete invalid reason here.
There was a problem hiding this comment.
But, how can i test the case like follow? for example, the quota is []string{"foo = foo"}}
There was a problem hiding this comment.
You could test this to be []string{"a=b=c"}, then it is invalid.
apis/opts/diskquota_test.go
Outdated
| @@ -19,7 +19,7 @@ func TestParseDiskQuota(t *testing.T) { | |||
| {name: "test2", args: args{diskquota: []string{"foo=foo=foo"}}, want: nil, wantErr: true}, | |||
| {name: "test3", args: args{diskquota: []string{"foo"}}, want: map[string]string{"/": "foo"}, wantErr: false}, | |||
| {name: "test4", args: args{diskquota: []string{"foo=foo"}}, want: map[string]string{"foo": "foo"}, wantErr: false}, | |||
| {name: "test5", args: args{diskquota: []string{"foo = foo"}}, want: map[string]string{"foo": "foo"}, wantErr: true}, | |||
| {name: "test5", args: args{diskquota: []string{"foo= foo"}}, want: map[string]string{"foo": "foo"}, wantErr: false}, | |||
There was a problem hiding this comment.
it had better to unify key=value format, for instance, to remove useless whitespace.
There was a problem hiding this comment.
Yes, i thinks so.
So, do i delete this test case?
There was a problem hiding this comment.
This should also be a test case, I think ParseDiskQuota should test it valid or invalid whether has space in string slice @hellolijj @chuanchang
|
LGTM, but ci failed. |
pouchrobot
added
the
LGTM
|
@hellolijj Thanks for your contribution. 🍻 |
|
I have rebase the code and force-pushed it. @fuweid please help me to review it again 😄 |
Ⅰ. Describe what this PR did
test: add test cases for diskquota_test.go
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews