From bef62faaead78d4d4d986c557695f711df14be52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 12:54:09 +0200 Subject: [PATCH] Make `NewCmdApi` testable --- command/root.go | 29 +++++- go.mod | 2 +- go.sum | 5 +- pkg/cmd/api/api.go | 22 ++--- pkg/cmd/api/api_test.go | 185 ++++++++++++++++++++++++++++++++++++++- pkg/cmd/api/http_test.go | 8 +- pkg/cmd/api/legacy.go | 34 ------- pkg/cmdutil/factory.go | 12 +++ 8 files changed, 238 insertions(+), 59 deletions(-) delete mode 100644 pkg/cmd/api/legacy.go create mode 100644 pkg/cmdutil/factory.go diff --git a/command/root.go b/command/root.go index 93ed51109..95f993151 100644 --- a/command/root.go +++ b/command/root.go @@ -3,6 +3,7 @@ package command import ( "fmt" "io" + "net/http" "os" "regexp" "runtime/debug" @@ -14,6 +15,7 @@ import ( "github.com/cli/cli/internal/ghrepo" apiCmd "github.com/cli/cli/pkg/cmd/api" "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" "github.com/spf13/cobra" @@ -64,7 +66,19 @@ func init() { return &cmdutil.FlagError{Err: err} }) - RootCmd.AddCommand(apiCmd.NewCmdApi()) + // TODO: iron out how a factory incorporates context + cmdFactory := &cmdutil.Factory{ + IOStreams: iostreams.System(), + HttpClient: func() (*http.Client, error) { + ctx := context.New() + token, err := ctx.AuthToken() + if err != nil { + return nil, err + } + return httpClient(token), nil + }, + } + RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil)) } // RootCmd is the entry point of command-line execution @@ -119,6 +133,19 @@ func contextForCommand(cmd *cobra.Command) context.Context { return ctx } +// for cmdutil-powered commands +func httpClient(token string) *http.Client { + var opts []api.ClientOption + if verbose := os.Getenv("DEBUG"); verbose != "" { + opts = append(opts, apiVerboseLog()) + } + opts = append(opts, + api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), + api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", Version)), + ) + return api.NewHTTPClient(opts...) +} + // overridden in tests var apiClientForContext = func(ctx context.Context) (*api.Client, error) { token, err := ctx.AuthToken() diff --git a/go.mod b/go.mod index 193320caa..407c8ab8a 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,7 @@ require ( github.com/shurcooL/graphql v0.0.0-20181231061246-d48a9a75455f // indirect github.com/spf13/cobra v0.0.6 github.com/spf13/pflag v1.0.5 - github.com/stretchr/testify v1.4.0 // indirect + github.com/stretchr/testify v1.5.1 golang.org/x/crypto v0.0.0-20200219234226-1ad67e1f0ef4 golang.org/x/net v0.0.0-20200219183655-46282727080f // indirect golang.org/x/text v0.3.2 diff --git a/go.sum b/go.sum index 807e68f62..09c195c5d 100644 --- a/go.sum +++ b/go.sum @@ -167,13 +167,14 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spf13/viper v1.4.0/go.mod h1:PTJ7Z/lr49W6bUbkmS1V3by4uWynFiR9p7+dSq/yZzE= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= -github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= -github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= +github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4= +github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGrc= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU= diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 02eac439b..89dc53103 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -9,7 +9,6 @@ import ( "strconv" "strings" - "github.com/cli/cli/context" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/spf13/cobra" @@ -29,8 +28,12 @@ type ApiOptions struct { HttpClient func() (*http.Client, error) } -func NewCmdApi() *cobra.Command { - opts := ApiOptions{} +func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command { + opts := ApiOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + } + cmd := &cobra.Command{ Use: "api ", Short: "Make an authenticated GitHub API request", @@ -58,18 +61,9 @@ on the format of the value: opts.RequestPath = args[0] opts.RequestMethodPassed = c.Flags().Changed("method") - // TODO: pass in via caller - opts.IO = iostreams.System() - - opts.HttpClient = func() (*http.Client, error) { - ctx := context.New() - token, err := ctx.AuthToken() - if err != nil { - return nil, err - } - return apiClientFromContext(token), nil + if runF != nil { + return runF(&opts) } - return apiRun(&opts) }, } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index ad8211761..8b906d3ba 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -3,15 +3,115 @@ package api import ( "bytes" "fmt" + "io" "io/ioutil" "net/http" - "reflect" + "os" "testing" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" ) +func Test_NewCmdApi(t *testing.T) { + f := &cmdutil.Factory{} + + tests := []struct { + name string + cli string + wants ApiOptions + wantsErr bool + }{ + { + name: "no flags", + cli: "graphql", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "graphql", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, + { + name: "override method", + cli: "repos/octocat/Spoon-Knife -XDELETE", + wants: ApiOptions{ + RequestMethod: "DELETE", + RequestMethodPassed: true, + RequestPath: "repos/octocat/Spoon-Knife", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, + { + name: "with fields", + cli: "graphql -f query=QUERY -F body=@file.txt", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "graphql", + RawFields: []string{"query=QUERY"}, + MagicFields: []string{"body=@file.txt"}, + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, + { + name: "with headers", + cli: "user -H 'accept: text/plain' -i", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "user", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string{"accept: text/plain"}, + ShowResponseHeaders: true, + }, + wantsErr: false, + }, + { + name: "no arguments", + cli: "", + wantsErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := NewCmdApi(f, func(o *ApiOptions) error { + assert.Equal(t, tt.wants.RequestMethod, o.RequestMethod) + assert.Equal(t, tt.wants.RequestMethodPassed, o.RequestMethodPassed) + assert.Equal(t, tt.wants.RequestPath, o.RequestPath) + assert.Equal(t, tt.wants.RawFields, o.RawFields) + assert.Equal(t, tt.wants.MagicFields, o.MagicFields) + assert.Equal(t, tt.wants.RequestHeaders, o.RequestHeaders) + assert.Equal(t, tt.wants.ShowResponseHeaders, o.ShowResponseHeaders) + return nil + }) + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + cmd.SetArgs(argv) + _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + }) + } +} + func Test_apiRun(t *testing.T) { tests := []struct { name string @@ -30,6 +130,16 @@ func Test_apiRun(t *testing.T) { stdout: `bam!`, stderr: ``, }, + { + name: "success 204", + httpResponse: &http.Response{ + StatusCode: 204, + Body: nil, + }, + err: nil, + stdout: ``, + stderr: ``, + }, { name: "failure", httpResponse: &http.Response{ @@ -109,7 +219,76 @@ func Test_parseFields(t *testing.T) { "enabled": true, "victories": 123, } - if !reflect.DeepEqual(params, expect) { - t.Errorf("expected %v, got %v", expect, params) + assert.Equal(t, expect, params) +} + +func Test_magicFieldValue(t *testing.T) { + f, err := ioutil.TempFile("", "gh-test") + if err != nil { + t.Fatal(err) + } + fmt.Fprint(f, "file contents") + f.Close() + t.Cleanup(func() { os.Remove(f.Name()) }) + + type args struct { + v string + stdin io.ReadCloser + } + tests := []struct { + name string + args args + want interface{} + wantErr bool + }{ + { + name: "string", + args: args{v: "hello"}, + want: "hello", + wantErr: false, + }, + { + name: "bool true", + args: args{v: "true"}, + want: true, + wantErr: false, + }, + { + name: "bool false", + args: args{v: "false"}, + want: false, + wantErr: false, + }, + { + name: "null", + args: args{v: "null"}, + want: nil, + wantErr: false, + }, + { + name: "file", + args: args{v: "@" + f.Name()}, + want: []byte("file contents"), + wantErr: false, + }, + { + name: "file error", + args: args{v: "@"}, + want: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := magicFieldValue(tt.args.v, tt.args.stdin) + if (err != nil) != tt.wantErr { + t.Errorf("magicFieldValue() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr { + return + } + assert.Equal(t, tt.want, got) + }) } } diff --git a/pkg/cmd/api/http_test.go b/pkg/cmd/api/http_test.go index 772a90409..e2ba8680b 100644 --- a/pkg/cmd/api/http_test.go +++ b/pkg/cmd/api/http_test.go @@ -4,8 +4,9 @@ import ( "bytes" "io/ioutil" "net/http" - "reflect" "testing" + + "github.com/stretchr/testify/assert" ) func Test_groupGraphQLVariables(t *testing.T) { @@ -57,9 +58,8 @@ func Test_groupGraphQLVariables(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := groupGraphQLVariables(tt.args); !reflect.DeepEqual(got, tt.want) { - t.Errorf("groupGraphQLVariables() = %v, want %v", got, tt.want) - } + got := groupGraphQLVariables(tt.args) + assert.Equal(t, tt.want, got) }) } } diff --git a/pkg/cmd/api/legacy.go b/pkg/cmd/api/legacy.go deleted file mode 100644 index 5c8429727..000000000 --- a/pkg/cmd/api/legacy.go +++ /dev/null @@ -1,34 +0,0 @@ -package api - -import ( - "fmt" - "net/http" - "os" - "strings" - - "github.com/cli/cli/api" - "github.com/cli/cli/utils" -) - -// TODO -func apiClientFromContext(token string) *http.Client { - var opts []api.ClientOption - if verbose := os.Getenv("DEBUG"); verbose != "" { - opts = append(opts, apiVerboseLog()) - } - opts = append(opts, - api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), - // FIXME - // api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", command.Version)), - // antiope-preview: Checks - api.AddHeader("Accept", "application/vnd.github.antiope-preview+json"), - ) - return api.NewHTTPClient(opts...) -} - -// TODO -func apiVerboseLog() api.ClientOption { - logTraffic := strings.Contains(os.Getenv("DEBUG"), "api") - colorize := utils.IsTerminal(os.Stderr) - return api.VerboseLog(utils.NewColorable(os.Stderr), logTraffic, colorize) -} diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go new file mode 100644 index 000000000..578b29561 --- /dev/null +++ b/pkg/cmdutil/factory.go @@ -0,0 +1,12 @@ +package cmdutil + +import ( + "net/http" + + "github.com/cli/cli/pkg/iostreams" +) + +type Factory struct { + IOStreams *iostreams.IOStreams + HttpClient func() (*http.Client, error) +}