Skip to content

Commit f44f9a4

Browse files
Merge pull request cli#9307 from cli/wm/9160-stateReason-panic
Fix panic when calling `gh pr view --json stateReason`
2 parents ca1f2a4 + 940b54c commit f44f9a4

4 files changed

Lines changed: 134 additions & 8 deletions

File tree

api/query_builder.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ func RequiredStatusCheckRollupGraphQL(prID, after string, includeEvent bool) str
249249
}`), afterClause, prID, eventField)
250250
}
251251

252-
var IssueFields = []string{
252+
var sharedIssuePRFields = []string{
253253
"assignees",
254254
"author",
255255
"body",
@@ -268,10 +268,20 @@ var IssueFields = []string{
268268
"title",
269269
"updatedAt",
270270
"url",
271+
}
272+
273+
// Some fields are only valid in the context of issues.
274+
// They need to be enumerated separately in order to be filtered
275+
// from existing code that expects to be able to pass Issue fields
276+
// to PR queries, e.g. the PullRequestGraphql function.
277+
var issueOnlyFields = []string{
278+
"isPinned",
271279
"stateReason",
272280
}
273281

274-
var PullRequestFields = append(IssueFields,
282+
var IssueFields = append(sharedIssuePRFields, issueOnlyFields...)
283+
284+
var PullRequestFields = append(sharedIssuePRFields,
275285
"additions",
276286
"autoMergeRequest",
277287
"baseRefName",
@@ -299,12 +309,6 @@ var PullRequestFields = append(IssueFields,
299309
"statusCheckRollup",
300310
)
301311

302-
// Some fields are only valid in the context of issues.
303-
var issueOnlyFields = []string{
304-
"isPinned",
305-
"stateReason",
306-
}
307-
308312
// IssueGraphQL constructs a GraphQL query fragment for a set of issue fields.
309313
func IssueGraphQL(fields []string) string {
310314
var q []string

pkg/cmd/issue/view/view_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,37 @@ import (
1616
"github.com/cli/cli/v2/pkg/cmdutil"
1717
"github.com/cli/cli/v2/pkg/httpmock"
1818
"github.com/cli/cli/v2/pkg/iostreams"
19+
"github.com/cli/cli/v2/pkg/jsonfieldstest"
1920
"github.com/cli/cli/v2/test"
2021
"github.com/google/shlex"
2122
"github.com/stretchr/testify/assert"
2223
)
2324

25+
func TestJSONFields(t *testing.T) {
26+
jsonfieldstest.ExpectCommandToSupportJSONFields(t, NewCmdView, []string{
27+
"assignees",
28+
"author",
29+
"body",
30+
"closed",
31+
"comments",
32+
"createdAt",
33+
"closedAt",
34+
"id",
35+
"labels",
36+
"milestone",
37+
"number",
38+
"projectCards",
39+
"projectItems",
40+
"reactionGroups",
41+
"state",
42+
"title",
43+
"updatedAt",
44+
"url",
45+
"isPinned",
46+
"stateReason",
47+
})
48+
}
49+
2450
func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) {
2551
ios, _, stdout, stderr := iostreams.Test()
2652
ios.SetStdoutTTY(isTTY)

pkg/cmd/pr/view/view_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,61 @@ import (
1818
"github.com/cli/cli/v2/pkg/cmdutil"
1919
"github.com/cli/cli/v2/pkg/httpmock"
2020
"github.com/cli/cli/v2/pkg/iostreams"
21+
"github.com/cli/cli/v2/pkg/jsonfieldstest"
2122
"github.com/cli/cli/v2/test"
2223
"github.com/google/shlex"
2324
"github.com/stretchr/testify/assert"
2425
"github.com/stretchr/testify/require"
2526
)
2627

28+
func TestJSONFields(t *testing.T) {
29+
jsonfieldstest.ExpectCommandToSupportJSONFields(t, NewCmdView, []string{
30+
"additions",
31+
"assignees",
32+
"author",
33+
"autoMergeRequest",
34+
"baseRefName",
35+
"body",
36+
"changedFiles",
37+
"closed",
38+
"closedAt",
39+
"comments",
40+
"commits",
41+
"createdAt",
42+
"deletions",
43+
"files",
44+
"headRefName",
45+
"headRefOid",
46+
"headRepository",
47+
"headRepositoryOwner",
48+
"id",
49+
"isCrossRepository",
50+
"isDraft",
51+
"labels",
52+
"latestReviews",
53+
"maintainerCanModify",
54+
"mergeCommit",
55+
"mergeStateStatus",
56+
"mergeable",
57+
"mergedAt",
58+
"mergedBy",
59+
"milestone",
60+
"number",
61+
"potentialMergeCommit",
62+
"projectCards",
63+
"projectItems",
64+
"reactionGroups",
65+
"reviewDecision",
66+
"reviewRequests",
67+
"reviews",
68+
"state",
69+
"statusCheckRollup",
70+
"title",
71+
"updatedAt",
72+
"url",
73+
})
74+
}
75+
2776
func Test_NewCmdView(t *testing.T) {
2877
tests := []struct {
2978
name string
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package jsonfieldstest
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
"github.com/cli/cli/v2/pkg/cmdutil"
8+
"github.com/spf13/cobra"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
func jsonFieldsFor(cmd *cobra.Command) []string {
14+
// This annotation is added by the `cmdutil.AddJSONFlags` function.
15+
//
16+
// This is an extremely janky way to get access to this information but due to the fact we pass
17+
// around concrete cobra.Command structs, there's no viable way to have typed access to the fields.
18+
//
19+
// It's also kind of fragile because it's several hops away from the code that actually validates the usage
20+
// of these flags, so it's possible for things to get out of sync.
21+
stringFields, ok := cmd.Annotations["help:json-fields"]
22+
if !ok {
23+
return nil
24+
}
25+
26+
return strings.Split(stringFields, ",")
27+
}
28+
29+
// NewCmdFunc represents the typical function signature we use for creating commands e.g. `NewCmdView`.
30+
//
31+
// It is generic over `T` as each command construction has their own Options type e.g. `ViewOptions`
32+
type NewCmdFunc[T any] func(f *cmdutil.Factory, runF func(*T) error) *cobra.Command
33+
34+
// ExpectCommandToSupportJSONFields asserts that the provided command supports exactly the provided fields.
35+
// Ordering of the expected fields is not important.
36+
//
37+
// Make sure you are not pointing to the same slice of fields in the test and the implementation.
38+
// It can be a little tedious to rewrite the fields inline in the test but it's significantly more useful because:
39+
// - It forces the test author to think about and convey exactly the expected fields for a command
40+
// - It avoids accidentally adding fields to a command, and the test passing unintentionally
41+
func ExpectCommandToSupportJSONFields[T any](t *testing.T, fn NewCmdFunc[T], expectedFields []string) {
42+
t.Helper()
43+
44+
actualFields := jsonFieldsFor(fn(&cmdutil.Factory{}, nil))
45+
assert.Equal(t, len(actualFields), len(expectedFields), "expected number of fields to match")
46+
require.ElementsMatch(t, expectedFields, actualFields)
47+
}

0 commit comments

Comments
 (0)