From 35e8cc93cfc85ad5e710a995cca2a041b582e1f2 Mon Sep 17 00:00:00 2001 From: Max Beizer Date: Tue, 21 Apr 2026 09:12:31 -0500 Subject: [PATCH 1/6] test(discussion): add httpmock unit tests for DiscussionClient Add comprehensive httpmock-based unit tests for the client package covering: - List: success path, discussions disabled, limit/filter validation, pagination - Search: success path, filter validation, pagination - ListCategories: success path, discussions disabled Tests use httpmock.Registry with defer Verify(t) to ensure all stubs are exercised, following the established testing pattern in this repo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/discussion/client/client_impl_test.go | 504 ++++++++++++++++++ 1 file changed, 504 insertions(+) create mode 100644 pkg/cmd/discussion/client/client_impl_test.go diff --git a/pkg/cmd/discussion/client/client_impl_test.go b/pkg/cmd/discussion/client/client_impl_test.go new file mode 100644 index 000000000..bcc678b5c --- /dev/null +++ b/pkg/cmd/discussion/client/client_impl_test.go @@ -0,0 +1,504 @@ +package client + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestDiscussionClient(reg *httpmock.Registry) DiscussionClient { + httpClient := &http.Client{} + httpmock.ReplaceTripper(httpClient, reg) + return NewDiscussionClient(httpClient) +} + +func ptr[T any](v T) *T { return &v } + +// --------------------------------------------------------------------------- +// List +// --------------------------------------------------------------------------- + +func TestList_success(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": true, + "discussions": { + "totalCount": 1, + "pageInfo": {"hasNextPage": false, "endCursor": ""}, + "nodes": [{ + "id": "D_id1", + "number": 1, + "title": "Hello world", + "body": "body text", + "url": "https://github.com/OWNER/REPO/discussions/1", + "closed": false, + "stateReason": "", + "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": {"__typename": "User", "login": "alice", "id": "U1", "name": "Alice"}, + "category": {"id": "C1", "name": "General", "slug": "general", "emoji": ":speech_balloon:", "isAnswerable": false}, + "answerChosenBy": null, + "labels": {"nodes": []}, + "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", + "updatedAt": "2024-01-02T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", + "locked": false + }] + } + }}}`), + ) + + c := newTestDiscussionClient(reg) + result, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 10) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, 1, result.TotalCount) + assert.Len(t, result.Discussions, 1) + assert.Equal(t, "Hello world", result.Discussions[0].Title) + assert.Equal(t, "", result.NextCursor) +} + +func TestList_discussionsDisabled(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": false, + "discussions": {"totalCount": 0, "pageInfo": {"hasNextPage": false, "endCursor": ""}, "nodes": []} + }}}`), + ) + + c := newTestDiscussionClient(reg) + _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "discussions disabled") +} + +func TestList_limitZero(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 0) + require.Error(t, err) + assert.Contains(t, err.Error(), "limit argument must be positive") +} + +func TestList_invalidOrderBy(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{OrderBy: "invalid"}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown order-by field") +} + +func TestList_invalidDirection(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{Direction: "sideways"}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown order direction") +} + +func TestList_invalidState(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{State: ptr("merged")}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown state filter") +} + +func TestList_pagination(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + // First page + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": true, + "discussions": { + "totalCount": 2, + "pageInfo": {"hasNextPage": true, "endCursor": "cursor1"}, + "nodes": [{ + "id": "D1", "number": 1, "title": "Discussion 1", "body": "", + "url": "", "closed": false, "stateReason": "", "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": {"__typename": "User", "login": "alice"}, + "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, + "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", "locked": false + }] + } + }}}`), + ) + // Second page + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": true, + "discussions": { + "totalCount": 2, + "pageInfo": {"hasNextPage": false, "endCursor": ""}, + "nodes": [{ + "id": "D2", "number": 2, "title": "Discussion 2", "body": "", + "url": "", "closed": false, "stateReason": "", "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": {"__typename": "User", "login": "bob"}, + "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, + "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], + "createdAt": "2024-01-02T00:00:00Z", "updatedAt": "2024-01-02T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", "locked": false + }] + } + }}}`), + ) + + c := newTestDiscussionClient(reg) + // limit > 1 forces pagination across both pages + result, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 2) + require.NoError(t, err) + assert.Len(t, result.Discussions, 2) + assert.Equal(t, "Discussion 1", result.Discussions[0].Title) + assert.Equal(t, "Discussion 2", result.Discussions[1].Title) + assert.Equal(t, "", result.NextCursor) +} + +func TestList_paginationSetsNextCursor(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + // When the caller requests fewer items than are available, NextCursor should be set. + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": true, + "discussions": { + "totalCount": 5, + "pageInfo": {"hasNextPage": true, "endCursor": "cursor42"}, + "nodes": [{ + "id": "D1", "number": 1, "title": "Discussion 1", "body": "", + "url": "", "closed": false, "stateReason": "", "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": {"__typename": "User", "login": "alice"}, + "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, + "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", "locked": false + }] + } + }}}`), + ) + + c := newTestDiscussionClient(reg) + result, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 1) + require.NoError(t, err) + assert.Len(t, result.Discussions, 1) + assert.Equal(t, "cursor42", result.NextCursor) +} + +func TestList_filters(t *testing.T) { + tests := []struct { + name string + filters ListFilters + }{ + { + name: "open state", + filters: ListFilters{State: ptr(FilterStateOpen)}, + }, + { + name: "closed state", + filters: ListFilters{State: ptr(FilterStateClosed)}, + }, + { + name: "answered", + filters: ListFilters{Answered: ptr(true)}, + }, + { + name: "unanswered", + filters: ListFilters{Answered: ptr(false)}, + }, + { + name: "category ID", + filters: ListFilters{CategoryID: "CAT123"}, + }, + { + name: "order by created asc", + filters: ListFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + }, + { + name: "order by updated desc", + filters: ListFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": true, + "discussions": {"totalCount": 0, "pageInfo": {"hasNextPage": false, "endCursor": ""}, "nodes": []} + }}}`), + ) + + c := newTestDiscussionClient(reg) + result, err := c.List(ghrepo.New("OWNER", "REPO"), tt.filters, "", 10) + require.NoError(t, err) + assert.NotNil(t, result) + }) + } +} + +// --------------------------------------------------------------------------- +// Search +// --------------------------------------------------------------------------- + +func TestSearch_success(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(`{"data":{"search":{ + "discussionCount": 1, + "pageInfo": {"hasNextPage": false, "endCursor": ""}, + "nodes": [{ + "id": "D1", "number": 1, "title": "Searched discussion", "body": "", + "url": "", "closed": false, "stateReason": "", "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": {"__typename": "User", "login": "alice"}, + "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, + "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", "locked": false + }] + }}}`), + ) + + c := newTestDiscussionClient(reg) + result, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{}, "", 10) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, 1, result.TotalCount) + assert.Len(t, result.Discussions, 1) + assert.Equal(t, "Searched discussion", result.Discussions[0].Title) +} + +func TestSearch_limitZero(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{}, "", 0) + require.Error(t, err) + assert.Contains(t, err.Error(), "limit argument must be positive") +} + +func TestSearch_invalidOrderBy(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{OrderBy: "bogus"}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown order-by field") +} + +func TestSearch_invalidDirection(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{Direction: "sideways"}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown order direction") +} + +func TestSearch_invalidState(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + c := newTestDiscussionClient(reg) + _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{State: ptr("merged")}, "", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown state filter") +} + +func TestSearch_filters(t *testing.T) { + tests := []struct { + name string + filters SearchFilters + }{ + { + name: "open state", + filters: SearchFilters{State: ptr(FilterStateOpen)}, + }, + { + name: "closed state", + filters: SearchFilters{State: ptr(FilterStateClosed)}, + }, + { + name: "answered", + filters: SearchFilters{Answered: ptr(true)}, + }, + { + name: "unanswered", + filters: SearchFilters{Answered: ptr(false)}, + }, + { + name: "author", + filters: SearchFilters{Author: "alice"}, + }, + { + name: "labels", + filters: SearchFilters{Labels: []string{"bug", "enhancement"}}, + }, + { + name: "category", + filters: SearchFilters{Category: "Q&A"}, + }, + { + name: "keywords", + filters: SearchFilters{Keywords: "some keyword"}, + }, + { + name: "order by created asc", + filters: SearchFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + }, + { + name: "order by updated desc", + filters: SearchFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(`{"data":{"search":{ + "discussionCount": 0, + "pageInfo": {"hasNextPage": false, "endCursor": ""}, + "nodes": [] + }}}`), + ) + + c := newTestDiscussionClient(reg) + result, err := c.Search(ghrepo.New("OWNER", "REPO"), tt.filters, "", 10) + require.NoError(t, err) + assert.NotNil(t, result) + }) + } +} + +func TestSearch_pagination(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + makeNode := func(id, title string) string { + return `{ + "id": "` + id + `", "number": 1, "title": "` + title + `", "body": "", + "url": "", "closed": false, "stateReason": "", "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": {"__typename": "User", "login": "alice"}, + "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, + "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", "locked": false + }` + } + + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(`{"data":{"search":{ + "discussionCount": 2, + "pageInfo": {"hasNextPage": true, "endCursor": "searchCursor1"}, + "nodes": [`+makeNode("D1", "First")+`] + }}}`), + ) + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(`{"data":{"search":{ + "discussionCount": 2, + "pageInfo": {"hasNextPage": false, "endCursor": ""}, + "nodes": [`+makeNode("D2", "Second")+`] + }}}`), + ) + + c := newTestDiscussionClient(reg) + result, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{}, "", 2) + require.NoError(t, err) + assert.Len(t, result.Discussions, 2) + assert.Equal(t, "First", result.Discussions[0].Title) + assert.Equal(t, "Second", result.Discussions[1].Title) +} + +// --------------------------------------------------------------------------- +// ListCategories +// --------------------------------------------------------------------------- + +func TestListCategories_success(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionCategoryList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": true, + "discussionCategories": {"nodes": [ + {"id": "C1", "name": "General", "slug": "general", "emoji": ":speech_balloon:", "isAnswerable": false}, + {"id": "C2", "name": "Q&A", "slug": "q-a", "emoji": ":question:", "isAnswerable": true} + ]} + }}}`), + ) + + c := newTestDiscussionClient(reg) + categories, err := c.ListCategories(ghrepo.New("OWNER", "REPO")) + require.NoError(t, err) + require.Len(t, categories, 2) + assert.Equal(t, "General", categories[0].Name) + assert.Equal(t, "Q&A", categories[1].Name) + assert.True(t, categories[1].IsAnswerable) +} + +func TestListCategories_discussionsDisabled(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query DiscussionCategoryList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled": false, + "discussionCategories": {"nodes": []} + }}}`), + ) + + c := newTestDiscussionClient(reg) + _, err := c.ListCategories(ghrepo.New("OWNER", "REPO")) + require.Error(t, err) + assert.Contains(t, err.Error(), "discussions disabled") +} From aa080ad28aa74425c45901fd8d2c729bb7a4f7a0 Mon Sep 17 00:00:00 2001 From: Max Beizer Date: Wed, 22 Apr 2026 15:04:59 -0500 Subject: [PATCH 2/6] refactor(discussion): convert client tests to table-driven Addresses babakks' review on PR #13252: - Convert 18 individual test functions to 3 table-driven functions - Replace ptr helper with new(value) syntax throughout - Assert all Discussion struct fields in success cases - Add after-cursor test cases for pagination - Fold pagination tests into table structure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/discussion/client/client_impl_test.go | 829 +++++++++--------- 1 file changed, 431 insertions(+), 398 deletions(-) diff --git a/pkg/cmd/discussion/client/client_impl_test.go b/pkg/cmd/discussion/client/client_impl_test.go index bcc678b5c..cb4a89f62 100644 --- a/pkg/cmd/discussion/client/client_impl_test.go +++ b/pkg/cmd/discussion/client/client_impl_test.go @@ -3,6 +3,7 @@ package client import ( "net/http" "testing" + "time" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" @@ -16,237 +17,214 @@ func newTestDiscussionClient(reg *httpmock.Registry) DiscussionClient { return NewDiscussionClient(httpClient) } -func ptr[T any](v T) *T { return &v } +// minimalNode returns a minimal JSON discussion node with the given id and title. +func minimalNode(id, title string) string { + return `{"id":"` + id + `","number":1,"title":"` + title + `","body":"","url":"","closed":false,"stateReason":"","isAnswered":false,"answerChosenAt":"0001-01-01T00:00:00Z","author":{"__typename":"User","login":"alice"},"category":{"id":"C1","name":"General","slug":"general","emoji":"","isAnswerable":false},"answerChosenBy":null,"labels":{"nodes":[]},"reactionGroups":[],"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","closedAt":"0001-01-01T00:00:00Z","locked":false}` +} + +// listResp builds a mock repository.discussions JSON response. +func listResp(hasNext bool, cursor string, total int, nodes string) string { + hasNextStr := "false" + if hasNext { + hasNextStr = "true" + } + return `{"data":{"repository":{"hasDiscussionsEnabled":true,"discussions":{"totalCount":` + + intStr(total) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}}` +} + +// searchResp builds a mock search JSON response. +func searchResp(hasNext bool, cursor string, count int, nodes string) string { + hasNextStr := "false" + if hasNext { + hasNextStr = "true" + } + return `{"data":{"search":{"discussionCount":` + + intStr(count) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}` +} + +// intStr converts an int to its decimal string representation without importing strconv. +func intStr(n int) string { + if n == 0 { + return "0" + } + neg := n < 0 + if neg { + n = -n + } + buf := [20]byte{} + pos := len(buf) + for n > 0 { + pos-- + buf[pos] = byte('0' + n%10) + n /= 10 + } + if neg { + pos-- + buf[pos] = '-' + } + return string(buf[pos:]) +} // --------------------------------------------------------------------------- // List // --------------------------------------------------------------------------- -func TestList_success(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) +func TestList(t *testing.T) { + repo := ghrepo.New("OWNER", "REPO") - reg.Register( - httpmock.GraphQL(`query DiscussionList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": true, - "discussions": { - "totalCount": 1, - "pageInfo": {"hasNextPage": false, "endCursor": ""}, - "nodes": [{ - "id": "D_id1", - "number": 1, - "title": "Hello world", - "body": "body text", - "url": "https://github.com/OWNER/REPO/discussions/1", - "closed": false, - "stateReason": "", - "isAnswered": false, - "answerChosenAt": "0001-01-01T00:00:00Z", - "author": {"__typename": "User", "login": "alice", "id": "U1", "name": "Alice"}, - "category": {"id": "C1", "name": "General", "slug": "general", "emoji": ":speech_balloon:", "isAnswerable": false}, - "answerChosenBy": null, - "labels": {"nodes": []}, - "reactionGroups": [], - "createdAt": "2024-01-01T00:00:00Z", - "updatedAt": "2024-01-02T00:00:00Z", - "closedAt": "0001-01-01T00:00:00Z", - "locked": false - }] - } - }}}`), - ) + richNode := `{ + "id":"D_rich1","number":42,"title":"Rich discussion","body":"body text here", + "url":"https://github.com/OWNER/REPO/discussions/42", + "closed":true,"stateReason":"RESOLVED","isAnswered":true, + "answerChosenAt":"2024-06-01T12:00:00Z", + "author":{"__typename":"User","login":"alice","id":"U1","name":"Alice"}, + "category":{"id":"C1","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true}, + "answerChosenBy":{"__typename":"User","login":"bob","id":"U2","name":"Bob"}, + "labels":{"nodes":[{"id":"L1","name":"bug","color":"d73a4a"},{"id":"L2","name":"enhancement","color":"a2eeef"}]}, + "reactionGroups":[], + "createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-06-02T00:00:00Z", + "closedAt":"2024-06-01T00:00:00Z","locked":true + }` - c := newTestDiscussionClient(reg) - result, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 10) - require.NoError(t, err) - require.NotNil(t, result) - assert.Equal(t, 1, result.TotalCount) - assert.Len(t, result.Discussions, 1) - assert.Equal(t, "Hello world", result.Discussions[0].Title) - assert.Equal(t, "", result.NextCursor) -} + emptyResp := listResp(false, "", 0, "") + disabledResp := `{"data":{"repository":{"hasDiscussionsEnabled":false,"discussions":{"totalCount":0,"pageInfo":{"hasNextPage":false,"endCursor":""},"nodes":[]}}}}` -func TestList_discussionsDisabled(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - reg.Register( - httpmock.GraphQL(`query DiscussionList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": false, - "discussions": {"totalCount": 0, "pageInfo": {"hasNextPage": false, "endCursor": ""}, "nodes": []} - }}}`), - ) - - c := newTestDiscussionClient(reg) - _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "discussions disabled") -} - -func TestList_limitZero(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 0) - require.Error(t, err) - assert.Contains(t, err.Error(), "limit argument must be positive") -} - -func TestList_invalidOrderBy(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{OrderBy: "invalid"}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "unknown order-by field") -} - -func TestList_invalidDirection(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{Direction: "sideways"}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "unknown order direction") -} - -func TestList_invalidState(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{State: ptr("merged")}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "unknown state filter") -} - -func TestList_pagination(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - // First page - reg.Register( - httpmock.GraphQL(`query DiscussionList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": true, - "discussions": { - "totalCount": 2, - "pageInfo": {"hasNextPage": true, "endCursor": "cursor1"}, - "nodes": [{ - "id": "D1", "number": 1, "title": "Discussion 1", "body": "", - "url": "", "closed": false, "stateReason": "", "isAnswered": false, - "answerChosenAt": "0001-01-01T00:00:00Z", - "author": {"__typename": "User", "login": "alice"}, - "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, - "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], - "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", - "closedAt": "0001-01-01T00:00:00Z", "locked": false - }] - } - }}}`), - ) - // Second page - reg.Register( - httpmock.GraphQL(`query DiscussionList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": true, - "discussions": { - "totalCount": 2, - "pageInfo": {"hasNextPage": false, "endCursor": ""}, - "nodes": [{ - "id": "D2", "number": 2, "title": "Discussion 2", "body": "", - "url": "", "closed": false, "stateReason": "", "isAnswered": false, - "answerChosenAt": "0001-01-01T00:00:00Z", - "author": {"__typename": "User", "login": "bob"}, - "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, - "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], - "createdAt": "2024-01-02T00:00:00Z", "updatedAt": "2024-01-02T00:00:00Z", - "closedAt": "0001-01-01T00:00:00Z", "locked": false - }] - } - }}}`), - ) - - c := newTestDiscussionClient(reg) - // limit > 1 forces pagination across both pages - result, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 2) - require.NoError(t, err) - assert.Len(t, result.Discussions, 2) - assert.Equal(t, "Discussion 1", result.Discussions[0].Title) - assert.Equal(t, "Discussion 2", result.Discussions[1].Title) - assert.Equal(t, "", result.NextCursor) -} - -func TestList_paginationSetsNextCursor(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - // When the caller requests fewer items than are available, NextCursor should be set. - reg.Register( - httpmock.GraphQL(`query DiscussionList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": true, - "discussions": { - "totalCount": 5, - "pageInfo": {"hasNextPage": true, "endCursor": "cursor42"}, - "nodes": [{ - "id": "D1", "number": 1, "title": "Discussion 1", "body": "", - "url": "", "closed": false, "stateReason": "", "isAnswered": false, - "answerChosenAt": "0001-01-01T00:00:00Z", - "author": {"__typename": "User", "login": "alice"}, - "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, - "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], - "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", - "closedAt": "0001-01-01T00:00:00Z", "locked": false - }] - } - }}}`), - ) - - c := newTestDiscussionClient(reg) - result, err := c.List(ghrepo.New("OWNER", "REPO"), ListFilters{}, "", 1) - require.NoError(t, err) - assert.Len(t, result.Discussions, 1) - assert.Equal(t, "cursor42", result.NextCursor) -} - -func TestList_filters(t *testing.T) { tests := []struct { - name string - filters ListFilters + name string + filters ListFilters + after string + limit int + responses []string + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantDisc *Discussion }{ { - name: "open state", - filters: ListFilters{State: ptr(FilterStateOpen)}, + name: "maps all fields", + limit: 10, + responses: []string{listResp(false, "", 1, richNode)}, + wantTotal: 1, + wantLen: 1, + wantDisc: &Discussion{ + ID: "D_rich1", Number: 42, Title: "Rich discussion", Body: "body text here", + URL: "https://github.com/OWNER/REPO/discussions/42", + Closed: true, + StateReason: "RESOLVED", + Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, + Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, + Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}, {ID: "L2", Name: "enhancement", Color: "a2eeef"}}, + Answered: true, + AnswerChosenAt: time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC), + AnswerChosenBy: &DiscussionActor{ID: "U2", Login: "bob", Name: "Bob"}, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), + ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), + Locked: true, + }, }, { - name: "closed state", - filters: ListFilters{State: ptr(FilterStateClosed)}, + name: "discussions disabled", + limit: 10, + responses: []string{disabledResp}, + wantErr: "discussions disabled", }, { - name: "answered", - filters: ListFilters{Answered: ptr(true)}, + name: "limit zero", + limit: 0, + wantErr: "limit argument must be positive", }, { - name: "unanswered", - filters: ListFilters{Answered: ptr(false)}, + name: "invalid orderBy", + limit: 10, + filters: ListFilters{OrderBy: "invalid"}, + wantErr: "unknown order-by field", }, { - name: "category ID", - filters: ListFilters{CategoryID: "CAT123"}, + name: "invalid direction", + limit: 10, + filters: ListFilters{Direction: "sideways"}, + wantErr: "unknown order direction", }, { - name: "order by created asc", - filters: ListFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + name: "invalid state", + limit: 10, + filters: ListFilters{State: new("merged")}, + wantErr: "unknown state filter", }, { - name: "order by updated desc", - filters: ListFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + name: "with after cursor", + limit: 10, + after: "someCursor", + responses: []string{emptyResp}, + }, + { + name: "open state filter", + limit: 10, + filters: ListFilters{State: new(FilterStateOpen)}, + responses: []string{emptyResp}, + }, + { + name: "closed state filter", + limit: 10, + filters: ListFilters{State: new(FilterStateClosed)}, + responses: []string{emptyResp}, + }, + { + name: "answered filter", + limit: 10, + filters: ListFilters{Answered: new(true)}, + responses: []string{emptyResp}, + }, + { + name: "unanswered filter", + limit: 10, + filters: ListFilters{Answered: new(false)}, + responses: []string{emptyResp}, + }, + { + name: "category ID filter", + limit: 10, + filters: ListFilters{CategoryID: "CAT123"}, + responses: []string{emptyResp}, + }, + { + name: "order by created asc", + limit: 10, + filters: ListFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + responses: []string{emptyResp}, + }, + { + name: "order by updated desc", + limit: 10, + filters: ListFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + responses: []string{emptyResp}, + }, + { + // When the page has more items than requested, NextCursor is set. + name: "pagination sets next cursor", + limit: 1, + responses: []string{ + listResp(true, "cursor42", 5, minimalNode("D1", "Discussion 1")), + }, + wantLen: 1, + wantTotal: 5, + wantCursor: "cursor42", + }, + { + // Two pages are fetched when limit exceeds the first page's results. + name: "pagination fetches multiple pages", + limit: 2, + responses: []string{ + listResp(true, "cursor1", 2, minimalNode("D1", "First")), + listResp(false, "", 2, minimalNode("D2", "Second")), + }, + wantLen: 2, + wantTotal: 2, + wantTitles: []string{"First", "Second"}, }, } @@ -255,18 +233,33 @@ func TestList_filters(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - reg.Register( - httpmock.GraphQL(`query DiscussionList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": true, - "discussions": {"totalCount": 0, "pageInfo": {"hasNextPage": false, "endCursor": ""}, "nodes": []} - }}}`), - ) + for _, resp := range tt.responses { + reg.Register(httpmock.GraphQL(`query DiscussionList\b`), httpmock.StringResponse(resp)) + } c := newTestDiscussionClient(reg) - result, err := c.List(ghrepo.New("OWNER", "REPO"), tt.filters, "", 10) + result, err := c.List(repo, tt.filters, tt.after, tt.limit) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) - assert.NotNil(t, result) + require.NotNil(t, result) + assert.Equal(t, tt.wantTotal, result.TotalCount) + assert.Len(t, result.Discussions, tt.wantLen) + assert.Equal(t, tt.wantCursor, result.NextCursor) + + for i, title := range tt.wantTitles { + assert.Equal(t, title, result.Discussions[i].Title) + } + + if tt.wantDisc != nil { + require.NotEmpty(t, result.Discussions) + assert.Equal(t, *tt.wantDisc, result.Discussions[0]) + } }) } } @@ -275,121 +268,173 @@ func TestList_filters(t *testing.T) { // Search // --------------------------------------------------------------------------- -func TestSearch_success(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) +func TestSearch(t *testing.T) { + repo := ghrepo.New("OWNER", "REPO") - reg.Register( - httpmock.GraphQL(`query DiscussionListSearch\b`), - httpmock.StringResponse(`{"data":{"search":{ - "discussionCount": 1, - "pageInfo": {"hasNextPage": false, "endCursor": ""}, - "nodes": [{ - "id": "D1", "number": 1, "title": "Searched discussion", "body": "", - "url": "", "closed": false, "stateReason": "", "isAnswered": false, - "answerChosenAt": "0001-01-01T00:00:00Z", - "author": {"__typename": "User", "login": "alice"}, - "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, - "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], - "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", - "closedAt": "0001-01-01T00:00:00Z", "locked": false - }] - }}}`), - ) + richNode := `{ + "id":"D_rich1","number":42,"title":"Rich search result","body":"body text here", + "url":"https://github.com/OWNER/REPO/discussions/42", + "closed":true,"stateReason":"RESOLVED","isAnswered":true, + "answerChosenAt":"2024-06-01T12:00:00Z", + "author":{"__typename":"User","login":"alice","id":"U1","name":"Alice"}, + "category":{"id":"C1","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true}, + "answerChosenBy":{"__typename":"User","login":"bob","id":"U2","name":"Bob"}, + "labels":{"nodes":[{"id":"L1","name":"bug","color":"d73a4a"}]}, + "reactionGroups":[], + "createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-06-02T00:00:00Z", + "closedAt":"2024-06-01T00:00:00Z","locked":true + }` - c := newTestDiscussionClient(reg) - result, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{}, "", 10) - require.NoError(t, err) - require.NotNil(t, result) - assert.Equal(t, 1, result.TotalCount) - assert.Len(t, result.Discussions, 1) - assert.Equal(t, "Searched discussion", result.Discussions[0].Title) -} + emptyResp := searchResp(false, "", 0, "") -func TestSearch_limitZero(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{}, "", 0) - require.Error(t, err) - assert.Contains(t, err.Error(), "limit argument must be positive") -} - -func TestSearch_invalidOrderBy(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{OrderBy: "bogus"}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "unknown order-by field") -} - -func TestSearch_invalidDirection(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{Direction: "sideways"}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "unknown order direction") -} - -func TestSearch_invalidState(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - c := newTestDiscussionClient(reg) - _, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{State: ptr("merged")}, "", 10) - require.Error(t, err) - assert.Contains(t, err.Error(), "unknown state filter") -} - -func TestSearch_filters(t *testing.T) { tests := []struct { - name string - filters SearchFilters + name string + filters SearchFilters + after string + limit int + responses []string + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantDisc *Discussion }{ { - name: "open state", - filters: SearchFilters{State: ptr(FilterStateOpen)}, + name: "maps all fields", + limit: 10, + responses: []string{searchResp(false, "", 1, richNode)}, + wantTotal: 1, + wantLen: 1, + wantDisc: &Discussion{ + ID: "D_rich1", Number: 42, Title: "Rich search result", Body: "body text here", + URL: "https://github.com/OWNER/REPO/discussions/42", + Closed: true, + StateReason: "RESOLVED", + Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, + Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, + Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}}, + Answered: true, + AnswerChosenAt: time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC), + AnswerChosenBy: &DiscussionActor{ID: "U2", Login: "bob", Name: "Bob"}, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), + ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), + Locked: true, + }, }, { - name: "closed state", - filters: SearchFilters{State: ptr(FilterStateClosed)}, + name: "limit zero", + limit: 0, + wantErr: "limit argument must be positive", }, { - name: "answered", - filters: SearchFilters{Answered: ptr(true)}, + name: "invalid orderBy", + limit: 10, + filters: SearchFilters{OrderBy: "bogus"}, + wantErr: "unknown order-by field", }, { - name: "unanswered", - filters: SearchFilters{Answered: ptr(false)}, + name: "invalid direction", + limit: 10, + filters: SearchFilters{Direction: "sideways"}, + wantErr: "unknown order direction", }, { - name: "author", - filters: SearchFilters{Author: "alice"}, + name: "invalid state", + limit: 10, + filters: SearchFilters{State: new("merged")}, + wantErr: "unknown state filter", }, { - name: "labels", - filters: SearchFilters{Labels: []string{"bug", "enhancement"}}, + name: "with after cursor", + limit: 10, + after: "someCursor", + responses: []string{emptyResp}, }, { - name: "category", - filters: SearchFilters{Category: "Q&A"}, + name: "open state filter", + limit: 10, + filters: SearchFilters{State: new(FilterStateOpen)}, + responses: []string{emptyResp}, }, { - name: "keywords", - filters: SearchFilters{Keywords: "some keyword"}, + name: "closed state filter", + limit: 10, + filters: SearchFilters{State: new(FilterStateClosed)}, + responses: []string{emptyResp}, }, { - name: "order by created asc", - filters: SearchFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + name: "answered filter", + limit: 10, + filters: SearchFilters{Answered: new(true)}, + responses: []string{emptyResp}, }, { - name: "order by updated desc", - filters: SearchFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + name: "unanswered filter", + limit: 10, + filters: SearchFilters{Answered: new(false)}, + responses: []string{emptyResp}, + }, + { + name: "author filter", + limit: 10, + filters: SearchFilters{Author: "alice"}, + responses: []string{emptyResp}, + }, + { + name: "labels filter", + limit: 10, + filters: SearchFilters{Labels: []string{"bug", "enhancement"}}, + responses: []string{emptyResp}, + }, + { + name: "category filter", + limit: 10, + filters: SearchFilters{Category: "Q&A"}, + responses: []string{emptyResp}, + }, + { + name: "keywords filter", + limit: 10, + filters: SearchFilters{Keywords: "some keyword"}, + responses: []string{emptyResp}, + }, + { + name: "order by created asc", + limit: 10, + filters: SearchFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + responses: []string{emptyResp}, + }, + { + name: "order by updated desc", + limit: 10, + filters: SearchFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + responses: []string{emptyResp}, + }, + { + // When the page has more items than requested, NextCursor is set. + name: "pagination sets next cursor", + limit: 1, + responses: []string{ + searchResp(true, "searchCursor42", 5, minimalNode("D1", "Discussion 1")), + }, + wantLen: 1, + wantTotal: 5, + wantCursor: "searchCursor42", + }, + { + // Two pages are fetched when limit exceeds the first page's results. + name: "pagination fetches multiple pages", + limit: 2, + responses: []string{ + searchResp(true, "searchCursor1", 2, minimalNode("D1", "First")), + searchResp(false, "", 2, minimalNode("D2", "Second")), + }, + wantLen: 2, + wantTotal: 2, + wantTitles: []string{"First", "Second"}, }, } @@ -398,107 +443,95 @@ func TestSearch_filters(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - reg.Register( - httpmock.GraphQL(`query DiscussionListSearch\b`), - httpmock.StringResponse(`{"data":{"search":{ - "discussionCount": 0, - "pageInfo": {"hasNextPage": false, "endCursor": ""}, - "nodes": [] - }}}`), - ) + for _, resp := range tt.responses { + reg.Register(httpmock.GraphQL(`query DiscussionListSearch\b`), httpmock.StringResponse(resp)) + } c := newTestDiscussionClient(reg) - result, err := c.Search(ghrepo.New("OWNER", "REPO"), tt.filters, "", 10) + result, err := c.Search(repo, tt.filters, tt.after, tt.limit) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) - assert.NotNil(t, result) + require.NotNil(t, result) + assert.Equal(t, tt.wantTotal, result.TotalCount) + assert.Len(t, result.Discussions, tt.wantLen) + assert.Equal(t, tt.wantCursor, result.NextCursor) + + for i, title := range tt.wantTitles { + assert.Equal(t, title, result.Discussions[i].Title) + } + + if tt.wantDisc != nil { + require.NotEmpty(t, result.Discussions) + assert.Equal(t, *tt.wantDisc, result.Discussions[0]) + } }) } } -func TestSearch_pagination(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - makeNode := func(id, title string) string { - return `{ - "id": "` + id + `", "number": 1, "title": "` + title + `", "body": "", - "url": "", "closed": false, "stateReason": "", "isAnswered": false, - "answerChosenAt": "0001-01-01T00:00:00Z", - "author": {"__typename": "User", "login": "alice"}, - "category": {"id": "C1", "name": "General", "slug": "general", "emoji": "", "isAnswerable": false}, - "answerChosenBy": null, "labels": {"nodes": []}, "reactionGroups": [], - "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-01T00:00:00Z", - "closedAt": "0001-01-01T00:00:00Z", "locked": false - }` - } - - reg.Register( - httpmock.GraphQL(`query DiscussionListSearch\b`), - httpmock.StringResponse(`{"data":{"search":{ - "discussionCount": 2, - "pageInfo": {"hasNextPage": true, "endCursor": "searchCursor1"}, - "nodes": [`+makeNode("D1", "First")+`] - }}}`), - ) - reg.Register( - httpmock.GraphQL(`query DiscussionListSearch\b`), - httpmock.StringResponse(`{"data":{"search":{ - "discussionCount": 2, - "pageInfo": {"hasNextPage": false, "endCursor": ""}, - "nodes": [`+makeNode("D2", "Second")+`] - }}}`), - ) - - c := newTestDiscussionClient(reg) - result, err := c.Search(ghrepo.New("OWNER", "REPO"), SearchFilters{}, "", 2) - require.NoError(t, err) - assert.Len(t, result.Discussions, 2) - assert.Equal(t, "First", result.Discussions[0].Title) - assert.Equal(t, "Second", result.Discussions[1].Title) -} - // --------------------------------------------------------------------------- // ListCategories // --------------------------------------------------------------------------- -func TestListCategories_success(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) +func TestListCategories(t *testing.T) { + repo := ghrepo.New("OWNER", "REPO") - reg.Register( - httpmock.GraphQL(`query DiscussionCategoryList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": true, - "discussionCategories": {"nodes": [ - {"id": "C1", "name": "General", "slug": "general", "emoji": ":speech_balloon:", "isAnswerable": false}, - {"id": "C2", "name": "Q&A", "slug": "q-a", "emoji": ":question:", "isAnswerable": true} - ]} - }}}`), - ) + tests := []struct { + name string + response string + wantErr string + wantCats []DiscussionCategory + }{ + { + name: "maps all fields", + response: `{"data":{"repository":{ + "hasDiscussionsEnabled":true, + "discussionCategories":{"nodes":[ + {"id":"C1","name":"General","slug":"general","emoji":":speech_balloon:","isAnswerable":false}, + {"id":"C2","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true} + ]} + }}}`, + wantCats: []DiscussionCategory{ + {ID: "C1", Name: "General", Slug: "general", Emoji: ":speech_balloon:", IsAnswerable: false}, + {ID: "C2", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, + }, + }, + { + name: "discussions disabled", + response: `{"data":{"repository":{ + "hasDiscussionsEnabled":false, + "discussionCategories":{"nodes":[]} + }}}`, + wantErr: "discussions disabled", + }, + } - c := newTestDiscussionClient(reg) - categories, err := c.ListCategories(ghrepo.New("OWNER", "REPO")) - require.NoError(t, err) - require.Len(t, categories, 2) - assert.Equal(t, "General", categories[0].Name) - assert.Equal(t, "Q&A", categories[1].Name) - assert.True(t, categories[1].IsAnswerable) -} - -func TestListCategories_discussionsDisabled(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - reg.Register( - httpmock.GraphQL(`query DiscussionCategoryList\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasDiscussionsEnabled": false, - "discussionCategories": {"nodes": []} - }}}`), - ) - - c := newTestDiscussionClient(reg) - _, err := c.ListCategories(ghrepo.New("OWNER", "REPO")) - require.Error(t, err) - assert.Contains(t, err.Error(), "discussions disabled") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register(httpmock.GraphQL(`query DiscussionCategoryList\b`), httpmock.StringResponse(tt.response)) + + c := newTestDiscussionClient(reg) + categories, err := c.ListCategories(repo) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + + require.NoError(t, err) + require.Len(t, categories, len(tt.wantCats)) + for i, want := range tt.wantCats { + assert.Equal(t, want, categories[i]) + } + }) + } } From 5db230b3171ede930d3033688a929f4778f4de37 Mon Sep 17 00:00:00 2001 From: Max Beizer Date: Wed, 22 Apr 2026 16:36:07 -0500 Subject: [PATCH 3/6] test(discussion): assert GQL variables, add Bot actor and limit>100 cases - Replace false-positive filter tests with GraphQLQuery responders that assert on actual GQL variables (first, after, states, answered, orderBy, categoryId in List; query string qualifiers in Search) - Add Bot actor test case (Bot.ID maps to DiscussionActor.ID, Name is empty) - Add limit>100 test cases for both List and Search to verify the per-iteration first variable is set correctly (100 on page 1, remainder on page 2) - Fix limit>100 bug in client_impl.go: move variables["first"] assignment inside the loop so each iteration caps at min(remaining, 100) - Remove intStr helper; use strconv.Itoa directly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/discussion/client/client_impl.go | 2 + pkg/cmd/discussion/client/client_impl_test.go | 295 +++++++++++++++--- 2 files changed, 246 insertions(+), 51 deletions(-) diff --git a/pkg/cmd/discussion/client/client_impl.go b/pkg/cmd/discussion/client/client_impl.go index ed44e49be..c14d22a77 100644 --- a/pkg/cmd/discussion/client/client_impl.go +++ b/pkg/cmd/discussion/client/client_impl.go @@ -214,6 +214,7 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, afte remaining := limit for { + variables["first"] = githubv4.Int(min(remaining, 100)) if err := c.gql.Query(repo.RepoHost(), "DiscussionList", &query, variables); err != nil { return nil, err } @@ -336,6 +337,7 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, remaining := limit for { + variables["first"] = githubv4.Int(min(remaining, 100)) if err := c.gql.Query(repo.RepoHost(), "DiscussionListSearch", &query, variables); err != nil { return nil, err } diff --git a/pkg/cmd/discussion/client/client_impl_test.go b/pkg/cmd/discussion/client/client_impl_test.go index cb4a89f62..1599a2b5b 100644 --- a/pkg/cmd/discussion/client/client_impl_test.go +++ b/pkg/cmd/discussion/client/client_impl_test.go @@ -2,6 +2,8 @@ package client import ( "net/http" + "strconv" + "strings" "testing" "time" @@ -22,6 +24,15 @@ func minimalNode(id, title string) string { return `{"id":"` + id + `","number":1,"title":"` + title + `","body":"","url":"","closed":false,"stateReason":"","isAnswered":false,"answerChosenAt":"0001-01-01T00:00:00Z","author":{"__typename":"User","login":"alice"},"category":{"id":"C1","name":"General","slug":"general","emoji":"","isAnswerable":false},"answerChosenBy":null,"labels":{"nodes":[]},"reactionGroups":[],"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","closedAt":"0001-01-01T00:00:00Z","locked":false}` } +// minimalNodes returns count comma-separated minimal JSON discussion nodes. +func minimalNodes(count int) string { + nodes := make([]string, count) + for i := range nodes { + nodes[i] = minimalNode("D"+strconv.Itoa(i+1), "Discussion "+strconv.Itoa(i+1)) + } + return strings.Join(nodes, ",") +} + // listResp builds a mock repository.discussions JSON response. func listResp(hasNext bool, cursor string, total int, nodes string) string { hasNextStr := "false" @@ -29,7 +40,7 @@ func listResp(hasNext bool, cursor string, total int, nodes string) string { hasNextStr = "true" } return `{"data":{"repository":{"hasDiscussionsEnabled":true,"discussions":{"totalCount":` + - intStr(total) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}}` + strconv.Itoa(total) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}}` } // searchResp builds a mock search JSON response. @@ -39,30 +50,7 @@ func searchResp(hasNext bool, cursor string, count int, nodes string) string { hasNextStr = "true" } return `{"data":{"search":{"discussionCount":` + - intStr(count) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}` -} - -// intStr converts an int to its decimal string representation without importing strconv. -func intStr(n int) string { - if n == 0 { - return "0" - } - neg := n < 0 - if neg { - n = -n - } - buf := [20]byte{} - pos := len(buf) - for n > 0 { - pos-- - buf[pos] = byte('0' + n%10) - n /= 10 - } - if neg { - pos-- - buf[pos] = '-' - } - return string(buf[pos:]) + strconv.Itoa(count) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}` } // --------------------------------------------------------------------------- @@ -90,17 +78,18 @@ func TestList(t *testing.T) { disabledResp := `{"data":{"repository":{"hasDiscussionsEnabled":false,"discussions":{"totalCount":0,"pageInfo":{"hasNextPage":false,"endCursor":""},"nodes":[]}}}}` tests := []struct { - name string - filters ListFilters - after string - limit int - responses []string - wantErr string - wantTotal int - wantLen int - wantCursor string - wantTitles []string - wantDisc *Discussion + name string + filters ListFilters + after string + limit int + responses []string + checkVarsFns []func(*testing.T, map[string]interface{}) + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantDisc *Discussion }{ { name: "maps all fields", @@ -160,48 +149,143 @@ func TestList(t *testing.T) { limit: 10, after: "someCursor", responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, "someCursor", vars["after"]) + }, + }, }, { name: "open state filter", limit: 10, filters: ListFilters{State: new(FilterStateOpen)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, []interface{}{"OPEN"}, vars["states"]) + }, + }, }, { name: "closed state filter", limit: 10, filters: ListFilters{State: new(FilterStateClosed)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, []interface{}{"CLOSED"}, vars["states"]) + }, + }, }, { name: "answered filter", limit: 10, filters: ListFilters{Answered: new(true)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, true, vars["answered"]) + }, + }, }, { name: "unanswered filter", limit: 10, filters: ListFilters{Answered: new(false)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, false, vars["answered"]) + }, + }, }, { name: "category ID filter", limit: 10, filters: ListFilters{CategoryID: "CAT123"}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, "CAT123", vars["categoryId"]) + }, + }, }, { name: "order by created asc", limit: 10, filters: ListFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + orderBy, ok := vars["orderBy"].(map[string]interface{}) + require.True(t, ok, "orderBy should be a map") + assert.Equal(t, "CREATED_AT", orderBy["field"]) + assert.Equal(t, "ASC", orderBy["direction"]) + }, + }, }, { name: "order by updated desc", limit: 10, filters: ListFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + orderBy, ok := vars["orderBy"].(map[string]interface{}) + require.True(t, ok, "orderBy should be a map") + assert.Equal(t, "UPDATED_AT", orderBy["field"]) + assert.Equal(t, "DESC", orderBy["direction"]) + }, + }, + }, + { + // Bot actors have no name; ID comes from the Bot.ID field. + name: "bot actor", + limit: 10, + responses: []string{listResp(false, "", 1, `{"id":"D_bot","number":1,"title":"Bot post","body":"","url":"","closed":false,"stateReason":"","isAnswered":false,"answerChosenAt":"0001-01-01T00:00:00Z","author":{"__typename":"Bot","login":"gh-bot","id":"bot-node-id"},"category":{"id":"C1","name":"General","slug":"general","emoji":"","isAnswerable":false},"answerChosenBy":null,"labels":{"nodes":[]},"reactionGroups":[],"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","closedAt":"0001-01-01T00:00:00Z","locked":false}`)}, + wantLen: 1, + wantTotal: 1, + wantDisc: &Discussion{ + ID: "D_bot", + Number: 1, + Title: "Bot post", + Author: DiscussionActor{ID: "bot-node-id", Login: "gh-bot", Name: ""}, + Category: DiscussionCategory{ID: "C1", Name: "General", Slug: "general"}, + Labels: []DiscussionLabel{}, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + }, + }, + { + // When limit > 100, the first page requests 100 and the second page + // requests the remainder, exercising the per-iteration first variable. + name: "limit greater than 100", + limit: 101, + responses: []string{ + listResp(true, "pg2cursor", 101, minimalNodes(100)), + listResp(false, "", 101, minimalNode("D101", "Discussion 101")), + }, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, float64(100), vars["first"]) + }, + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, float64(1), vars["first"]) + }, + }, + wantLen: 101, + wantTotal: 101, }, { // When the page has more items than requested, NextCursor is set. @@ -233,8 +317,17 @@ func TestList(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - for _, resp := range tt.responses { - reg.Register(httpmock.GraphQL(`query DiscussionList\b`), httpmock.StringResponse(resp)) + for i, resp := range tt.responses { + var responder httpmock.Responder + if i < len(tt.checkVarsFns) && tt.checkVarsFns[i] != nil { + fn := tt.checkVarsFns[i] + responder = httpmock.GraphQLQuery(resp, func(_ string, vars map[string]interface{}) { + fn(t, vars) + }) + } else { + responder = httpmock.StringResponse(resp) + } + reg.Register(httpmock.GraphQL(`query DiscussionList\b`), responder) } c := newTestDiscussionClient(reg) @@ -288,17 +381,18 @@ func TestSearch(t *testing.T) { emptyResp := searchResp(false, "", 0, "") tests := []struct { - name string - filters SearchFilters - after string - limit int - responses []string - wantErr string - wantTotal int - wantLen int - wantCursor string - wantTitles []string - wantDisc *Discussion + name string + filters SearchFilters + after string + limit int + responses []string + checkVarsFns []func(*testing.T, map[string]interface{}) + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantDisc *Discussion }{ { name: "maps all fields", @@ -352,66 +446,156 @@ func TestSearch(t *testing.T) { limit: 10, after: "someCursor", responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, "someCursor", vars["after"]) + }, + }, }, { name: "open state filter", limit: 10, filters: SearchFilters{State: new(FilterStateOpen)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "is:open") + }, + }, }, { name: "closed state filter", limit: 10, filters: SearchFilters{State: new(FilterStateClosed)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "is:closed") + }, + }, }, { name: "answered filter", limit: 10, filters: SearchFilters{Answered: new(true)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "is:answered") + }, + }, }, { name: "unanswered filter", limit: 10, filters: SearchFilters{Answered: new(false)}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "is:unanswered") + }, + }, }, { name: "author filter", limit: 10, filters: SearchFilters{Author: "alice"}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), `author:"alice"`) + }, + }, }, { name: "labels filter", limit: 10, filters: SearchFilters{Labels: []string{"bug", "enhancement"}}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + q := vars["query"].(string) + assert.Contains(t, q, `label:"bug"`) + assert.Contains(t, q, `label:"enhancement"`) + }, + }, }, { name: "category filter", limit: 10, filters: SearchFilters{Category: "Q&A"}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), `category:"Q&A"`) + }, + }, }, { name: "keywords filter", limit: 10, filters: SearchFilters{Keywords: "some keyword"}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "some keyword") + }, + }, }, { name: "order by created asc", limit: 10, filters: SearchFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "sort:created-asc") + }, + }, }, { name: "order by updated desc", limit: 10, filters: SearchFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, responses: []string{emptyResp}, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Contains(t, vars["query"].(string), "sort:updated-desc") + }, + }, + }, + { + // When limit > 100, the first page requests 100 and the second page + // requests the remainder, exercising the per-iteration first variable. + name: "limit greater than 100", + limit: 101, + responses: []string{ + searchResp(true, "pg2cursor", 101, minimalNodes(100)), + searchResp(false, "", 101, minimalNode("D101", "Discussion 101")), + }, + checkVarsFns: []func(*testing.T, map[string]interface{}){ + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, float64(100), vars["first"]) + }, + func(t *testing.T, vars map[string]interface{}) { + t.Helper() + assert.Equal(t, float64(1), vars["first"]) + }, + }, + wantLen: 101, + wantTotal: 101, }, { // When the page has more items than requested, NextCursor is set. @@ -443,8 +627,17 @@ func TestSearch(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - for _, resp := range tt.responses { - reg.Register(httpmock.GraphQL(`query DiscussionListSearch\b`), httpmock.StringResponse(resp)) + for i, resp := range tt.responses { + var responder httpmock.Responder + if i < len(tt.checkVarsFns) && tt.checkVarsFns[i] != nil { + fn := tt.checkVarsFns[i] + responder = httpmock.GraphQLQuery(resp, func(_ string, vars map[string]interface{}) { + fn(t, vars) + }) + } else { + responder = httpmock.StringResponse(resp) + } + reg.Register(httpmock.GraphQL(`query DiscussionListSearch\b`), responder) } c := newTestDiscussionClient(reg) From 45de7db4d5b607fbe25a5a88f9bd0e62d8875abe Mon Sep 17 00:00:00 2001 From: Max Beizer Date: Wed, 22 Apr 2026 16:49:22 -0500 Subject: [PATCH 4/6] style: run gofmt on client_impl_test.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/discussion/client/client_impl_test.go | 122 +++++++++--------- 1 file changed, 61 insertions(+), 61 deletions(-) diff --git a/pkg/cmd/discussion/client/client_impl_test.go b/pkg/cmd/discussion/client/client_impl_test.go index 1599a2b5b..f06723235 100644 --- a/pkg/cmd/discussion/client/client_impl_test.go +++ b/pkg/cmd/discussion/client/client_impl_test.go @@ -78,18 +78,18 @@ func TestList(t *testing.T) { disabledResp := `{"data":{"repository":{"hasDiscussionsEnabled":false,"discussions":{"totalCount":0,"pageInfo":{"hasNextPage":false,"endCursor":""},"nodes":[]}}}}` tests := []struct { - name string - filters ListFilters - after string - limit int - responses []string - checkVarsFns []func(*testing.T, map[string]interface{}) - wantErr string - wantTotal int - wantLen int - wantCursor string - wantTitles []string - wantDisc *Discussion + name string + filters ListFilters + after string + limit int + responses []string + checkVarsFns []func(*testing.T, map[string]interface{}) + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantDisc *Discussion }{ { name: "maps all fields", @@ -99,20 +99,20 @@ func TestList(t *testing.T) { wantLen: 1, wantDisc: &Discussion{ ID: "D_rich1", Number: 42, Title: "Rich discussion", Body: "body text here", - URL: "https://github.com/OWNER/REPO/discussions/42", - Closed: true, - StateReason: "RESOLVED", - Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, - Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, - Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}, {ID: "L2", Name: "enhancement", Color: "a2eeef"}}, - Answered: true, + URL: "https://github.com/OWNER/REPO/discussions/42", + Closed: true, + StateReason: "RESOLVED", + Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, + Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, + Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}, {ID: "L2", Name: "enhancement", Color: "a2eeef"}}, + Answered: true, AnswerChosenAt: time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC), AnswerChosenBy: &DiscussionActor{ID: "U2", Login: "bob", Name: "Bob"}, - Comments: DiscussionCommentList{}, - CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), - ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), - Locked: true, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), + ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), + Locked: true, }, }, { @@ -248,19 +248,19 @@ func TestList(t *testing.T) { }, { // Bot actors have no name; ID comes from the Bot.ID field. - name: "bot actor", - limit: 10, + name: "bot actor", + limit: 10, responses: []string{listResp(false, "", 1, `{"id":"D_bot","number":1,"title":"Bot post","body":"","url":"","closed":false,"stateReason":"","isAnswered":false,"answerChosenAt":"0001-01-01T00:00:00Z","author":{"__typename":"Bot","login":"gh-bot","id":"bot-node-id"},"category":{"id":"C1","name":"General","slug":"general","emoji":"","isAnswerable":false},"answerChosenBy":null,"labels":{"nodes":[]},"reactionGroups":[],"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","closedAt":"0001-01-01T00:00:00Z","locked":false}`)}, wantLen: 1, wantTotal: 1, wantDisc: &Discussion{ - ID: "D_bot", - Number: 1, - Title: "Bot post", - Author: DiscussionActor{ID: "bot-node-id", Login: "gh-bot", Name: ""}, - Category: DiscussionCategory{ID: "C1", Name: "General", Slug: "general"}, - Labels: []DiscussionLabel{}, - Comments: DiscussionCommentList{}, + ID: "D_bot", + Number: 1, + Title: "Bot post", + Author: DiscussionActor{ID: "bot-node-id", Login: "gh-bot", Name: ""}, + Category: DiscussionCategory{ID: "C1", Name: "General", Slug: "general"}, + Labels: []DiscussionLabel{}, + Comments: DiscussionCommentList{}, CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), UpdatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), }, @@ -381,18 +381,18 @@ func TestSearch(t *testing.T) { emptyResp := searchResp(false, "", 0, "") tests := []struct { - name string - filters SearchFilters - after string - limit int - responses []string - checkVarsFns []func(*testing.T, map[string]interface{}) - wantErr string - wantTotal int - wantLen int - wantCursor string - wantTitles []string - wantDisc *Discussion + name string + filters SearchFilters + after string + limit int + responses []string + checkVarsFns []func(*testing.T, map[string]interface{}) + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantDisc *Discussion }{ { name: "maps all fields", @@ -402,20 +402,20 @@ func TestSearch(t *testing.T) { wantLen: 1, wantDisc: &Discussion{ ID: "D_rich1", Number: 42, Title: "Rich search result", Body: "body text here", - URL: "https://github.com/OWNER/REPO/discussions/42", - Closed: true, - StateReason: "RESOLVED", - Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, - Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, - Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}}, - Answered: true, + URL: "https://github.com/OWNER/REPO/discussions/42", + Closed: true, + StateReason: "RESOLVED", + Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, + Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, + Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}}, + Answered: true, AnswerChosenAt: time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC), AnswerChosenBy: &DiscussionActor{ID: "U2", Login: "bob", Name: "Bob"}, - Comments: DiscussionCommentList{}, - CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), - ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), - Locked: true, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), + ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), + Locked: true, }, }, { @@ -675,10 +675,10 @@ func TestListCategories(t *testing.T) { repo := ghrepo.New("OWNER", "REPO") tests := []struct { - name string - response string - wantErr string - wantCats []DiscussionCategory + name string + response string + wantErr string + wantCats []DiscussionCategory }{ { name: "maps all fields", From 81117364ba32b100758a345935d4d50e17a241f5 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 24 Apr 2026 14:02:43 +0100 Subject: [PATCH 5/6] refactor(discussion/client): convert tests to httpStubs pattern Replace responses/checkVarsFns with httpStubs func per test case in TestList, TestSearch, and TestListCategories, matching the pattern used in agent-task/capi tests. Expand inline JSON to heredoc, remove flower box comments, and add "empty list" and "exact fit" test cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/discussion/client/client_impl_test.go | 1024 +++++++++++------ 1 file changed, 651 insertions(+), 373 deletions(-) diff --git a/pkg/cmd/discussion/client/client_impl_test.go b/pkg/cmd/discussion/client/client_impl_test.go index f06723235..0a6392798 100644 --- a/pkg/cmd/discussion/client/client_impl_test.go +++ b/pkg/cmd/discussion/client/client_impl_test.go @@ -1,12 +1,13 @@ package client import ( + "fmt" "net/http" - "strconv" "strings" "testing" "time" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" @@ -21,36 +22,87 @@ func newTestDiscussionClient(reg *httpmock.Registry) DiscussionClient { // minimalNode returns a minimal JSON discussion node with the given id and title. func minimalNode(id, title string) string { - return `{"id":"` + id + `","number":1,"title":"` + title + `","body":"","url":"","closed":false,"stateReason":"","isAnswered":false,"answerChosenAt":"0001-01-01T00:00:00Z","author":{"__typename":"User","login":"alice"},"category":{"id":"C1","name":"General","slug":"general","emoji":"","isAnswerable":false},"answerChosenBy":null,"labels":{"nodes":[]},"reactionGroups":[],"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","closedAt":"0001-01-01T00:00:00Z","locked":false}` + return heredoc.Docf(` + { + "id": %q, + "number": 1, + "title": %q, + "body": "", + "url": "", + "closed": false, + "stateReason": "", + "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": { + "__typename": "User", + "login": "alice" + }, + "category": { + "id": "C1", + "name": "General", + "slug": "general", + "emoji": "", + "isAnswerable": false + }, + "answerChosenBy": null, + "labels": { + "nodes": [] + }, + "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", + "updatedAt": "2024-01-01T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", + "locked": false + } + `, id, title) } // minimalNodes returns count comma-separated minimal JSON discussion nodes. func minimalNodes(count int) string { nodes := make([]string, count) for i := range nodes { - nodes[i] = minimalNode("D"+strconv.Itoa(i+1), "Discussion "+strconv.Itoa(i+1)) + nodes[i] = minimalNode(fmt.Sprintf("D%d", i+1), fmt.Sprintf("Discussion %d", i+1)) } return strings.Join(nodes, ",") } // listResp builds a mock repository.discussions JSON response. func listResp(hasNext bool, cursor string, total int, nodes string) string { - hasNextStr := "false" - if hasNext { - hasNextStr = "true" - } - return `{"data":{"repository":{"hasDiscussionsEnabled":true,"discussions":{"totalCount":` + - strconv.Itoa(total) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}}` + return heredoc.Docf(` + { + "data": { + "repository": { + "hasDiscussionsEnabled": true, + "discussions": { + "totalCount": %d, + "pageInfo": { + "hasNextPage": %t, + "endCursor": %q + }, + "nodes": [%s] + } + } + } + } + `, total, hasNext, cursor, nodes) } // searchResp builds a mock search JSON response. func searchResp(hasNext bool, cursor string, count int, nodes string) string { - hasNextStr := "false" - if hasNext { - hasNextStr = "true" - } - return `{"data":{"search":{"discussionCount":` + - strconv.Itoa(count) + `,"pageInfo":{"hasNextPage":` + hasNextStr + `,"endCursor":"` + cursor + `"},"nodes":[` + nodes + `]}}}` + return heredoc.Docf(` + { + "data": { + "search": { + "discussionCount": %d, + "pageInfo": { + "hasNextPage": %t, + "endCursor": %q + }, + "nodes": [%s] + } + } + } + `, count, hasNext, cursor, nodes) } // --------------------------------------------------------------------------- @@ -60,66 +112,153 @@ func searchResp(hasNext bool, cursor string, count int, nodes string) string { func TestList(t *testing.T) { repo := ghrepo.New("OWNER", "REPO") - richNode := `{ - "id":"D_rich1","number":42,"title":"Rich discussion","body":"body text here", - "url":"https://github.com/OWNER/REPO/discussions/42", - "closed":true,"stateReason":"RESOLVED","isAnswered":true, - "answerChosenAt":"2024-06-01T12:00:00Z", - "author":{"__typename":"User","login":"alice","id":"U1","name":"Alice"}, - "category":{"id":"C1","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true}, - "answerChosenBy":{"__typename":"User","login":"bob","id":"U2","name":"Bob"}, - "labels":{"nodes":[{"id":"L1","name":"bug","color":"d73a4a"},{"id":"L2","name":"enhancement","color":"a2eeef"}]}, - "reactionGroups":[], - "createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-06-02T00:00:00Z", - "closedAt":"2024-06-01T00:00:00Z","locked":true - }` + richNode := heredoc.Doc(` + { + "id": "D_rich1", + "number": 42, + "title": "Rich discussion", + "body": "body text here", + "url": "https://github.com/OWNER/REPO/discussions/42", + "closed": true, + "stateReason": "RESOLVED", + "isAnswered": true, + "answerChosenAt": "2024-06-01T12:00:00Z", + "author": { + "__typename": "User", + "login": "alice", + "id": "U1", + "name": "Alice" + }, + "category": { + "id": "C1", + "name": "Q&A", + "slug": "q-a", + "emoji": ":question:", + "isAnswerable": true + }, + "answerChosenBy": { + "__typename": "User", + "login": "bob", + "id": "U2", + "name": "Bob" + }, + "labels": { + "nodes": [ + {"id": "L1", "name": "bug", "color": "d73a4a"}, + {"id": "L2", "name": "enhancement", "color": "a2eeef"} + ] + }, + "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", + "updatedAt": "2024-06-02T00:00:00Z", + "closedAt": "2024-06-01T00:00:00Z", + "locked": true + } + `) emptyResp := listResp(false, "", 0, "") - disabledResp := `{"data":{"repository":{"hasDiscussionsEnabled":false,"discussions":{"totalCount":0,"pageInfo":{"hasNextPage":false,"endCursor":""},"nodes":[]}}}}` + disabledResp := heredoc.Doc(` + { + "data": { + "repository": { + "hasDiscussionsEnabled": false, + "discussions": { + "totalCount": 0, + "pageInfo": { + "hasNextPage": false, + "endCursor": "" + }, + "nodes": [] + } + } + } + } + `) tests := []struct { - name string - filters ListFilters - after string - limit int - responses []string - checkVarsFns []func(*testing.T, map[string]interface{}) - wantErr string - wantTotal int - wantLen int - wantCursor string - wantTitles []string - wantDisc *Discussion + name string + filters ListFilters + after string + limit int + httpStubs func(*testing.T, *httpmock.Registry) + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantSingleDisc *Discussion }{ { - name: "maps all fields", - limit: 10, - responses: []string{listResp(false, "", 1, richNode)}, + name: "maps all fields", + limit: 10, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(listResp(false, "", 1, richNode)), + ) + }, wantTotal: 1, wantLen: 1, - wantDisc: &Discussion{ - ID: "D_rich1", Number: 42, Title: "Rich discussion", Body: "body text here", - URL: "https://github.com/OWNER/REPO/discussions/42", - Closed: true, - StateReason: "RESOLVED", - Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, - Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, - Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}, {ID: "L2", Name: "enhancement", Color: "a2eeef"}}, + wantSingleDisc: &Discussion{ + ID: "D_rich1", + Number: 42, + Title: "Rich discussion", + Body: "body text here", + URL: "https://github.com/OWNER/REPO/discussions/42", + Closed: true, + StateReason: "RESOLVED", + Author: DiscussionActor{ + ID: "U1", + Login: "alice", + Name: "Alice", + }, + Category: DiscussionCategory{ + ID: "C1", + Name: "Q&A", + Slug: "q-a", + Emoji: ":question:", + IsAnswerable: true, + }, + Labels: []DiscussionLabel{ + {ID: "L1", Name: "bug", Color: "d73a4a"}, + {ID: "L2", Name: "enhancement", Color: "a2eeef"}, + }, Answered: true, AnswerChosenAt: time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC), - AnswerChosenBy: &DiscussionActor{ID: "U2", Login: "bob", Name: "Bob"}, - Comments: DiscussionCommentList{}, - CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), - ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), - Locked: true, + AnswerChosenBy: &DiscussionActor{ + ID: "U2", + Login: "bob", + Name: "Bob", + }, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), + ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), + Locked: true, }, }, { - name: "discussions disabled", - limit: 10, - responses: []string{disabledResp}, - wantErr: "discussions disabled", + name: "empty list", + limit: 10, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(emptyResp), + ) + }, + wantTotal: 0, + wantLen: 0, + }, + { + name: "discussions disabled", + limit: 10, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(disabledResp), + ) + }, + wantErr: "discussions disabled", }, { name: "limit zero", @@ -145,115 +284,161 @@ func TestList(t *testing.T) { wantErr: "unknown state filter", }, { - name: "with after cursor", - limit: 10, - after: "someCursor", - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, "someCursor", vars["after"]) - }, + name: "with after cursor", + limit: 10, + after: "someCursor", + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, "someCursor", vars["after"]) + }), + ) }, }, { - name: "open state filter", - limit: 10, - filters: ListFilters{State: new(FilterStateOpen)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, []interface{}{"OPEN"}, vars["states"]) - }, + name: "open state filter", + limit: 10, + filters: ListFilters{State: new(FilterStateOpen)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, []interface{}{"OPEN"}, vars["states"]) + }), + ) }, }, { - name: "closed state filter", - limit: 10, - filters: ListFilters{State: new(FilterStateClosed)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, []interface{}{"CLOSED"}, vars["states"]) - }, + name: "closed state filter", + limit: 10, + filters: ListFilters{State: new(FilterStateClosed)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, []interface{}{"CLOSED"}, vars["states"]) + }), + ) }, }, { - name: "answered filter", - limit: 10, - filters: ListFilters{Answered: new(true)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, true, vars["answered"]) - }, + name: "answered filter", + limit: 10, + filters: ListFilters{Answered: new(true)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, true, vars["answered"]) + }), + ) }, }, { - name: "unanswered filter", - limit: 10, - filters: ListFilters{Answered: new(false)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, false, vars["answered"]) - }, + name: "unanswered filter", + limit: 10, + filters: ListFilters{Answered: new(false)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, false, vars["answered"]) + }), + ) }, }, { - name: "category ID filter", - limit: 10, - filters: ListFilters{CategoryID: "CAT123"}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, "CAT123", vars["categoryId"]) - }, + name: "category ID filter", + limit: 10, + filters: ListFilters{CategoryID: "CAT123"}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, "CAT123", vars["categoryId"]) + }), + ) }, }, { - name: "order by created asc", - limit: 10, - filters: ListFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - orderBy, ok := vars["orderBy"].(map[string]interface{}) - require.True(t, ok, "orderBy should be a map") - assert.Equal(t, "CREATED_AT", orderBy["field"]) - assert.Equal(t, "ASC", orderBy["direction"]) - }, + name: "order by created asc", + limit: 10, + filters: ListFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + orderBy, ok := vars["orderBy"].(map[string]interface{}) + require.True(t, ok, "orderBy should be a map") + assert.Equal(t, "CREATED_AT", orderBy["field"]) + assert.Equal(t, "ASC", orderBy["direction"]) + }), + ) }, }, { - name: "order by updated desc", - limit: 10, - filters: ListFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - orderBy, ok := vars["orderBy"].(map[string]interface{}) - require.True(t, ok, "orderBy should be a map") - assert.Equal(t, "UPDATED_AT", orderBy["field"]) - assert.Equal(t, "DESC", orderBy["direction"]) - }, + name: "order by updated desc", + limit: 10, + filters: ListFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + orderBy, ok := vars["orderBy"].(map[string]interface{}) + require.True(t, ok, "orderBy should be a map") + assert.Equal(t, "UPDATED_AT", orderBy["field"]) + assert.Equal(t, "DESC", orderBy["direction"]) + }), + ) }, }, { // Bot actors have no name; ID comes from the Bot.ID field. - name: "bot actor", - limit: 10, - responses: []string{listResp(false, "", 1, `{"id":"D_bot","number":1,"title":"Bot post","body":"","url":"","closed":false,"stateReason":"","isAnswered":false,"answerChosenAt":"0001-01-01T00:00:00Z","author":{"__typename":"Bot","login":"gh-bot","id":"bot-node-id"},"category":{"id":"C1","name":"General","slug":"general","emoji":"","isAnswerable":false},"answerChosenBy":null,"labels":{"nodes":[]},"reactionGroups":[],"createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-01-01T00:00:00Z","closedAt":"0001-01-01T00:00:00Z","locked":false}`)}, + name: "bot actor", + limit: 10, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(listResp(false, "", 1, heredoc.Doc(` + { + "id": "D_bot", + "number": 1, + "title": "Bot post", + "body": "", + "url": "", + "closed": false, + "stateReason": "", + "isAnswered": false, + "answerChosenAt": "0001-01-01T00:00:00Z", + "author": { + "__typename": "Bot", + "login": "gh-bot", + "id": "bot-node-id" + }, + "category": { + "id": "C1", + "name": "General", + "slug": "general", + "emoji": "", + "isAnswerable": false + }, + "answerChosenBy": null, + "labels": { + "nodes": [] + }, + "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", + "updatedAt": "2024-01-01T00:00:00Z", + "closedAt": "0001-01-01T00:00:00Z", + "locked": false + } + `))), + ) + }, wantLen: 1, wantTotal: 1, - wantDisc: &Discussion{ + wantSingleDisc: &Discussion{ ID: "D_bot", Number: 1, Title: "Bot post", @@ -270,19 +455,19 @@ func TestList(t *testing.T) { // requests the remainder, exercising the per-iteration first variable. name: "limit greater than 100", limit: 101, - responses: []string{ - listResp(true, "pg2cursor", 101, minimalNodes(100)), - listResp(false, "", 101, minimalNode("D101", "Discussion 101")), - }, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, float64(100), vars["first"]) - }, - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, float64(1), vars["first"]) - }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(listResp(true, "pg2cursor", 101, minimalNodes(100)), func(_ string, vars map[string]interface{}) { + assert.Equal(t, float64(100), vars["first"]) + }), + ) + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.GraphQLQuery(listResp(false, "", 101, minimalNode("D101", "Discussion 101")), func(_ string, vars map[string]interface{}) { + assert.Equal(t, float64(1), vars["first"]) + }), + ) }, wantLen: 101, wantTotal: 101, @@ -291,8 +476,11 @@ func TestList(t *testing.T) { // When the page has more items than requested, NextCursor is set. name: "pagination sets next cursor", limit: 1, - responses: []string{ - listResp(true, "cursor42", 5, minimalNode("D1", "Discussion 1")), + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(listResp(true, "cursor42", 5, minimalNode("D1", "Discussion 1"))), + ) }, wantLen: 1, wantTotal: 5, @@ -302,14 +490,33 @@ func TestList(t *testing.T) { // Two pages are fetched when limit exceeds the first page's results. name: "pagination fetches multiple pages", limit: 2, - responses: []string{ - listResp(true, "cursor1", 2, minimalNode("D1", "First")), - listResp(false, "", 2, minimalNode("D2", "Second")), + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(listResp(true, "cursor1", 2, minimalNode("D1", "First"))), + ) + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(listResp(false, "", 2, minimalNode("D2", "Second"))), + ) }, wantLen: 2, wantTotal: 2, wantTitles: []string{"First", "Second"}, }, + { + name: "exact fit does not overfetch", + limit: 1, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionList\b`), + httpmock.StringResponse(listResp(false, "", 1, minimalNode("D1", "Only one"))), + ) + }, + wantLen: 1, + wantTotal: 1, + wantTitles: []string{"Only one"}, + }, } for _, tt := range tests { @@ -317,17 +524,8 @@ func TestList(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - for i, resp := range tt.responses { - var responder httpmock.Responder - if i < len(tt.checkVarsFns) && tt.checkVarsFns[i] != nil { - fn := tt.checkVarsFns[i] - responder = httpmock.GraphQLQuery(resp, func(_ string, vars map[string]interface{}) { - fn(t, vars) - }) - } else { - responder = httpmock.StringResponse(resp) - } - reg.Register(httpmock.GraphQL(`query DiscussionList\b`), responder) + if tt.httpStubs != nil { + tt.httpStubs(t, reg) } c := newTestDiscussionClient(reg) @@ -349,73 +547,121 @@ func TestList(t *testing.T) { assert.Equal(t, title, result.Discussions[i].Title) } - if tt.wantDisc != nil { + if tt.wantSingleDisc != nil { require.NotEmpty(t, result.Discussions) - assert.Equal(t, *tt.wantDisc, result.Discussions[0]) + assert.Equal(t, *tt.wantSingleDisc, result.Discussions[0]) } }) } } -// --------------------------------------------------------------------------- -// Search -// --------------------------------------------------------------------------- - func TestSearch(t *testing.T) { repo := ghrepo.New("OWNER", "REPO") - richNode := `{ - "id":"D_rich1","number":42,"title":"Rich search result","body":"body text here", - "url":"https://github.com/OWNER/REPO/discussions/42", - "closed":true,"stateReason":"RESOLVED","isAnswered":true, - "answerChosenAt":"2024-06-01T12:00:00Z", - "author":{"__typename":"User","login":"alice","id":"U1","name":"Alice"}, - "category":{"id":"C1","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true}, - "answerChosenBy":{"__typename":"User","login":"bob","id":"U2","name":"Bob"}, - "labels":{"nodes":[{"id":"L1","name":"bug","color":"d73a4a"}]}, - "reactionGroups":[], - "createdAt":"2024-01-01T00:00:00Z","updatedAt":"2024-06-02T00:00:00Z", - "closedAt":"2024-06-01T00:00:00Z","locked":true - }` + richNode := heredoc.Doc(` + { + "id": "D_rich1", + "number": 42, + "title": "Rich search result", + "body": "body text here", + "url": "https://github.com/OWNER/REPO/discussions/42", + "closed": true, + "stateReason": "RESOLVED", + "isAnswered": true, + "answerChosenAt": "2024-06-01T12:00:00Z", + "author": { + "__typename": "User", + "login": "alice", + "id": "U1", + "name": "Alice" + }, + "category": { + "id": "C1", + "name": "Q&A", + "slug": "q-a", + "emoji": ":question:", + "isAnswerable": true + }, + "answerChosenBy": { + "__typename": "User", + "login": "bob", + "id": "U2", + "name": "Bob" + }, + "labels": { + "nodes": [ + {"id": "L1", "name": "bug", "color": "d73a4a"} + ] + }, + "reactionGroups": [], + "createdAt": "2024-01-01T00:00:00Z", + "updatedAt": "2024-06-02T00:00:00Z", + "closedAt": "2024-06-01T00:00:00Z", + "locked": true + } + `) emptyResp := searchResp(false, "", 0, "") tests := []struct { - name string - filters SearchFilters - after string - limit int - responses []string - checkVarsFns []func(*testing.T, map[string]interface{}) - wantErr string - wantTotal int - wantLen int - wantCursor string - wantTitles []string - wantDisc *Discussion + name string + filters SearchFilters + after string + limit int + httpStubs func(*testing.T, *httpmock.Registry) + wantErr string + wantTotal int + wantLen int + wantCursor string + wantTitles []string + wantSingleDisc *Discussion }{ { - name: "maps all fields", - limit: 10, - responses: []string{searchResp(false, "", 1, richNode)}, + name: "maps all fields", + limit: 10, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(searchResp(false, "", 1, richNode)), + ) + }, wantTotal: 1, wantLen: 1, - wantDisc: &Discussion{ - ID: "D_rich1", Number: 42, Title: "Rich search result", Body: "body text here", - URL: "https://github.com/OWNER/REPO/discussions/42", - Closed: true, - StateReason: "RESOLVED", - Author: DiscussionActor{ID: "U1", Login: "alice", Name: "Alice"}, - Category: DiscussionCategory{ID: "C1", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, - Labels: []DiscussionLabel{{ID: "L1", Name: "bug", Color: "d73a4a"}}, + wantSingleDisc: &Discussion{ + ID: "D_rich1", + Number: 42, + Title: "Rich search result", + Body: "body text here", + URL: "https://github.com/OWNER/REPO/discussions/42", + Closed: true, + StateReason: "RESOLVED", + Author: DiscussionActor{ + ID: "U1", + Login: "alice", + Name: "Alice", + }, + Category: DiscussionCategory{ + ID: "C1", + Name: "Q&A", + Slug: "q-a", + Emoji: ":question:", + IsAnswerable: true, + }, + Labels: []DiscussionLabel{ + {ID: "L1", Name: "bug", Color: "d73a4a"}, + }, Answered: true, AnswerChosenAt: time.Date(2024, 6, 1, 12, 0, 0, 0, time.UTC), - AnswerChosenBy: &DiscussionActor{ID: "U2", Login: "bob", Name: "Bob"}, - Comments: DiscussionCommentList{}, - CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), - ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), - Locked: true, + AnswerChosenBy: &DiscussionActor{ + ID: "U2", + Login: "bob", + Name: "Bob", + }, + Comments: DiscussionCommentList{}, + CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + UpdatedAt: time.Date(2024, 6, 2, 0, 0, 0, 0, time.UTC), + ClosedAt: time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC), + Locked: true, }, }, { @@ -442,137 +688,148 @@ func TestSearch(t *testing.T) { wantErr: "unknown state filter", }, { - name: "with after cursor", - limit: 10, - after: "someCursor", - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, "someCursor", vars["after"]) - }, + name: "with after cursor", + limit: 10, + after: "someCursor", + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Equal(t, "someCursor", vars["after"]) + }), + ) }, }, { - name: "open state filter", - limit: 10, - filters: SearchFilters{State: new(FilterStateOpen)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "is:open") - }, + name: "open state filter", + limit: 10, + filters: SearchFilters{State: new(FilterStateOpen)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "is:open") + }), + ) }, }, { - name: "closed state filter", - limit: 10, - filters: SearchFilters{State: new(FilterStateClosed)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "is:closed") - }, + name: "closed state filter", + limit: 10, + filters: SearchFilters{State: new(FilterStateClosed)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "is:closed") + }), + ) }, }, { - name: "answered filter", - limit: 10, - filters: SearchFilters{Answered: new(true)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "is:answered") - }, + name: "answered filter", + limit: 10, + filters: SearchFilters{Answered: new(true)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "is:answered") + }), + ) }, }, { - name: "unanswered filter", - limit: 10, - filters: SearchFilters{Answered: new(false)}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "is:unanswered") - }, + name: "unanswered filter", + limit: 10, + filters: SearchFilters{Answered: new(false)}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "is:unanswered") + }), + ) }, }, { - name: "author filter", - limit: 10, - filters: SearchFilters{Author: "alice"}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), `author:"alice"`) - }, + name: "author filter", + limit: 10, + filters: SearchFilters{Author: "alice"}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), `author:"alice"`) + }), + ) }, }, { - name: "labels filter", - limit: 10, - filters: SearchFilters{Labels: []string{"bug", "enhancement"}}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - q := vars["query"].(string) - assert.Contains(t, q, `label:"bug"`) - assert.Contains(t, q, `label:"enhancement"`) - }, + name: "labels filter", + limit: 10, + filters: SearchFilters{Labels: []string{"bug", "enhancement"}}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + q := vars["query"].(string) + assert.Contains(t, q, `label:"bug"`) + assert.Contains(t, q, `label:"enhancement"`) + }), + ) }, }, { - name: "category filter", - limit: 10, - filters: SearchFilters{Category: "Q&A"}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), `category:"Q&A"`) - }, + name: "category filter", + limit: 10, + filters: SearchFilters{Category: "Q&A"}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), `category:"Q&A"`) + }), + ) }, }, { - name: "keywords filter", - limit: 10, - filters: SearchFilters{Keywords: "some keyword"}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "some keyword") - }, + name: "keywords filter", + limit: 10, + filters: SearchFilters{Keywords: "some keyword"}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "some keyword") + }), + ) }, }, { - name: "order by created asc", - limit: 10, - filters: SearchFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "sort:created-asc") - }, + name: "order by created asc", + limit: 10, + filters: SearchFilters{OrderBy: OrderByCreated, Direction: OrderDirectionAsc}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "sort:created-asc") + }), + ) }, }, { - name: "order by updated desc", - limit: 10, - filters: SearchFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, - responses: []string{emptyResp}, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Contains(t, vars["query"].(string), "sort:updated-desc") - }, + name: "order by updated desc", + limit: 10, + filters: SearchFilters{OrderBy: OrderByUpdated, Direction: OrderDirectionDesc}, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(emptyResp, func(_ string, vars map[string]interface{}) { + assert.Contains(t, vars["query"].(string), "sort:updated-desc") + }), + ) }, }, { @@ -580,19 +837,19 @@ func TestSearch(t *testing.T) { // requests the remainder, exercising the per-iteration first variable. name: "limit greater than 100", limit: 101, - responses: []string{ - searchResp(true, "pg2cursor", 101, minimalNodes(100)), - searchResp(false, "", 101, minimalNode("D101", "Discussion 101")), - }, - checkVarsFns: []func(*testing.T, map[string]interface{}){ - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, float64(100), vars["first"]) - }, - func(t *testing.T, vars map[string]interface{}) { - t.Helper() - assert.Equal(t, float64(1), vars["first"]) - }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(searchResp(true, "pg2cursor", 101, minimalNodes(100)), func(_ string, vars map[string]interface{}) { + assert.Equal(t, float64(100), vars["first"]) + }), + ) + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.GraphQLQuery(searchResp(false, "", 101, minimalNode("D101", "Discussion 101")), func(_ string, vars map[string]interface{}) { + assert.Equal(t, float64(1), vars["first"]) + }), + ) }, wantLen: 101, wantTotal: 101, @@ -601,8 +858,11 @@ func TestSearch(t *testing.T) { // When the page has more items than requested, NextCursor is set. name: "pagination sets next cursor", limit: 1, - responses: []string{ - searchResp(true, "searchCursor42", 5, minimalNode("D1", "Discussion 1")), + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(searchResp(true, "searchCursor42", 5, minimalNode("D1", "Discussion 1"))), + ) }, wantLen: 1, wantTotal: 5, @@ -612,14 +872,33 @@ func TestSearch(t *testing.T) { // Two pages are fetched when limit exceeds the first page's results. name: "pagination fetches multiple pages", limit: 2, - responses: []string{ - searchResp(true, "searchCursor1", 2, minimalNode("D1", "First")), - searchResp(false, "", 2, minimalNode("D2", "Second")), + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(searchResp(true, "searchCursor1", 2, minimalNode("D1", "First"))), + ) + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(searchResp(false, "", 2, minimalNode("D2", "Second"))), + ) }, wantLen: 2, wantTotal: 2, wantTitles: []string{"First", "Second"}, }, + { + name: "exact fit does not overfetch", + limit: 1, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionListSearch\b`), + httpmock.StringResponse(searchResp(false, "", 1, minimalNode("D1", "Only one"))), + ) + }, + wantLen: 1, + wantTotal: 1, + wantTitles: []string{"Only one"}, + }, } for _, tt := range tests { @@ -627,17 +906,8 @@ func TestSearch(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - for i, resp := range tt.responses { - var responder httpmock.Responder - if i < len(tt.checkVarsFns) && tt.checkVarsFns[i] != nil { - fn := tt.checkVarsFns[i] - responder = httpmock.GraphQLQuery(resp, func(_ string, vars map[string]interface{}) { - fn(t, vars) - }) - } else { - responder = httpmock.StringResponse(resp) - } - reg.Register(httpmock.GraphQL(`query DiscussionListSearch\b`), responder) + if tt.httpStubs != nil { + tt.httpStubs(t, reg) } c := newTestDiscussionClient(reg) @@ -659,36 +929,37 @@ func TestSearch(t *testing.T) { assert.Equal(t, title, result.Discussions[i].Title) } - if tt.wantDisc != nil { + if tt.wantSingleDisc != nil { require.NotEmpty(t, result.Discussions) - assert.Equal(t, *tt.wantDisc, result.Discussions[0]) + assert.Equal(t, *tt.wantSingleDisc, result.Discussions[0]) } }) } } -// --------------------------------------------------------------------------- -// ListCategories -// --------------------------------------------------------------------------- - func TestListCategories(t *testing.T) { repo := ghrepo.New("OWNER", "REPO") tests := []struct { - name string - response string - wantErr string - wantCats []DiscussionCategory + name string + httpStubs func(*testing.T, *httpmock.Registry) + wantErr string + wantCats []DiscussionCategory }{ { name: "maps all fields", - response: `{"data":{"repository":{ - "hasDiscussionsEnabled":true, - "discussionCategories":{"nodes":[ - {"id":"C1","name":"General","slug":"general","emoji":":speech_balloon:","isAnswerable":false}, - {"id":"C2","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true} - ]} - }}}`, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionCategoryList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled":true, + "discussionCategories":{"nodes":[ + {"id":"C1","name":"General","slug":"general","emoji":":speech_balloon:","isAnswerable":false}, + {"id":"C2","name":"Q&A","slug":"q-a","emoji":":question:","isAnswerable":true} + ]} + }}}`), + ) + }, wantCats: []DiscussionCategory{ {ID: "C1", Name: "General", Slug: "general", Emoji: ":speech_balloon:", IsAnswerable: false}, {ID: "C2", Name: "Q&A", Slug: "q-a", Emoji: ":question:", IsAnswerable: true}, @@ -696,10 +967,15 @@ func TestListCategories(t *testing.T) { }, { name: "discussions disabled", - response: `{"data":{"repository":{ - "hasDiscussionsEnabled":false, - "discussionCategories":{"nodes":[]} - }}}`, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query DiscussionCategoryList\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasDiscussionsEnabled":false, + "discussionCategories":{"nodes":[]} + }}}`), + ) + }, wantErr: "discussions disabled", }, } @@ -709,7 +985,9 @@ func TestListCategories(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - reg.Register(httpmock.GraphQL(`query DiscussionCategoryList\b`), httpmock.StringResponse(tt.response)) + if tt.httpStubs != nil { + tt.httpStubs(t, reg) + } c := newTestDiscussionClient(reg) categories, err := c.ListCategories(repo) From 2b794ed99213b7b11ca8bdccb22b604ab19dba3d Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Fri, 24 Apr 2026 14:21:24 +0100 Subject: [PATCH 6/6] refactor(discussion/client): remove redundant "first" variable init The fetch loop already assigns "first" on each iteration, so the initial assignment in the variables map is dead code. Remove it from both List and Search. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/discussion/client/client_impl.go | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/discussion/client/client_impl.go b/pkg/cmd/discussion/client/client_impl.go index c14d22a77..d3f8e817b 100644 --- a/pkg/cmd/discussion/client/client_impl.go +++ b/pkg/cmd/discussion/client/client_impl.go @@ -11,6 +11,9 @@ import ( "github.com/shurcooL/githubv4" ) +// maxPageSize is the maximum number of items per page allowed by the GitHub GraphQL API. +const maxPageSize = 100 + type discussionClient struct { gql *api.Client } @@ -171,15 +174,9 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, afte } } - perPage := limit - if perPage > 100 { - perPage = 100 - } - variables := map[string]interface{}{ "owner": githubv4.String(repo.RepoOwner()), "name": githubv4.String(repo.RepoName()), - "first": githubv4.Int(perPage), "after": (*githubv4.String)(nil), "orderBy": githubv4.DiscussionOrder{Field: orderField, Direction: orderDir}, "categoryId": (*githubv4.ID)(nil), @@ -214,7 +211,7 @@ func (c *discussionClient) List(repo ghrepo.Interface, filters ListFilters, afte remaining := limit for { - variables["first"] = githubv4.Int(min(remaining, 100)) + variables["first"] = githubv4.Int(min(remaining, maxPageSize)) if err := c.gql.Query(repo.RepoHost(), "DiscussionList", &query, variables); err != nil { return nil, err } @@ -319,14 +316,8 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, searchQuery += " " + filters.Keywords } - perPage := limit - if perPage > 100 { - perPage = 100 - } - variables := map[string]interface{}{ "query": githubv4.String(searchQuery), - "first": githubv4.Int(perPage), "after": (*githubv4.String)(nil), } if after != "" { @@ -337,7 +328,7 @@ func (c *discussionClient) Search(repo ghrepo.Interface, filters SearchFilters, remaining := limit for { - variables["first"] = githubv4.Int(min(remaining, 100)) + variables["first"] = githubv4.Int(min(remaining, maxPageSize)) if err := c.gql.Query(repo.RepoHost(), "DiscussionListSearch", &query, variables); err != nil { return nil, err }