From 3bcf9758ad24a3c8caf000c91b42a332c132877c Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 17 Apr 2025 22:13:31 +0200 Subject: [PATCH 1/2] Feature detect v1 projects on pr view --- pkg/cmd/pr/shared/finder.go | 29 +++++++++++-- pkg/cmd/pr/view/view.go | 5 +++ pkg/cmd/pr/view/view_test.go | 81 ++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 6d36ef816e4..b509f946c7b 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -16,6 +16,7 @@ import ( ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" fd "github.com/cli/cli/v2/internal/featuredetection" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" o "github.com/cli/cli/v2/pkg/option" @@ -79,6 +80,10 @@ func RunCommandFinder(selector string, pr *api.PullRequest, repo ghrepo.Interfac return finder } +func ResetRunCommandFinder() { + runCommandFinder = nil +} + type FindOptions struct { // Selector can be a number with optional `#` prefix, a branch name with optional `:` prefix, or // a PR URL. @@ -89,6 +94,8 @@ type FindOptions struct { BaseBranch string // States lists the possible PR states to scope the PR-for-branch lookup to. States []string + + Detector fd.Detector } func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, error) { @@ -193,9 +200,11 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err fields.AddValues([]string{"id", "number"}) // for additional preload queries below if fields.Contains("isInMergeQueue") || fields.Contains("isMergeQueueEnabled") { - cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - detector := fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost()) - prFeatures, err := detector.PullRequestFeatures() + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost()) + } + prFeatures, err := opts.Detector.PullRequestFeatures() if err != nil { return nil, nil, err } @@ -211,6 +220,20 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err fields.Remove("projectItems") } + // TODO projectsV1Deprecation + // Remove this block + // When removing this, remember to remove `projectCards` from the list of default fields in pr/view.go + if fields.Contains("projectCards") { + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost()) + } + + if opts.Detector.ProjectsV1() == gh.ProjectsV1Unsupported { + fields.Remove("projectCards") + } + } + var pr *api.PullRequest if f.prNumber > 0 { if numberFieldOnly { diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index 997f74d877d..8a39d113463 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -10,6 +10,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -22,6 +23,9 @@ import ( type ViewOptions struct { IO *iostreams.IOStreams Browser browser.Browser + // TODO projectsV1Deprecation + // Remove this detector since it is only used for test validation. + Detector fd.Detector Finder shared.PRFinder Exporter cmdutil.Exporter @@ -89,6 +93,7 @@ func viewRun(opts *ViewOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, Fields: defaultFields, + Detector: opts.Detector, } if opts.BrowserMode { findOptions.Fields = []string{"url"} diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go index e7f572c7663..3a2a87e5cf7 100644 --- a/pkg/cmd/pr/view/view_test.go +++ b/pkg/cmd/pr/view/view_test.go @@ -12,6 +12,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/run" "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -175,6 +176,9 @@ func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*t factory := &cmdutil.Factory{ IOStreams: ios, Browser: browser, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, } cmd := NewCmdView(factory, nil) @@ -398,6 +402,8 @@ func TestPRView_Preview_nontty(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { + t.Cleanup(shared.ResetRunCommandFinder) + http := &httpmock.Registry{} defer http.Verify(t) @@ -602,6 +608,8 @@ func TestPRView_Preview(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { + t.Cleanup(shared.ResetRunCommandFinder) + http := &httpmock.Registry{} defer http.Verify(t) @@ -846,6 +854,8 @@ func TestPRView_nontty_Comments(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { + t.Cleanup(shared.ResetRunCommandFinder) + http := &httpmock.Registry{} defer http.Verify(t) @@ -869,3 +879,74 @@ func TestPRView_nontty_Comments(t *testing.T) { }) } } + +// TODO projectsV1Deprecation +// Remove this test. +func TestProjectsV1Deprecation(t *testing.T) { + t.Run("when projects v1 is supported, is included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`projectCards`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + f := &cmdutil.Factory{ + IOStreams: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + } + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = viewRun(&ViewOptions{ + IO: ios, + Finder: shared.NewFinder(f), + Detector: &fd.EnabledDetectorMock{}, + + SelectorArg: "https://github.com/cli/cli/pull/123", + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) + + t.Run("when projects v1 is not supported, is not included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Exclude( + t, + httpmock.GraphQL(`projectCards`), + ) + + f := &cmdutil.Factory{ + IOStreams: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + } + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = viewRun(&ViewOptions{ + IO: ios, + Finder: shared.NewFinder(f), + Detector: &fd.DisabledDetectorMock{}, + + SelectorArg: "https://github.com/cli/cli/pull/123", + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) +} From 64370ce73e6774cd5c7ec912c4cafbd513f2af73 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 2 May 2025 14:41:24 +0200 Subject: [PATCH 2/2] Cleanup run command stubbed finders in tests --- pkg/cmd/pr/checkout/checkout_test.go | 26 +++++------ pkg/cmd/pr/close/close_test.go | 14 +++--- pkg/cmd/pr/merge/merge_test.go | 66 ++++++++++++++-------------- pkg/cmd/pr/ready/ready_test.go | 10 ++--- pkg/cmd/pr/reopen/reopen_test.go | 8 ++-- pkg/cmd/pr/review/review_test.go | 8 ++-- pkg/cmd/pr/shared/finder.go | 30 ++++++++----- pkg/cmd/pr/view/view_test.go | 22 ++++------ 8 files changed, 93 insertions(+), 91 deletions(-) diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 40917fd7629..496139423e9 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -518,7 +518,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") - finder := shared.RunCommandFinder("123", pr, baseRepo) + finder := shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) finder.ExpectFields([]string{"number", "headRefName", "headRepository", "headRepositoryOwner", "isCrossRepository", "maintainerCanModify"}) cs, cmdTeardown := run.Stub() @@ -539,7 +539,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -570,7 +570,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO", "hubot/REPO:feature") - finder := shared.RunCommandFinder("123", pr, baseRepo) + finder := shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) finder.ExpectFields([]string{"number", "headRefName", "headRepository", "headRepositoryOwner", "isCrossRepository", "maintainerCanModify"}) cs, cmdTeardown := run.Stub() @@ -590,7 +590,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - finder := shared.RunCommandFinder("123", pr, baseRepo) + finder := shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) finder.ExpectFields([]string{"number", "headRefName", "headRepository", "headRepositoryOwner", "isCrossRepository", "maintainerCanModify"}) cs, cmdTeardown := run.Stub() @@ -613,7 +613,7 @@ func TestPRCheckout_differentRepoForce(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - finder := shared.RunCommandFinder("123", pr, baseRepo) + finder := shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) finder.ExpectFields([]string{"number", "headRefName", "headRepository", "headRepositoryOwner", "isCrossRepository", "maintainerCanModify"}) cs, cmdTeardown := run.Stub() @@ -636,7 +636,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -655,7 +655,7 @@ func TestPRCheckout_detachedHead(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -674,7 +674,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -693,7 +693,7 @@ func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO", "hubot/REPO:-foo") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) _, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -711,7 +711,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") pr.MaintainerCanModify = true - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -732,7 +732,7 @@ func TestPRCheckout_recurseSubmodules(t *testing.T) { http := &httpmock.Registry{} baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -753,7 +753,7 @@ func TestPRCheckout_force(t *testing.T) { http := &httpmock.Registry{} baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -774,7 +774,7 @@ func TestPRCheckout_detach(t *testing.T) { defer http.Verify(t) baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") - shared.RunCommandFinder("123", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) diff --git a/pkg/cmd/pr/close/close_test.go b/pkg/cmd/pr/close/close_test.go index 959af0e04f2..57ee0f0e643 100644 --- a/pkg/cmd/pr/close/close_test.go +++ b/pkg/cmd/pr/close/close_test.go @@ -110,7 +110,7 @@ func TestPrClose(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), @@ -133,7 +133,7 @@ func TestPrClose_alreadyClosed(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") pr.State = "CLOSED" pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) output, err := runCommand(http, true, "96") assert.NoError(t, err) @@ -147,7 +147,7 @@ func TestPrClose_deleteBranch_sameRepo(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:blueberries") pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), @@ -181,7 +181,7 @@ func TestPrClose_deleteBranch_crossRepo(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO", "hubot/REPO:blueberries") pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), @@ -213,7 +213,7 @@ func TestPrClose_deleteBranch_sameBranch(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO:main", "OWNER/REPO:trunk") pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), @@ -248,7 +248,7 @@ func TestPrClose_deleteBranch_notInGitRepo(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO:main", "OWNER/REPO:trunk") pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), @@ -282,7 +282,7 @@ func TestPrClose_withComment(t *testing.T) { baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") pr.Title = "The title of the PR" - shared.RunCommandFinder("96", pr, baseRepo) + shared.StubFinderForRunCommandStyleTests(t, "96", pr, baseRepo) http.Register( httpmock.GraphQL(`mutation CommentCreate\b`), diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index f1c2e37fefb..4ca8c5d06df 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -307,7 +307,7 @@ func TestPrMerge(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -348,7 +348,7 @@ func TestPrMerge_blocked(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -379,7 +379,7 @@ func TestPrMerge_dirty(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -413,7 +413,7 @@ func TestPrMerge_nontty(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -451,7 +451,7 @@ func TestPrMerge_editMessage_nontty(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -490,7 +490,7 @@ func TestPrMerge_withRepoFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -529,7 +529,7 @@ func TestPrMerge_withMatchCommitHeadFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -570,7 +570,7 @@ func TestPrMerge_withAuthorFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -612,7 +612,7 @@ func TestPrMerge_deleteBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "PR_10", @@ -663,7 +663,7 @@ func TestPrMerge_deleteBranch_mergeQueue(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "PR_10", @@ -686,7 +686,7 @@ func TestPrMerge_deleteBranch_nonDefault(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "PR_10", @@ -737,7 +737,7 @@ func TestPrMerge_deleteBranch_onlyLocally(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "PR_10", @@ -785,7 +785,7 @@ func TestPrMerge_deleteBranch_checkoutNewBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "PR_10", @@ -836,7 +836,7 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "blueberries", &api.PullRequest{ ID: "PR_10", @@ -893,7 +893,7 @@ func Test_nonDivergingPullRequest(t *testing.T) { } stubCommit(pr, "COMMITSHA1") - shared.RunCommandFinder("", pr, baseRepo("OWNER", "REPO", "main")) + shared.StubFinderForRunCommandStyleTests(t, "", pr, baseRepo("OWNER", "REPO", "main")) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), @@ -933,7 +933,7 @@ func Test_divergingPullRequestWarning(t *testing.T) { } stubCommit(pr, "COMMITSHA1") - shared.RunCommandFinder("", pr, baseRepo("OWNER", "REPO", "main")) + shared.StubFinderForRunCommandStyleTests(t, "", pr, baseRepo("OWNER", "REPO", "main")) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), @@ -964,7 +964,7 @@ func Test_pullRequestWithoutCommits(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "PR_10", @@ -1003,7 +1003,7 @@ func TestPrMerge_rebase(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "2", &api.PullRequest{ ID: "THE-ID", @@ -1044,7 +1044,7 @@ func TestPrMerge_squash(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "3", &api.PullRequest{ ID: "THE-ID", @@ -1084,7 +1084,7 @@ func TestPrMerge_alreadyMerged(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "4", &api.PullRequest{ ID: "THE-ID", @@ -1129,7 +1129,7 @@ func TestPrMerge_alreadyMerged_withMergeStrategy(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "4", &api.PullRequest{ ID: "THE-ID", @@ -1159,7 +1159,7 @@ func TestPrMerge_alreadyMerged_withMergeStrategy_TTY(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "4", &api.PullRequest{ ID: "THE-ID", @@ -1200,7 +1200,7 @@ func TestPrMerge_alreadyMerged_withMergeStrategy_crossRepo(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "4", &api.PullRequest{ ID: "THE-ID", @@ -1239,7 +1239,7 @@ func TestPRMergeTTY(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "THE-ID", @@ -1305,7 +1305,7 @@ func TestPRMergeTTY_withDeleteBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ ID: "THE-ID", @@ -1468,7 +1468,7 @@ func TestPRMergeEmptyStrategyNonTTY(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -1495,7 +1495,7 @@ func TestPRTTY_cancelled(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ID: "THE-ID", Number: 123, Title: "title", MergeStateStatus: "CLEAN"}, ghrepo.New("OWNER", "REPO"), @@ -1679,7 +1679,7 @@ func TestPrInMergeQueue(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -1710,7 +1710,7 @@ func TestPrAddToMergeQueueWithMergeMethod(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -1748,7 +1748,7 @@ func TestPrAddToMergeQueueClean(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -1788,7 +1788,7 @@ func TestPrAddToMergeQueueBlocked(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -1828,7 +1828,7 @@ func TestPrAddToMergeQueueAdmin(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", @@ -1897,7 +1897,7 @@ func TestPrAddToMergeQueueAdminWithMergeStrategy(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - shared.RunCommandFinder( + shared.StubFinderForRunCommandStyleTests(t, "1", &api.PullRequest{ ID: "THE-ID", diff --git a/pkg/cmd/pr/ready/ready_test.go b/pkg/cmd/pr/ready/ready_test.go index 9046ab3aca3..5a6053a17c7 100644 --- a/pkg/cmd/pr/ready/ready_test.go +++ b/pkg/cmd/pr/ready/ready_test.go @@ -124,7 +124,7 @@ func TestPRReady(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "OPEN", @@ -149,7 +149,7 @@ func TestPRReady_alreadyReady(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "OPEN", @@ -166,7 +166,7 @@ func TestPRReadyUndo(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "OPEN", @@ -191,7 +191,7 @@ func TestPRReadyUndo_alreadyDraft(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "OPEN", @@ -208,7 +208,7 @@ func TestPRReady_closed(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "CLOSED", diff --git a/pkg/cmd/pr/reopen/reopen_test.go b/pkg/cmd/pr/reopen/reopen_test.go index 856e191721c..9fb3702c082 100644 --- a/pkg/cmd/pr/reopen/reopen_test.go +++ b/pkg/cmd/pr/reopen/reopen_test.go @@ -53,7 +53,7 @@ func TestPRReopen(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "CLOSED", @@ -78,7 +78,7 @@ func TestPRReopen_alreadyOpen(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "OPEN", @@ -95,7 +95,7 @@ func TestPRReopen_alreadyMerged(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "MERGED", @@ -112,7 +112,7 @@ func TestPRReopen_withComment(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("123", &api.PullRequest{ + shared.StubFinderForRunCommandStyleTests(t, "123", &api.PullRequest{ ID: "THE-ID", Number: 123, State: "CLOSED", diff --git a/pkg/cmd/pr/review/review_test.go b/pkg/cmd/pr/review/review_test.go index f9e00c3b8ee..684617ca97a 100644 --- a/pkg/cmd/pr/review/review_test.go +++ b/pkg/cmd/pr/review/review_test.go @@ -235,7 +235,7 @@ func TestPRReview(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID"}, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ID: "THE-ID"}, ghrepo.New("OWNER", "REPO")) http.Register( httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), @@ -261,7 +261,7 @@ func TestPRReview_interactive(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) http.Register( httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), @@ -293,7 +293,7 @@ func TestPRReview_interactive_no_body(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) pm := &prompter.PrompterMock{ SelectFunc: func(_, _ string, _ []string) (int, error) { return 2, nil }, @@ -308,7 +308,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) http.Register( httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index b509f946c7b..04e8baf2c61 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -10,6 +10,7 @@ import ( "sort" "strconv" "strings" + "testing" "time" "github.com/cli/cli/v2/api" @@ -55,9 +56,9 @@ type finder struct { } func NewFinder(factory *cmdutil.Factory) PRFinder { - if runCommandFinder != nil { - f := runCommandFinder - runCommandFinder = &mockFinder{err: errors.New("you must use a RunCommandFinder to stub PR lookups")} + if finderForRunCommandStyleTests != nil { + f := finderForRunCommandStyleTests + finderForRunCommandStyleTests = &mockFinder{err: errors.New("you must use StubFinderForRunCommandStyleTests to stub PR lookups")} return f } @@ -71,17 +72,24 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { } } -var runCommandFinder PRFinder +var finderForRunCommandStyleTests PRFinder -// RunCommandFinder is the NewMockFinder substitute to be used ONLY in runCommand-style tests. -func RunCommandFinder(selector string, pr *api.PullRequest, repo ghrepo.Interface) *mockFinder { +// StubFinderForRunCommandStyleTests is the NewMockFinder substitute to be used ONLY in runCommand-style tests. +func StubFinderForRunCommandStyleTests(t *testing.T, selector string, pr *api.PullRequest, repo ghrepo.Interface) *mockFinder { + // Create a new mock finder and override the "runCommandFinder" variable so that calls to + // NewFinder() will return this mock. This is a bad pattern, and a result of old style runCommand + // tests that would ideally be replaced. The reason we need to do this is that the runCommand style tests + // construct the cobra command via NewCmd* functions, and then Execute them directly, providing no opportunity + // to inject a test double unless it's on the factory, which finder never is, because only PR commands need it. finder := NewMockFinder(selector, pr, repo) - runCommandFinder = finder - return finder -} + finderForRunCommandStyleTests = finder -func ResetRunCommandFinder() { - runCommandFinder = nil + // Ensure that at the end of the test, we reset the "runCommandFinder" variable so that tests are isolated, + // at least if they are run sequentially. + t.Cleanup(func() { + finderForRunCommandStyleTests = nil + }) + return finder } type FindOptions struct { diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go index 3a2a87e5cf7..ec169130517 100644 --- a/pkg/cmd/pr/view/view_test.go +++ b/pkg/cmd/pr/view/view_test.go @@ -402,14 +402,12 @@ func TestPRView_Preview_nontty(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - t.Cleanup(shared.ResetRunCommandFinder) - http := &httpmock.Registry{} defer http.Verify(t) pr, err := prFromFixtures(tc.fixtures) require.NoError(t, err) - shared.RunCommandFinder("12", pr, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "12", pr, ghrepo.New("OWNER", "REPO")) output, err := runCommand(http, tc.branch, false, tc.args) if err != nil { @@ -608,14 +606,12 @@ func TestPRView_Preview(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - t.Cleanup(shared.ResetRunCommandFinder) - http := &httpmock.Registry{} defer http.Verify(t) pr, err := prFromFixtures(tc.fixtures) require.NoError(t, err) - shared.RunCommandFinder("12", pr, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "12", pr, ghrepo.New("OWNER", "REPO")) output, err := runCommand(http, tc.branch, true, tc.args) if err != nil { @@ -638,7 +634,7 @@ func TestPRView_web_currentBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/10"}, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/10"}, ghrepo.New("OWNER", "REPO")) _, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -657,7 +653,7 @@ func TestPRView_web_noResultsForBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - shared.RunCommandFinder("", nil, nil) + shared.StubFinderForRunCommandStyleTests(t, "", nil, nil) _, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -749,9 +745,9 @@ func TestPRView_tty_Comments(t *testing.T) { if len(tt.fixtures) > 0 { pr, err := prFromFixtures(tt.fixtures) require.NoError(t, err) - shared.RunCommandFinder("123", pr, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, ghrepo.New("OWNER", "REPO")) } else { - shared.RunCommandFinder("123", nil, nil) + shared.StubFinderForRunCommandStyleTests(t, "123", nil, nil) } output, err := runCommand(http, tt.branch, true, tt.cli) @@ -854,17 +850,15 @@ func TestPRView_nontty_Comments(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { - t.Cleanup(shared.ResetRunCommandFinder) - http := &httpmock.Registry{} defer http.Verify(t) if len(tt.fixtures) > 0 { pr, err := prFromFixtures(tt.fixtures) require.NoError(t, err) - shared.RunCommandFinder("123", pr, ghrepo.New("OWNER", "REPO")) + shared.StubFinderForRunCommandStyleTests(t, "123", pr, ghrepo.New("OWNER", "REPO")) } else { - shared.RunCommandFinder("123", nil, nil) + shared.StubFinderForRunCommandStyleTests(t, "123", nil, nil) } output, err := runCommand(http, tt.branch, false, tt.cli)