test(discussion view): add tests for client methods and --replies mode#13299
Merged
Conversation
…--replies mode Add table-driven tests for: Client (client_impl_test.go): - TestGetByNumber: field mapping, discussions disabled - TestGetWithComments: field mapping, forward/backward pagination, reply reversal in newest mode, discussions disabled - TestGetCommentReplies: field mapping, forward/backward pagination, reply reversal, discussions disabled, nil node, wrong node type Command (view_test.go): - TestNewCmdView_repliesFlags: mutual exclusivity with --comments/--web, --order/--limit/--after require --comments or --replies, pagination flags work with --replies - TestViewRun_replies: TTY/non-TTY/JSON output, pagination hints, routing assertion (GetCommentReplies called, not GetByNumber or GetWithComments) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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. |
Inline mock JSON responses, use non-default values for all fields to verify mapping, add repo-not-found test case, and match real API behaviour for discussions-disabled response (null discussion with NOT_FOUND error). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Inline all JSON response helpers (getWithCommentsResp, wrapCommentsBlock, commentNode) to avoid cross-test coupling. Add missing test cases for empty comments, first page newest reversal, multiple replies on a single comment, and repo not found. Populate the "maps comments with replies" case with non-zero field values and use a single assert.Equal for the full Discussion struct. Match real API error responses for discussions disabled and repo not found cases. Rename wantDisc to assertDisc across all client test functions and require it for non-error cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inline all JSON response helpers (getCommentRepliesResp, wrapRepliesBlock, bareCommentNode, replyNode) to avoid cross-test coupling. Fix JSON response shape to place "node" as sibling of "repository" under "data", matching the real GraphQL query structure. Populate "maps all fields" with non-zero values and use a single assert.Equal for the full Discussion struct. Match real API error responses: discussions disabled returns NOT_FOUND on discussion, node not found returns NOT_FOUND with null node, wrong-type node returns empty object. Add missing test cases for repo not found and first page newest reversal. Move shared wantComments/wantTotal/wantCursor/wantNext/wantDirection fields into assertDisc callbacks for both TestGetWithComments and TestGetCommentReplies, giving each case full ownership of its assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace scattered standalone test functions with a single table-driven TestNewCmdView covering all arg/flag parsing: number, hash, URL, web, comments, replies, limit, after, order, mutual exclusivity, and invalid inputs. Uses shlex.Split for arg parsing and asserts opts fields individually per case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
Thanks for the PR, @maxbeizer! 🙏 I went through it and made some changes. I think this is now a good example to be referenced for the upcoming commands. Have a look and let me know WDYT. If you're okay with it I'll merge it and we're done with ChangesDetails
This branch adds test coverage for Client tests (
View command tests (
|
Contributor
Author
|
Changes look good to me — go ahead and merge when ready. The consolidated table-driven tests and helpers will be a great template for Phase 2. 👍 |
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 table-driven tests for the
discussion viewcommand, covering both client-level and command-level behavior. This is the follow-up test work discussed after merging #13295.Client Tests (
client_impl_test.go)TestGetByNumber: field mapping, discussions-disabled errorTestGetWithComments: field mapping with nested replies, forward/backward pagination, reply reversal in newest mode, cursor/direction tracking, discussions-disabled errorTestGetCommentReplies: full field mapping, forward/backward pagination, reply reversal in newest mode, cursor/direction tracking, discussions-disabled error, nil node guard, wrong-node-type guardCommand Tests (
view_test.go)TestNewCmdView_repliesFlags(table-driven): mutual exclusivity (--replieswith--comments/--web),--order/--limit/--afterrequire--commentsor--replies, pagination flags work with--repliesTestViewRun_replies(table-driven): TTY rendering, non-TTY raw output, JSON export, pagination hint presence/absence, routing assertion (verifiesGetCommentRepliesis called andGetByNumber/GetWithCommentsare not)All new tests are table-driven per reviewer request.
Test Counts
cc @babakks