test: add mock test for system operations on client side #1015
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1015 +/- ##
==========================================
+ Coverage 14.91% 15.18% +0.27%
==========================================
Files 133 135 +2
Lines 8483 8483
==========================================
+ Hits 1265 1288 +23
+ Misses 7118 7094 -24
- Partials 100 101 +1
Continue to review full report at Codecov.
|
|
PTAL @allencloud |
client/system_version_test.go
Outdated
| @@ -30,11 +30,11 @@ func TestSystemVersion(t *testing.T) { | |||
| if !strings.HasPrefix(req.URL.Path, expectedURL) { | |||
| return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL) | |||
| } | |||
| version := types.SystemVersion{ | |||
| versionConfig := types.SystemVersion{ | |||
There was a problem hiding this comment.
I do not think this is a config, version is just OK for me.
There was a problem hiding this comment.
The config stuff is only for post request, right?
There was a problem hiding this comment.
Yes, config is usually stored in the post request body. While this part of code is simulating a server which is returning a http response. And for this API, it returns types.SystemVersion.
client/registry_login.go
Outdated
| "github.com/alibaba/pouch/apis/types" | ||
| ) | ||
|
|
||
| // RegistryLogin requests a registry server to login. |
There was a problem hiding this comment.
This comment is quite confusing. Could we change this?
client/registry_login_test.go
Outdated
| if !strings.HasPrefix(req.URL.Path, expectedURL) { | ||
| return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL) | ||
| } | ||
| loginConfig := types.AuthConfig{ |
There was a problem hiding this comment.
I see that in the test you input AuthConfig in client.RegistryLogin(context.Background(), &types.AuthConfig{Username: "user_name", Password: "123456"}). Why do you construct a new one in this place?
And actually this function would return a struct named AuthResponse, which is defined :
type AuthResponse struct {
// An opaque token used to authenticate a user after a successful login
IdentityToken string `json:"IdentityToken,omitempty"`
// The status of the authentication
// Required: true
Status string `json:"Status"`
}
Please change this part. Thanks a lot.
There was a problem hiding this comment.
I see. I mixed the get request with post request again. Will change it ASAP.
ZouRui89
changed the title
|
LGTM |
pouchrobot
added
the
LGTM
| // RegistryLogin authenticates the server with a given registry to login. | ||
| func (client *APIClient) RegistryLogin(ctx context.Context, auth *types.AuthConfig) (*types.AuthResponse, error) { | ||
| resp, err := client.post(ctx, "/auth", nil, auth, nil) | ||
| if err != nil || resp.StatusCode == http.StatusUnauthorized { |
There was a problem hiding this comment.
This part has a potential logic bug which is already described in #1030.
client/system_info_test.go
Outdated
| HTTPCli: httpClient, | ||
| } | ||
|
|
||
| if infoRes, err := client.SystemInfo(context.Background()); err != nil { |
There was a problem hiding this comment.
Why do we assert values when err != nil? @ZouRui89
|
not-LGTM, unless update the err judging. |
Signed-off-by: Zou Rui <21751189@zju.edu.cn>
|
LGTM |
Signed-off-by: Zou Rui 21751189@zju.edu.cn
Ⅰ. Describe what this PR did
add all the mock for system operations on client side
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews