-
Notifications
You must be signed in to change notification settings - Fork 378
[jsweep] Clean add_reaction_and_edit_comment.cjs #30756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,8 @@ global.context = mockContext; | |
|
|
||
| // Helper to import the module fresh (bust module cache) | ||
| async function loadModule() { | ||
| const { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction } = await import("./add_reaction_and_edit_comment.cjs?" + Date.now()); | ||
| return { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction }; | ||
| const { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction, resolveEventEndpoints, VALID_REACTIONS } = await import("./add_reaction_and_edit_comment.cjs?" + Date.now()); | ||
| return { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction, resolveEventEndpoints, VALID_REACTIONS }; | ||
| } | ||
|
|
||
| describe("add_reaction_and_edit_comment.cjs", () => { | ||
|
|
@@ -609,4 +609,88 @@ describe("add_reaction_and_edit_comment.cjs", () => { | |
| expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", ""); | ||
| }); | ||
| }); | ||
|
|
||
| describe("VALID_REACTIONS", () => { | ||
| it("should export the list of valid reaction types", async () => { | ||
| const { VALID_REACTIONS } = await loadModule(); | ||
| expect(VALID_REACTIONS).toEqual(["+1", "-1", "laugh", "confused", "heart", "hooray", "rocket", "eyes"]); | ||
| }); | ||
| }); | ||
|
|
||
| describe("resolveEventEndpoints()", () => { | ||
| it("should resolve endpoints for issues event", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const payload = { issue: { number: 42 } }; | ||
| const result = await resolveEventEndpoints("issues", "owner", "repo", payload); | ||
| expect(result).toEqual({ | ||
| reactionEndpoint: "/repos/owner/repo/issues/42/reactions", | ||
| commentUpdateEndpoint: "/repos/owner/repo/issues/42/comments", | ||
| }); | ||
| }); | ||
|
|
||
| it("should return null and call setFailed when issue number is missing", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const result = await resolveEventEndpoints("issues", "owner", "repo", {}); | ||
| expect(result).toBeNull(); | ||
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining(ERR_NOT_FOUND)); | ||
| }); | ||
|
|
||
| it("should resolve endpoints for pull_request event", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const payload = { pull_request: { number: 7 } }; | ||
| const result = await resolveEventEndpoints("pull_request", "owner", "repo", payload); | ||
| expect(result).toEqual({ | ||
| reactionEndpoint: "/repos/owner/repo/issues/7/reactions", | ||
| commentUpdateEndpoint: "/repos/owner/repo/issues/7/comments", | ||
| }); | ||
| }); | ||
|
|
||
| it("should resolve endpoints for issue_comment event", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const payload = { comment: { id: 55 }, issue: { number: 10 } }; | ||
| const result = await resolveEventEndpoints("issue_comment", "owner", "repo", payload); | ||
| expect(result).toEqual({ | ||
| reactionEndpoint: "/repos/owner/repo/issues/comments/55/reactions", | ||
| commentUpdateEndpoint: "/repos/owner/repo/issues/10/comments", | ||
| }); | ||
| }); | ||
|
|
||
| it("should resolve endpoints for pull_request_review_comment event", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const payload = { comment: { id: 99 }, pull_request: { number: 3 } }; | ||
| const result = await resolveEventEndpoints("pull_request_review_comment", "owner", "repo", payload); | ||
| expect(result).toEqual({ | ||
| reactionEndpoint: "/repos/owner/repo/pulls/comments/99/reactions", | ||
| commentUpdateEndpoint: "/repos/owner/repo/issues/3/comments", | ||
| }); | ||
| }); | ||
|
|
||
| it("should resolve endpoints for discussion event using GraphQL node ID", async () => { | ||
| mockGithub.graphql.mockResolvedValueOnce({ repository: { discussion: { id: "D_node123", url: "https://github.com/testowner/testrepo/discussions/5" } } }); | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const payload = { discussion: { number: 5 } }; | ||
| const result = await resolveEventEndpoints("discussion", "owner", "repo", payload); | ||
| expect(result).toEqual({ | ||
| reactionEndpoint: "D_node123", | ||
| commentUpdateEndpoint: "discussion:5", | ||
| }); | ||
| }); | ||
|
|
||
| it("should resolve endpoints for discussion_comment event", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const payload = { discussion: { number: 5 }, comment: { id: 88, node_id: "DC_node88" } }; | ||
| const result = await resolveEventEndpoints("discussion_comment", "owner", "repo", payload); | ||
| expect(result).toEqual({ | ||
| reactionEndpoint: "DC_node88", | ||
| commentUpdateEndpoint: "discussion_comment:5:88", | ||
| }); | ||
| }); | ||
|
|
||
| it("should return null and call setFailed for unknown event type", async () => { | ||
| const { resolveEventEndpoints } = await loadModule(); | ||
| const result = await resolveEventEndpoints("push", "owner", "repo", {}); | ||
| expect(result).toBeNull(); | ||
| expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining(ERR_VALIDATION)); | ||
| }); | ||
| }); | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out] The new
Since the stated goal of extracting this function is to make the logic independently testable, covering these failure paths would complete the picture: it('returns null and calls setFailed when issue_comment is missing commentId', async () => {
const { resolveEventEndpoints } = await loadModule();
const result = await resolveEventEndpoints('issue_comment', 'owner', 'repo', { issue: { number: 1 } });
expect(result).toBeNull();
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining(ERR_VALIDATION));
}); |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/zoom-out]
resolveEventEndpointscallscore.setFaileddirectly, coupling a pure endpoint-resolver to the GitHub Actions runtime. This makes the function harder to reuse or test without mockingcore. Consider letting it throw a typed error and keeping thecore.setFailedcall inmain():Minor point given the existing pattern in the file — feel free to defer.