diff --git a/api/query_builder.go b/api/query_builder.go index 0f131232c..0aa89513d 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -249,7 +249,7 @@ func RequiredStatusCheckRollupGraphQL(prID, after string, includeEvent bool) str }`), afterClause, prID, eventField) } -var IssueFields = []string{ +var sharedIssuePRFields = []string{ "assignees", "author", "body", @@ -268,10 +268,20 @@ var IssueFields = []string{ "title", "updatedAt", "url", +} + +// Some fields are only valid in the context of issues. +// They need to be enumerated separately in order to be filtered +// from existing code that expects to be able to pass Issue fields +// to PR queries, e.g. the PullRequestGraphql function. +var issueOnlyFields = []string{ + "isPinned", "stateReason", } -var PullRequestFields = append(IssueFields, +var IssueFields = append(sharedIssuePRFields, issueOnlyFields...) + +var PullRequestFields = append(sharedIssuePRFields, "additions", "autoMergeRequest", "baseRefName", @@ -299,12 +309,6 @@ var PullRequestFields = append(IssueFields, "statusCheckRollup", ) -// Some fields are only valid in the context of issues. -var issueOnlyFields = []string{ - "isPinned", - "stateReason", -} - // IssueGraphQL constructs a GraphQL query fragment for a set of issue fields. func IssueGraphQL(fields []string) string { var q []string diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index 2f91ba13e..f77db9abc 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -16,11 +16,37 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/jsonfieldstest" "github.com/cli/cli/v2/test" "github.com/google/shlex" "github.com/stretchr/testify/assert" ) +func TestJSONFields(t *testing.T) { + jsonfieldstest.ExpectCommandToSupportJSONFields(t, NewCmdView, []string{ + "assignees", + "author", + "body", + "closed", + "comments", + "createdAt", + "closedAt", + "id", + "labels", + "milestone", + "number", + "projectCards", + "projectItems", + "reactionGroups", + "state", + "title", + "updatedAt", + "url", + "isPinned", + "stateReason", + }) +} + func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(isTTY) diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go index 6fc955bc3..0efe667a7 100644 --- a/pkg/cmd/pr/view/view_test.go +++ b/pkg/cmd/pr/view/view_test.go @@ -18,12 +18,61 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/jsonfieldstest" "github.com/cli/cli/v2/test" "github.com/google/shlex" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestJSONFields(t *testing.T) { + jsonfieldstest.ExpectCommandToSupportJSONFields(t, NewCmdView, []string{ + "additions", + "assignees", + "author", + "autoMergeRequest", + "baseRefName", + "body", + "changedFiles", + "closed", + "closedAt", + "comments", + "commits", + "createdAt", + "deletions", + "files", + "headRefName", + "headRefOid", + "headRepository", + "headRepositoryOwner", + "id", + "isCrossRepository", + "isDraft", + "labels", + "latestReviews", + "maintainerCanModify", + "mergeCommit", + "mergeStateStatus", + "mergeable", + "mergedAt", + "mergedBy", + "milestone", + "number", + "potentialMergeCommit", + "projectCards", + "projectItems", + "reactionGroups", + "reviewDecision", + "reviewRequests", + "reviews", + "state", + "statusCheckRollup", + "title", + "updatedAt", + "url", + }) +} + func Test_NewCmdView(t *testing.T) { tests := []struct { name string diff --git a/pkg/jsonfieldstest/jsonfieldstest.go b/pkg/jsonfieldstest/jsonfieldstest.go new file mode 100644 index 000000000..21140024b --- /dev/null +++ b/pkg/jsonfieldstest/jsonfieldstest.go @@ -0,0 +1,47 @@ +package jsonfieldstest + +import ( + "strings" + "testing" + + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func jsonFieldsFor(cmd *cobra.Command) []string { + // This annotation is added by the `cmdutil.AddJSONFlags` function. + // + // This is an extremely janky way to get access to this information but due to the fact we pass + // around concrete cobra.Command structs, there's no viable way to have typed access to the fields. + // + // It's also kind of fragile because it's several hops away from the code that actually validates the usage + // of these flags, so it's possible for things to get out of sync. + stringFields, ok := cmd.Annotations["help:json-fields"] + if !ok { + return nil + } + + return strings.Split(stringFields, ",") +} + +// NewCmdFunc represents the typical function signature we use for creating commands e.g. `NewCmdView`. +// +// It is generic over `T` as each command construction has their own Options type e.g. `ViewOptions` +type NewCmdFunc[T any] func(f *cmdutil.Factory, runF func(*T) error) *cobra.Command + +// ExpectCommandToSupportJSONFields asserts that the provided command supports exactly the provided fields. +// Ordering of the expected fields is not important. +// +// Make sure you are not pointing to the same slice of fields in the test and the implementation. +// It can be a little tedious to rewrite the fields inline in the test but it's significantly more useful because: +// - It forces the test author to think about and convey exactly the expected fields for a command +// - It avoids accidentally adding fields to a command, and the test passing unintentionally +func ExpectCommandToSupportJSONFields[T any](t *testing.T, fn NewCmdFunc[T], expectedFields []string) { + t.Helper() + + actualFields := jsonFieldsFor(fn(&cmdutil.Factory{}, nil)) + assert.Equal(t, len(actualFields), len(expectedFields), "expected number of fields to match") + require.ElementsMatch(t, expectedFields, actualFields) +}