Merge pull request #1585 from cli/pr-commands-repo-override-arg
Disallow `pr -R` flag for commands that operate on the current branch
This commit is contained in:
commit
74524fc8f7
10 changed files with 513 additions and 151 deletions
|
|
@ -2,6 +2,7 @@ package diff
|
|||
|
||||
import (
|
||||
"bufio"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
|
|
@ -46,6 +47,10 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
|
|||
// support `-R, --repo` override
|
||||
opts.BaseRepo = f.BaseRepo
|
||||
|
||||
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
|
||||
return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")}
|
||||
}
|
||||
|
||||
if len(args) > 0 {
|
||||
opts.SelectorArg = args[0]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -19,8 +19,97 @@ import (
|
|||
"github.com/google/go-cmp/cmp"
|
||||
"github.com/google/shlex"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func Test_NewCmdDiff(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
args string
|
||||
isTTY bool
|
||||
want DiffOptions
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "number argument",
|
||||
args: "123",
|
||||
isTTY: true,
|
||||
want: DiffOptions{
|
||||
SelectorArg: "123",
|
||||
UseColor: "auto",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument",
|
||||
args: "",
|
||||
isTTY: true,
|
||||
want: DiffOptions{
|
||||
SelectorArg: "",
|
||||
UseColor: "auto",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no color when redirected",
|
||||
args: "",
|
||||
isTTY: false,
|
||||
want: DiffOptions{
|
||||
SelectorArg: "",
|
||||
UseColor: "never",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument with --repo override",
|
||||
args: "-R owner/repo",
|
||||
isTTY: true,
|
||||
wantErr: "argument required when using the --repo flag",
|
||||
},
|
||||
{
|
||||
name: "invalid --color argument",
|
||||
args: "--color doublerainbow",
|
||||
isTTY: true,
|
||||
wantErr: `did not understand color: "doublerainbow". Expected one of always, never, or auto`,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
io, _, _, _ := iostreams.Test()
|
||||
io.SetStdoutTTY(tt.isTTY)
|
||||
io.SetStdinTTY(tt.isTTY)
|
||||
io.SetStderrTTY(tt.isTTY)
|
||||
|
||||
f := &cmdutil.Factory{
|
||||
IOStreams: io,
|
||||
}
|
||||
|
||||
var opts *DiffOptions
|
||||
cmd := NewCmdDiff(f, func(o *DiffOptions) error {
|
||||
opts = o
|
||||
return nil
|
||||
})
|
||||
cmd.PersistentFlags().StringP("repo", "R", "", "")
|
||||
|
||||
argv, err := shlex.Split(tt.args)
|
||||
require.NoError(t, err)
|
||||
cmd.SetArgs(argv)
|
||||
|
||||
cmd.SetIn(&bytes.Buffer{})
|
||||
cmd.SetOut(ioutil.Discard)
|
||||
cmd.SetErr(ioutil.Discard)
|
||||
|
||||
_, err = cmd.ExecuteC()
|
||||
if tt.wantErr != "" {
|
||||
require.EqualError(t, err, tt.wantErr)
|
||||
return
|
||||
} else {
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg)
|
||||
assert.Equal(t, tt.want.UseColor, opts.UseColor)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli string) (*test.CmdOut, error) {
|
||||
io, _, stdout, stderr := iostreams.Test()
|
||||
io.SetStdoutTTY(isTTY)
|
||||
|
|
@ -74,14 +163,6 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli s
|
|||
}, err
|
||||
}
|
||||
|
||||
func TestPRDiff_validation(t *testing.T) {
|
||||
_, err := runCommand(nil, nil, false, "--color=doublerainbow")
|
||||
if err == nil {
|
||||
t.Fatal("expected error")
|
||||
}
|
||||
assert.Equal(t, `did not understand color: "doublerainbow". Expected one of always, never, or auto`, err.Error())
|
||||
}
|
||||
|
||||
func TestPRDiff_no_current_pr(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
|
|
|||
|
|
@ -64,6 +64,10 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm
|
|||
// support `-R, --repo` override
|
||||
opts.BaseRepo = f.BaseRepo
|
||||
|
||||
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
|
||||
return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")}
|
||||
}
|
||||
|
||||
if len(args) > 0 {
|
||||
opts.SelectorArg = args[0]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,8 +20,97 @@ import (
|
|||
"github.com/cli/cli/test"
|
||||
"github.com/google/shlex"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func Test_NewCmdMerge(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
args string
|
||||
isTTY bool
|
||||
want MergeOptions
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "number argument",
|
||||
args: "123",
|
||||
isTTY: true,
|
||||
want: MergeOptions{
|
||||
SelectorArg: "123",
|
||||
DeleteBranch: true,
|
||||
DeleteLocalBranch: true,
|
||||
MergeMethod: api.PullRequestMergeMethodMerge,
|
||||
InteractiveMode: true,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument with --repo override",
|
||||
args: "-R owner/repo",
|
||||
isTTY: true,
|
||||
wantErr: "argument required when using the --repo flag",
|
||||
},
|
||||
{
|
||||
name: "insufficient flags in non-interactive mode",
|
||||
args: "123",
|
||||
isTTY: false,
|
||||
wantErr: "--merge, --rebase, or --squash required when not attached to a terminal",
|
||||
},
|
||||
{
|
||||
name: "multiple merge methods",
|
||||
args: "123 --merge --rebase",
|
||||
isTTY: true,
|
||||
wantErr: "only one of --merge, --rebase, or --squash can be enabled",
|
||||
},
|
||||
{
|
||||
name: "multiple merge methods, non-tty",
|
||||
args: "123 --merge --rebase",
|
||||
isTTY: false,
|
||||
wantErr: "only one of --merge, --rebase, or --squash can be enabled",
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
io, _, _, _ := iostreams.Test()
|
||||
io.SetStdoutTTY(tt.isTTY)
|
||||
io.SetStdinTTY(tt.isTTY)
|
||||
io.SetStderrTTY(tt.isTTY)
|
||||
|
||||
f := &cmdutil.Factory{
|
||||
IOStreams: io,
|
||||
}
|
||||
|
||||
var opts *MergeOptions
|
||||
cmd := NewCmdMerge(f, func(o *MergeOptions) error {
|
||||
opts = o
|
||||
return nil
|
||||
})
|
||||
cmd.PersistentFlags().StringP("repo", "R", "", "")
|
||||
|
||||
argv, err := shlex.Split(tt.args)
|
||||
require.NoError(t, err)
|
||||
cmd.SetArgs(argv)
|
||||
|
||||
cmd.SetIn(&bytes.Buffer{})
|
||||
cmd.SetOut(ioutil.Discard)
|
||||
cmd.SetErr(ioutil.Discard)
|
||||
|
||||
_, err = cmd.ExecuteC()
|
||||
if tt.wantErr != "" {
|
||||
require.EqualError(t, err, tt.wantErr)
|
||||
return
|
||||
} else {
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg)
|
||||
assert.Equal(t, tt.want.DeleteBranch, opts.DeleteBranch)
|
||||
assert.Equal(t, tt.want.DeleteLocalBranch, opts.DeleteLocalBranch)
|
||||
assert.Equal(t, tt.want.MergeMethod, opts.MergeMethod)
|
||||
assert.Equal(t, tt.want.InteractiveMode, opts.InteractiveMode)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*test.CmdOut, error) {
|
||||
io, _, stdout, stderr := iostreams.Test()
|
||||
io.SetStdoutTTY(isTTY)
|
||||
|
|
@ -168,16 +257,6 @@ func TestPrMerge_nontty(t *testing.T) {
|
|||
assert.Equal(t, "", output.Stderr())
|
||||
}
|
||||
|
||||
func TestPrMerge_nontty_insufficient_flags(t *testing.T) {
|
||||
output, err := runCommand(nil, "master", false, "pr merge 1")
|
||||
if err == nil {
|
||||
t.Fatal("expected error")
|
||||
}
|
||||
|
||||
assert.Equal(t, "--merge, --rebase, or --squash required when not attached to a terminal", err.Error())
|
||||
assert.Equal(t, "", output.String())
|
||||
}
|
||||
|
||||
func TestPrMerge_withRepoFlag(t *testing.T) {
|
||||
http := initFakeHTTP()
|
||||
defer http.Verify(t)
|
||||
|
|
@ -489,17 +568,3 @@ func TestPRMerge_interactive(t *testing.T) {
|
|||
|
||||
test.ExpectLines(t, output.Stderr(), "Merged pull request #3", `Deleted branch.*blueberries`)
|
||||
}
|
||||
|
||||
func TestPrMerge_multipleMergeMethods(t *testing.T) {
|
||||
_, err := runCommand(nil, "master", true, "1 --merge --squash")
|
||||
if err == nil {
|
||||
t.Fatal("expected error running `pr merge` with multiple merge methods")
|
||||
}
|
||||
}
|
||||
|
||||
func TestPrMerge_multipleMergeMethods_nontty(t *testing.T) {
|
||||
_, err := runCommand(nil, "master", false, "1 --merge --squash")
|
||||
if err == nil {
|
||||
t.Fatal("expected error running `pr merge` with multiple merge methods")
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
package ready
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
|
||||
|
|
@ -43,6 +44,10 @@ func NewCmdReady(f *cmdutil.Factory, runF func(*ReadyOptions) error) *cobra.Comm
|
|||
// support `-R, --repo` override
|
||||
opts.BaseRepo = f.BaseRepo
|
||||
|
||||
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
|
||||
return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")}
|
||||
}
|
||||
|
||||
if len(args) > 0 {
|
||||
opts.SelectorArg = args[0]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -16,8 +16,80 @@ import (
|
|||
"github.com/cli/cli/pkg/iostreams"
|
||||
"github.com/cli/cli/test"
|
||||
"github.com/google/shlex"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func Test_NewCmdReady(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
args string
|
||||
isTTY bool
|
||||
want ReadyOptions
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "number argument",
|
||||
args: "123",
|
||||
isTTY: true,
|
||||
want: ReadyOptions{
|
||||
SelectorArg: "123",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument",
|
||||
args: "",
|
||||
isTTY: true,
|
||||
want: ReadyOptions{
|
||||
SelectorArg: "",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument with --repo override",
|
||||
args: "-R owner/repo",
|
||||
isTTY: true,
|
||||
wantErr: "argument required when using the --repo flag",
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
io, _, _, _ := iostreams.Test()
|
||||
io.SetStdoutTTY(tt.isTTY)
|
||||
io.SetStdinTTY(tt.isTTY)
|
||||
io.SetStderrTTY(tt.isTTY)
|
||||
|
||||
f := &cmdutil.Factory{
|
||||
IOStreams: io,
|
||||
}
|
||||
|
||||
var opts *ReadyOptions
|
||||
cmd := NewCmdReady(f, func(o *ReadyOptions) error {
|
||||
opts = o
|
||||
return nil
|
||||
})
|
||||
cmd.PersistentFlags().StringP("repo", "R", "", "")
|
||||
|
||||
argv, err := shlex.Split(tt.args)
|
||||
require.NoError(t, err)
|
||||
cmd.SetArgs(argv)
|
||||
|
||||
cmd.SetIn(&bytes.Buffer{})
|
||||
cmd.SetOut(ioutil.Discard)
|
||||
cmd.SetErr(ioutil.Discard)
|
||||
|
||||
_, err = cmd.ExecuteC()
|
||||
if tt.wantErr != "" {
|
||||
require.EqualError(t, err, tt.wantErr)
|
||||
return
|
||||
} else {
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) {
|
||||
io, _, stdout, stderr := iostreams.Test()
|
||||
io.SetStdoutTTY(isTTY)
|
||||
|
|
|
|||
|
|
@ -28,11 +28,10 @@ type ReviewOptions struct {
|
|||
Remotes func() (context.Remotes, error)
|
||||
Branch func() (string, error)
|
||||
|
||||
SelectorArg string
|
||||
Approve bool
|
||||
RequestChanges bool
|
||||
Comment bool
|
||||
Body string
|
||||
SelectorArg string
|
||||
InteractiveMode bool
|
||||
ReviewType api.PullRequestReviewState
|
||||
Body string
|
||||
}
|
||||
|
||||
func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Command {
|
||||
|
|
@ -44,6 +43,12 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co
|
|||
Branch: f.Branch,
|
||||
}
|
||||
|
||||
var (
|
||||
flagApprove bool
|
||||
flagRequestChanges bool
|
||||
flagComment bool
|
||||
)
|
||||
|
||||
cmd := &cobra.Command{
|
||||
Use: "review [<number> | <url> | <branch>]",
|
||||
Short: "Add a review to a pull request",
|
||||
|
|
@ -70,10 +75,45 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co
|
|||
// support `-R, --repo` override
|
||||
opts.BaseRepo = f.BaseRepo
|
||||
|
||||
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
|
||||
return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")}
|
||||
}
|
||||
|
||||
if len(args) > 0 {
|
||||
opts.SelectorArg = args[0]
|
||||
}
|
||||
|
||||
found := 0
|
||||
if flagApprove {
|
||||
found++
|
||||
opts.ReviewType = api.ReviewApprove
|
||||
}
|
||||
if flagRequestChanges {
|
||||
found++
|
||||
opts.ReviewType = api.ReviewRequestChanges
|
||||
if opts.Body == "" {
|
||||
return &cmdutil.FlagError{Err: errors.New("body cannot be blank for request-changes review")}
|
||||
}
|
||||
}
|
||||
if flagComment {
|
||||
found++
|
||||
opts.ReviewType = api.ReviewComment
|
||||
if opts.Body == "" {
|
||||
return &cmdutil.FlagError{Err: errors.New("body cannot be blank for comment review")}
|
||||
}
|
||||
}
|
||||
|
||||
if found == 0 && opts.Body == "" {
|
||||
if !opts.IO.IsStdoutTTY() || !opts.IO.IsStdinTTY() {
|
||||
return &cmdutil.FlagError{Err: errors.New("--approve, --request-changes, or --comment required when not attached to a tty")}
|
||||
}
|
||||
opts.InteractiveMode = true
|
||||
} else if found == 0 && opts.Body != "" {
|
||||
return &cmdutil.FlagError{Err: errors.New("--body unsupported without --approve, --request-changes, or --comment")}
|
||||
} else if found > 1 {
|
||||
return &cmdutil.FlagError{Err: errors.New("need exactly one of --approve, --request-changes, or --comment")}
|
||||
}
|
||||
|
||||
if runF != nil {
|
||||
return runF(opts)
|
||||
}
|
||||
|
|
@ -81,9 +121,9 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co
|
|||
},
|
||||
}
|
||||
|
||||
cmd.Flags().BoolVarP(&opts.Approve, "approve", "a", false, "Approve pull request")
|
||||
cmd.Flags().BoolVarP(&opts.RequestChanges, "request-changes", "r", false, "Request changes on a pull request")
|
||||
cmd.Flags().BoolVarP(&opts.Comment, "comment", "c", false, "Comment on a pull request")
|
||||
cmd.Flags().BoolVarP(&flagApprove, "approve", "a", false, "Approve pull request")
|
||||
cmd.Flags().BoolVarP(&flagRequestChanges, "request-changes", "r", false, "Request changes on a pull request")
|
||||
cmd.Flags().BoolVarP(&flagComment, "comment", "c", false, "Comment on a pull request")
|
||||
cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Specify the body of a review")
|
||||
|
||||
return cmd
|
||||
|
|
@ -96,17 +136,13 @@ func reviewRun(opts *ReviewOptions) error {
|
|||
}
|
||||
apiClient := api.NewClientFromHTTP(httpClient)
|
||||
|
||||
reviewData, err := processReviewOpt(opts)
|
||||
if err != nil {
|
||||
return fmt.Errorf("did not understand desired review action: %w", err)
|
||||
}
|
||||
|
||||
pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if reviewData == nil {
|
||||
var reviewData *api.PullRequestReviewInput
|
||||
if opts.InteractiveMode {
|
||||
editorCommand, err := cmdutil.DetermineEditor(opts.Config)
|
||||
if err != nil {
|
||||
return err
|
||||
|
|
@ -119,6 +155,11 @@ func reviewRun(opts *ReviewOptions) error {
|
|||
fmt.Fprint(opts.IO.ErrOut, "Discarding.\n")
|
||||
return nil
|
||||
}
|
||||
} else {
|
||||
reviewData = &api.PullRequestReviewInput{
|
||||
State: opts.ReviewType,
|
||||
Body: opts.Body,
|
||||
}
|
||||
}
|
||||
|
||||
err = api.AddReview(apiClient, baseRepo, pr, reviewData)
|
||||
|
|
@ -142,51 +183,6 @@ func reviewRun(opts *ReviewOptions) error {
|
|||
return nil
|
||||
}
|
||||
|
||||
// TODO: move to Command.Args, raise FlagError
|
||||
func processReviewOpt(opts *ReviewOptions) (*api.PullRequestReviewInput, error) {
|
||||
found := 0
|
||||
flag := ""
|
||||
var state api.PullRequestReviewState
|
||||
|
||||
if opts.Approve {
|
||||
found++
|
||||
flag = "approve"
|
||||
state = api.ReviewApprove
|
||||
}
|
||||
if opts.RequestChanges {
|
||||
found++
|
||||
flag = "request-changes"
|
||||
state = api.ReviewRequestChanges
|
||||
}
|
||||
if opts.Comment {
|
||||
found++
|
||||
flag = "comment"
|
||||
state = api.ReviewComment
|
||||
}
|
||||
|
||||
body := opts.Body
|
||||
|
||||
if found == 0 && body == "" {
|
||||
if opts.IO.IsStdoutTTY() && opts.IO.IsStderrTTY() {
|
||||
return nil, nil // signal interactive mode
|
||||
}
|
||||
return nil, errors.New("--approve, --request-changes, or --comment required when not attached to a tty")
|
||||
} else if found == 0 && body != "" {
|
||||
return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment")
|
||||
} else if found > 1 {
|
||||
return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")
|
||||
}
|
||||
|
||||
if (flag == "request-changes" || flag == "comment") && body == "" {
|
||||
return nil, fmt.Errorf("body cannot be blank for %s review", flag)
|
||||
}
|
||||
|
||||
return &api.PullRequestReviewInput{
|
||||
Body: body,
|
||||
State: state,
|
||||
}, nil
|
||||
}
|
||||
|
||||
func reviewSurvey(io *iostreams.IOStreams, editorCommand string) (*api.PullRequestReviewInput, error) {
|
||||
typeAnswers := struct {
|
||||
ReviewType string
|
||||
|
|
|
|||
|
|
@ -19,8 +19,114 @@ import (
|
|||
"github.com/cli/cli/test"
|
||||
"github.com/google/shlex"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func Test_NewCmdReview(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
args string
|
||||
isTTY bool
|
||||
want ReviewOptions
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "number argument",
|
||||
args: "123",
|
||||
isTTY: true,
|
||||
want: ReviewOptions{
|
||||
SelectorArg: "123",
|
||||
ReviewType: 0,
|
||||
Body: "",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument",
|
||||
args: "",
|
||||
isTTY: true,
|
||||
want: ReviewOptions{
|
||||
SelectorArg: "",
|
||||
ReviewType: 0,
|
||||
Body: "",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument with --repo override",
|
||||
args: "-R owner/repo",
|
||||
isTTY: true,
|
||||
wantErr: "argument required when using the --repo flag",
|
||||
},
|
||||
{
|
||||
name: "no arguments in non-interactive mode",
|
||||
args: "",
|
||||
isTTY: false,
|
||||
wantErr: "--approve, --request-changes, or --comment required when not attached to a tty",
|
||||
},
|
||||
{
|
||||
name: "mutually exclusive review types",
|
||||
args: `--approve --comment -b hello`,
|
||||
isTTY: true,
|
||||
wantErr: "need exactly one of --approve, --request-changes, or --comment",
|
||||
},
|
||||
{
|
||||
name: "comment without body",
|
||||
args: `--comment`,
|
||||
isTTY: true,
|
||||
wantErr: "body cannot be blank for comment review",
|
||||
},
|
||||
{
|
||||
name: "request changes without body",
|
||||
args: `--request-changes`,
|
||||
isTTY: true,
|
||||
wantErr: "body cannot be blank for request-changes review",
|
||||
},
|
||||
{
|
||||
name: "only body argument",
|
||||
args: `-b hello`,
|
||||
isTTY: true,
|
||||
wantErr: "--body unsupported without --approve, --request-changes, or --comment",
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
io, _, _, _ := iostreams.Test()
|
||||
io.SetStdoutTTY(tt.isTTY)
|
||||
io.SetStdinTTY(tt.isTTY)
|
||||
io.SetStderrTTY(tt.isTTY)
|
||||
|
||||
f := &cmdutil.Factory{
|
||||
IOStreams: io,
|
||||
}
|
||||
|
||||
var opts *ReviewOptions
|
||||
cmd := NewCmdReview(f, func(o *ReviewOptions) error {
|
||||
opts = o
|
||||
return nil
|
||||
})
|
||||
cmd.PersistentFlags().StringP("repo", "R", "", "")
|
||||
|
||||
argv, err := shlex.Split(tt.args)
|
||||
require.NoError(t, err)
|
||||
cmd.SetArgs(argv)
|
||||
|
||||
cmd.SetIn(&bytes.Buffer{})
|
||||
cmd.SetOut(ioutil.Discard)
|
||||
cmd.SetErr(ioutil.Discard)
|
||||
|
||||
_, err = cmd.ExecuteC()
|
||||
if tt.wantErr != "" {
|
||||
require.EqualError(t, err, tt.wantErr)
|
||||
return
|
||||
} else {
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg)
|
||||
assert.Equal(t, tt.want.Body, opts.Body)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli string) (*test.CmdOut, error) {
|
||||
io, _, stdout, stderr := iostreams.Test()
|
||||
io.SetStdoutTTY(isTTY)
|
||||
|
|
@ -74,36 +180,6 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli s
|
|||
}, err
|
||||
}
|
||||
|
||||
func TestPRReview_validation(t *testing.T) {
|
||||
for _, cmd := range []string{
|
||||
`--approve --comment 123`,
|
||||
`--approve --comment -b"hey" 123`,
|
||||
} {
|
||||
t.Run(cmd, func(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
||||
_, err := runCommand(http, nil, false, cmd)
|
||||
if err == nil {
|
||||
t.Fatal("expected error")
|
||||
}
|
||||
assert.Equal(t, "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment", err.Error())
|
||||
})
|
||||
|
||||
}
|
||||
}
|
||||
|
||||
func TestPRReview_bad_body(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
||||
_, err := runCommand(http, nil, false, `123 -b "radical"`)
|
||||
if err == nil {
|
||||
t.Fatal("expected error")
|
||||
}
|
||||
assert.Equal(t, "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment", err.Error())
|
||||
}
|
||||
|
||||
func TestPRReview_url_arg(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
|
@ -233,22 +309,6 @@ func TestPRReview_no_arg(t *testing.T) {
|
|||
assert.Equal(t, "cool story", reqBody.Variables.Input.Body)
|
||||
}
|
||||
|
||||
func TestPRReview_blank_comment(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
||||
_, err := runCommand(http, nil, false, `--comment 123`)
|
||||
assert.Equal(t, "did not understand desired review action: body cannot be blank for comment review", err.Error())
|
||||
}
|
||||
|
||||
func TestPRReview_blank_request_changes(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
||||
_, err := runCommand(http, nil, false, `-r 123`)
|
||||
assert.Equal(t, "did not understand desired review action: body cannot be blank for request-changes review", err.Error())
|
||||
}
|
||||
|
||||
func TestPRReview(t *testing.T) {
|
||||
type c struct {
|
||||
Cmd string
|
||||
|
|
@ -298,20 +358,6 @@ func TestPRReview(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestPRReview_nontty_insufficient_flags(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
||||
output, err := runCommand(http, nil, false, "")
|
||||
if err == nil {
|
||||
t.Fatal("expected error")
|
||||
}
|
||||
|
||||
assert.Equal(t, "", output.String())
|
||||
|
||||
assert.Equal(t, "did not understand desired review action: --approve, --request-changes, or --comment required when not attached to a tty", err.Error())
|
||||
}
|
||||
|
||||
func TestPRReview_nontty(t *testing.T) {
|
||||
http := &httpmock.Registry{}
|
||||
defer http.Verify(t)
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
package view
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
|
|
@ -56,6 +57,10 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
|
|||
// support `-R, --repo` override
|
||||
opts.BaseRepo = f.BaseRepo
|
||||
|
||||
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
|
||||
return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")}
|
||||
}
|
||||
|
||||
if len(args) > 0 {
|
||||
opts.SelectorArg = args[0]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -19,8 +19,91 @@ import (
|
|||
"github.com/cli/cli/pkg/iostreams"
|
||||
"github.com/cli/cli/test"
|
||||
"github.com/google/shlex"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func Test_NewCmdView(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
args string
|
||||
isTTY bool
|
||||
want ViewOptions
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "number argument",
|
||||
args: "123",
|
||||
isTTY: true,
|
||||
want: ViewOptions{
|
||||
SelectorArg: "123",
|
||||
BrowserMode: false,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument",
|
||||
args: "",
|
||||
isTTY: true,
|
||||
want: ViewOptions{
|
||||
SelectorArg: "",
|
||||
BrowserMode: false,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "web mode",
|
||||
args: "123 -w",
|
||||
isTTY: true,
|
||||
want: ViewOptions{
|
||||
SelectorArg: "123",
|
||||
BrowserMode: true,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no argument with --repo override",
|
||||
args: "-R owner/repo",
|
||||
isTTY: true,
|
||||
wantErr: "argument required when using the --repo flag",
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
io, _, _, _ := iostreams.Test()
|
||||
io.SetStdoutTTY(tt.isTTY)
|
||||
io.SetStdinTTY(tt.isTTY)
|
||||
io.SetStderrTTY(tt.isTTY)
|
||||
|
||||
f := &cmdutil.Factory{
|
||||
IOStreams: io,
|
||||
}
|
||||
|
||||
var opts *ViewOptions
|
||||
cmd := NewCmdView(f, func(o *ViewOptions) error {
|
||||
opts = o
|
||||
return nil
|
||||
})
|
||||
cmd.PersistentFlags().StringP("repo", "R", "", "")
|
||||
|
||||
argv, err := shlex.Split(tt.args)
|
||||
require.NoError(t, err)
|
||||
cmd.SetArgs(argv)
|
||||
|
||||
cmd.SetIn(&bytes.Buffer{})
|
||||
cmd.SetOut(ioutil.Discard)
|
||||
cmd.SetErr(ioutil.Discard)
|
||||
|
||||
_, err = cmd.ExecuteC()
|
||||
if tt.wantErr != "" {
|
||||
require.EqualError(t, err, tt.wantErr)
|
||||
return
|
||||
} else {
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func eq(t *testing.T, got interface{}, expected interface{}) {
|
||||
t.Helper()
|
||||
if !reflect.DeepEqual(got, expected) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue