test(discussion): add httpmock unit tests for DiscussionClient#13252
Merged
babakks merged 6 commits intoApr 24, 2026
Merged
Conversation
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>
github-actions
Bot
added
external
|
Thanks for your pull request! Unfortunately, it doesn't meet the minimum requirements for review:
Please update your PR to address the above. Requirements:
This PR will be automatically closed in 7 days if these requirements are not met. |
babakks
reviewed
Apr 21, 2026
babakks
left a comment
babakks
left a comment
Member
There was a problem hiding this comment.
I know it's still in draft, but I did a quick review.
Addresses babakks' review on PR cli#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>
maxbeizer
commented
Apr 22, 2026
maxbeizer
left a comment
maxbeizer
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for the thorough review @babakks! Addressed all 7 points in the latest commit:
- Converted all 18 individual test functions to 3 table-driven test functions (TestList, TestSearch, TestListCategories)
- Removed the
ptrgeneric helper; replaced all uses withnew(value)syntax - Added full-field assertions for all Discussion struct fields in success cases
- Added after-cursor test cases to cover pagination continuation
- Folded pagination tests into the table structure
All tests pass.
…ases - 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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>
Member
|
Thanks for applying the changes, @maxbeizer! 🙏 I made some changes mostly refactoring the tests. The main is converting all three test functions ( I'll merge this once CI finished. |
Member
|
Merging as the CI fail is already fixed in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds comprehensive httpmock-based unit tests for
pkg/cmd/discussion/client/as requested in PR #13084 review.Tests added
ListNextCursorwhen more pages remain; does not setNextCursorwhen fully fetchedSearchListCategoriesTechnical note
The
shurcooL-graphqldecoder expects inline-fragment fields (graphql:"... on Type") to be flattened in the JSON response at the same level as sibling fields — not nested under a"... on Type"key. Tests use this correct format.