From 84c956a1712d8d275e1263591802c40617005724 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 8 Aug 2023 17:31:03 +0200 Subject: [PATCH 1/2] cli/command/registry: cleanup search tests - TestSearchContext: don't use un-keyed structs - TestSearchContext: don't use CompareMultipleValues as it was not needed - TestSearchContextDescription: don't use un-keyed structs - TestSearchContextDescription: don't use CompareMultipleValues as it was not needed - TestSearchContextWrite: don't use un-keyed structs, and include the code-comments into the test-table as names for the tests to give them some context. - TestSearchContextWriteJSON and TestSearchContextWriteJSONField were not validating the output format, but validating if the JSON output could be marshalled back to a struct. Let's just role them into TestSearchContextWrite. Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/formatter_search_test.go | 272 +++++++++--------- 1 file changed, 139 insertions(+), 133 deletions(-) diff --git a/cli/command/registry/formatter_search_test.go b/cli/command/registry/formatter_search_test.go index 1f6906461c..c518f908f4 100644 --- a/cli/command/registry/formatter_search_test.go +++ b/cli/command/registry/formatter_search_test.go @@ -2,12 +2,9 @@ package registry import ( "bytes" - "encoding/json" - "strings" "testing" "github.com/docker/cli/cli/command/formatter" - "github.com/docker/cli/internal/test" registrytypes "github.com/docker/docker/api/types/registry" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -15,50 +12,67 @@ import ( ) func TestSearchContext(t *testing.T) { - name := "nginx" - starCount := 5000 - var ctx searchContext cases := []struct { searchCtx searchContext expValue string call func() string }{ - {searchContext{ - s: registrytypes.SearchResult{Name: name}, - }, name, ctx.Name}, - {searchContext{ - s: registrytypes.SearchResult{StarCount: starCount}, - }, "5000", ctx.StarCount}, - {searchContext{ - s: registrytypes.SearchResult{IsOfficial: true}, - }, "[OK]", ctx.IsOfficial}, - {searchContext{ - s: registrytypes.SearchResult{IsOfficial: false}, - }, "", ctx.IsOfficial}, - {searchContext{ - s: registrytypes.SearchResult{IsAutomated: true}, //nolint:staticcheck // ignore SA1019 (IsAutomated is deprecated). - }, "[OK]", ctx.IsAutomated}, - {searchContext{ - s: registrytypes.SearchResult{}, - }, "", ctx.IsAutomated}, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Name: "nginx"}, + }, + expValue: "nginx", + call: ctx.Name, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{StarCount: 5000}, + }, + expValue: "5000", + call: ctx.StarCount, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{IsOfficial: true}, + }, + expValue: "[OK]", + call: ctx.IsOfficial, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{IsOfficial: false}, + }, + call: ctx.IsOfficial, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{IsAutomated: true}, //nolint:staticcheck // ignore SA1019 (IsAutomated is deprecated). + }, + expValue: "[OK]", + call: ctx.IsAutomated, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{}, + }, + call: ctx.IsAutomated, + }, } for _, c := range cases { ctx = c.searchCtx v := c.call() - if strings.Contains(v, ",") { - test.CompareMultipleValues(t, v, c.expValue) - } else if v != c.expValue { - t.Fatalf("Expected %s, was %s\n", c.expValue, v) - } + assert.Check(t, is.Equal(v, c.expValue)) } } func TestSearchContextDescription(t *testing.T) { - shortDescription := "Official build of Nginx." - longDescription := "Automated Nginx reverse proxy for docker containers" - descriptionWReturns := "Automated\nNginx reverse\rproxy\rfor docker\ncontainers" + const ( + shortDescription = "Official build of Nginx." + longDescription = "Automated Nginx reverse proxy for docker containers" + descriptionWReturns = "Automated\nNginx reverse\rproxy\rfor docker\ncontainers" + ) var ctx searchContext cases := []struct { @@ -66,80 +80,118 @@ func TestSearchContextDescription(t *testing.T) { expValue string call func() string }{ - {searchContext{ - s: registrytypes.SearchResult{Description: shortDescription}, - trunc: true, - }, shortDescription, ctx.Description}, - {searchContext{ - s: registrytypes.SearchResult{Description: shortDescription}, - trunc: false, - }, shortDescription, ctx.Description}, - {searchContext{ - s: registrytypes.SearchResult{Description: longDescription}, - trunc: false, - }, longDescription, ctx.Description}, - {searchContext{ - s: registrytypes.SearchResult{Description: longDescription}, - trunc: true, - }, formatter.Ellipsis(longDescription, 45), ctx.Description}, - {searchContext{ - s: registrytypes.SearchResult{Description: descriptionWReturns}, - trunc: false, - }, longDescription, ctx.Description}, - {searchContext{ - s: registrytypes.SearchResult{Description: descriptionWReturns}, - trunc: true, - }, formatter.Ellipsis(longDescription, 45), ctx.Description}, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Description: shortDescription}, + trunc: true, + }, + expValue: shortDescription, + call: ctx.Description, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Description: shortDescription}, + trunc: false, + }, + expValue: shortDescription, + call: ctx.Description, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Description: longDescription}, + trunc: false, + }, + expValue: longDescription, + call: ctx.Description, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Description: longDescription}, + trunc: true, + }, + expValue: formatter.Ellipsis(longDescription, 45), + call: ctx.Description, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Description: descriptionWReturns}, + trunc: false, + }, + expValue: longDescription, + call: ctx.Description, + }, + { + searchCtx: searchContext{ + s: registrytypes.SearchResult{Description: descriptionWReturns}, + trunc: true, + }, + expValue: formatter.Ellipsis(longDescription, 45), + call: ctx.Description, + }, } for _, c := range cases { ctx = c.searchCtx v := c.call() - if strings.Contains(v, ",") { - test.CompareMultipleValues(t, v, c.expValue) - } else if v != c.expValue { - t.Fatalf("Expected %s, was %s\n", c.expValue, v) - } + assert.Check(t, is.Equal(v, c.expValue)) } } func TestSearchContextWrite(t *testing.T) { cases := []struct { - context formatter.Context - expected string + doc string + format formatter.Format + expected string + expectedErr string }{ - // Errors { - formatter.Context{Format: "{{InvalidFunction}}"}, - `template parsing error: template: :1: function "InvalidFunction" not defined`, + doc: "Errors", + format: "{{InvalidFunction}}", + expectedErr: `template parsing error: template: :1: function "InvalidFunction" not defined`, }, { - formatter.Context{Format: "{{nil}}"}, - `template parsing error: template: :1:2: executing "" at : nil is not a command`, - }, - // Table format - { - formatter.Context{Format: NewSearchFormat("table")}, - string(golden.Get(t, "search-context-write-table.golden")), + doc: "Nil format", + format: "{{nil}}", + expectedErr: `template parsing error: template: :1:2: executing "" at : nil is not a command`, }, { - formatter.Context{Format: NewSearchFormat("table {{.Name}}")}, - `NAME + doc: "JSON format", + format: "{{json .}}", + expected: `{"Description":"Official build","IsAutomated":"false","IsOfficial":"true","Name":"result1","StarCount":"5000"} +{"Description":"Not official","IsAutomated":"true","IsOfficial":"false","Name":"result2","StarCount":"5"} +`, + }, + { + doc: "JSON format, single field", + format: "{{json .Name}}", + expected: `"result1" +"result2" +`, + }, + { + doc: "Table format", + format: NewSearchFormat("table"), + expected: string(golden.Get(t, "search-context-write-table.golden")), + }, + { + doc: "Table format, single column", + format: NewSearchFormat("table {{.Name}}"), + expected: `NAME result1 result2 `, }, - // Custom Format { - formatter.Context{Format: NewSearchFormat("{{.Name}}")}, - `result1 + doc: "Custom format, single field", + format: NewSearchFormat("{{.Name}}"), + expected: `result1 result2 `, }, - // Custom Format with CreatedAt { - formatter.Context{Format: NewSearchFormat("{{.Name}} {{.StarCount}}")}, - `result1 5000 + doc: "Custom Format, two columns", + format: NewSearchFormat("{{.Name}} {{.StarCount}}"), + expected: `result1 5000 result2 5 `, }, @@ -152,61 +204,15 @@ result2 5 for _, tc := range cases { tc := tc - t.Run(string(tc.context.Format), func(t *testing.T) { + t.Run(tc.doc, func(t *testing.T) { var out bytes.Buffer - tc.context.Output = &out - - err := SearchWrite(tc.context, results) - if err != nil { - assert.Error(t, err, tc.expected) + err := SearchWrite(formatter.Context{Format: tc.format, Output: &out}, results) + if tc.expectedErr != "" { + assert.Check(t, is.Error(err, tc.expectedErr)) } else { - assert.Equal(t, out.String(), tc.expected) + assert.Check(t, err) } + assert.Check(t, is.Equal(out.String(), tc.expected)) }) } } - -func TestSearchContextWriteJSON(t *testing.T) { - results := []registrytypes.SearchResult{ - {Name: "result1", Description: "Official build", StarCount: 5000, IsOfficial: true}, - {Name: "result2", Description: "Not official", StarCount: 5, IsOfficial: false, IsAutomated: true}, //nolint:staticcheck // ignore SA1019 (IsAutomated is deprecated). - } - expectedJSONs := []map[string]interface{}{ - {"Name": "result1", "Description": "Official build", "StarCount": "5000", "IsOfficial": "true", "IsAutomated": "false"}, - {"Name": "result2", "Description": "Not official", "StarCount": "5", "IsOfficial": "false", "IsAutomated": "true"}, - } - - out := bytes.NewBufferString("") - err := SearchWrite(formatter.Context{Format: "{{json .}}", Output: out}, results) - if err != nil { - t.Fatal(err) - } - for i, line := range strings.Split(strings.TrimSpace(out.String()), "\n") { - t.Logf("Output: line %d: %s", i, line) - var m map[string]interface{} - if err := json.Unmarshal([]byte(line), &m); err != nil { - t.Fatal(err) - } - assert.Check(t, is.DeepEqual(m, expectedJSONs[i])) - } -} - -func TestSearchContextWriteJSONField(t *testing.T) { - results := []registrytypes.SearchResult{ - {Name: "result1", Description: "Official build", StarCount: 5000, IsOfficial: true}, - {Name: "result2", Description: "Not official", StarCount: 5, IsOfficial: false, IsAutomated: true}, //nolint:staticcheck // ignore SA1019 (IsAutomated is deprecated). - } - out := bytes.NewBufferString("") - err := SearchWrite(formatter.Context{Format: "{{json .Name}}", Output: out}, results) - if err != nil { - t.Fatal(err) - } - for i, line := range strings.Split(strings.TrimSpace(out.String()), "\n") { - t.Logf("Output: line %d: %s", i, line) - var s string - if err := json.Unmarshal([]byte(line), &s); err != nil { - t.Fatal(err) - } - assert.Check(t, is.Equal(s, results[i].Name)) - } -} From 57f0e46de1ced157dada9e1ccdc903216017ec4c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 8 Aug 2023 17:31:55 +0200 Subject: [PATCH 2/2] cli/command/registry: cleanup login tests - use consts for fixed values, and rename some for clarity - remove testAuthErrors map and inline the logic (same as we do for other cases) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login_test.go | 49 +++++++++++++++--------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index 263e774fae..ddbba746d2 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -17,15 +17,8 @@ import ( ) const ( - userErr = "userunknownError" - testAuthErrMsg = "UNKNOWN_ERR" -) - -var testAuthErrors = map[string]error{ - userErr: fmt.Errorf(testAuthErrMsg), -} - -var ( + unknownUser = "userunknownError" + errUnknownUser = "UNKNOWN_ERR" expiredPassword = "I_M_EXPIRED" useToken = "I_M_TOKEN" ) @@ -47,8 +40,10 @@ func (c fakeClient) RegistryLogin(_ context.Context, auth registrytypes.AuthConf IdentityToken: auth.Password, }, nil } - err := testAuthErrors[auth.Username] - return registrytypes.AuthenticateOKBody{}, err + if auth.Username == unknownUser { + return registrytypes.AuthenticateOKBody{}, fmt.Errorf(errUnknownUser) + } + return registrytypes.AuthenticateOKBody{}, nil } func TestLoginWithCredStoreCreds(t *testing.T) { @@ -63,10 +58,10 @@ func TestLoginWithCredStoreCreds(t *testing.T) { }, { inputAuthConfig: registrytypes.AuthConfig{ - Username: userErr, + Username: unknownUser, }, expectedMsg: "Authenticating with existing credentials...\n", - expectedErr: fmt.Sprintf("Login did not succeed, error: %s\n", testAuthErrMsg), + expectedErr: fmt.Sprintf("Login did not succeed, error: %s\n", errUnknownUser), }, } ctx := context.Background() @@ -83,10 +78,12 @@ func TestLoginWithCredStoreCreds(t *testing.T) { } func TestRunLogin(t *testing.T) { - const storedServerAddress = "reg1" - const validUsername = "u1" - const validPassword = "p1" - const validPassword2 = "p2" + const ( + storedServerAddress = "reg1" + validUsername = "u1" + validPassword = "p1" + validPassword2 = "p2" + ) validAuthConfig := configtypes.AuthConfig{ ServerAddress: storedServerAddress, @@ -104,20 +101,22 @@ func TestRunLogin(t *testing.T) { IdentityToken: useToken, } testCases := []struct { + doc string inputLoginOption loginOptions inputStoredCred *configtypes.AuthConfig expectedErr string expectedSavedCred configtypes.AuthConfig }{ { + doc: "valid auth from store", inputLoginOption: loginOptions{ serverAddress: storedServerAddress, }, inputStoredCred: &validAuthConfig, - expectedErr: "", expectedSavedCred: validAuthConfig, }, { + doc: "expired auth", inputLoginOption: loginOptions{ serverAddress: storedServerAddress, }, @@ -125,13 +124,13 @@ func TestRunLogin(t *testing.T) { expectedErr: "Error: Cannot perform an interactive login from a non TTY device", }, { + doc: "valid username and password", inputLoginOption: loginOptions{ serverAddress: storedServerAddress, user: validUsername, password: validPassword2, }, inputStoredCred: &validAuthConfig, - expectedErr: "", expectedSavedCred: configtypes.AuthConfig{ ServerAddress: storedServerAddress, Username: validUsername, @@ -139,27 +138,29 @@ func TestRunLogin(t *testing.T) { }, }, { + doc: "unknown user", inputLoginOption: loginOptions{ serverAddress: storedServerAddress, - user: userErr, + user: unknownUser, password: validPassword, }, inputStoredCred: &validAuthConfig, - expectedErr: testAuthErrMsg, + expectedErr: errUnknownUser, }, { + doc: "valid token", inputLoginOption: loginOptions{ serverAddress: storedServerAddress, user: validUsername, password: useToken, }, inputStoredCred: &validIdentityToken, - expectedErr: "", expectedSavedCred: validIdentityToken, }, } - for i, tc := range testCases { - t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.doc, func(t *testing.T) { tmpFile := fs.NewFile(t, "test-run-login") defer tmpFile.Remove() cli := test.NewFakeCli(&fakeClient{})