Don't create a comment with hide: true (#1661)

Author: marocchino

__tests__/main.test.ts

@@ -119,6 +119,24 @@ describe("run", () => {
     )
   })
 
+  test("fails when deleteOldComment and onlyCreateComment are both true", async () => {
+    mockConfig.deleteOldComment = true
+    mockConfig.onlyCreateComment = true
+    const {core} = await runMain()
+    expect(core.setFailed).toHaveBeenCalledWith(
+      "delete and only_create cannot be both set to true",
+    )
+  })
+
+  test("fails when deleteOldComment and hideOldComment are both true", async () => {
+    mockConfig.deleteOldComment = true
+    mockConfig.hideOldComment = true
+    const {core} = await runMain()
+    expect(core.setFailed).toHaveBeenCalledWith(
+      "delete and hide cannot be both set to true",
+    )
+  })
+
   test("fails when onlyCreateComment and onlyUpdateComment are both true", async () => {
     mockConfig.onlyCreateComment = true
     mockConfig.onlyUpdateComment = true
@@ -137,7 +155,7 @@ describe("run", () => {
     )
   })
 
-  test("deletes previous comment when deleteOldComment is true and previous exists", async () => {
+  test("deletes previous comment when deleteOldComment is true and previous comment exists", async () => {
     mockConfig.deleteOldComment = true
     const previous = {id: "existing-id", body: "old body"}
     const {comment, core} = await runMain(({comment}) => {
@@ -146,19 +164,41 @@ describe("run", () => {
     expect(core.setOutput).toHaveBeenCalledWith("previous_comment_id", "existing-id")
     expect(comment.deleteComment).toHaveBeenCalledWith(expect.anything(), "existing-id")
     expect(comment.createComment).not.toHaveBeenCalled()
+    expect(comment.updateComment).not.toHaveBeenCalled()
   })
 
   test("skips delete when deleteOldComment is true but no previous comment exists", async () => {
     mockConfig.deleteOldComment = true
     const {comment, core} = await runMain()
     expect(core.setOutput).toHaveBeenCalledWith("previous_comment_id", undefined)
     expect(comment.deleteComment).not.toHaveBeenCalled()
+    expect(comment.createComment).not.toHaveBeenCalled()
+    expect(comment.updateComment).not.toHaveBeenCalled()
   })
 
-  test("skips creating comment when onlyUpdateComment is true and no previous exists", async () => {
+  test("Updates previous comment when onlyUpdateComment is true and previous comment exists", async () => {
+    mockConfig.onlyUpdateComment = true
+    const previous = {id: "existing-id", body: "old body"}
+    const {comment, core} = await runMain(({comment}) => {
+      vi.mocked(comment.findPreviousComment).mockResolvedValue(previous as any)
+      vi.mocked(comment.getBodyOf).mockReturnValue("previous body content")
+    })
+    expect(comment.updateComment).toHaveBeenCalledWith(
+      expect.anything(),
+      "existing-id",
+      "test body",
+      "",
+      "previous body content",
+    )
+    expect(comment.createComment).not.toHaveBeenCalled()
+    expect(core.setOutput).toHaveBeenCalledWith("previous_comment_id", "existing-id")
+  })
+
+  test("skips creating comment when onlyUpdateComment is true and no previous comment exists", async () => {
     mockConfig.onlyUpdateComment = true
     const {comment} = await runMain()
     expect(comment.createComment).not.toHaveBeenCalled()
+    expect(comment.updateComment).not.toHaveBeenCalled()
   })
 
   test("creates comment when no previous comment exists", async () => {
@@ -173,7 +213,7 @@ describe("run", () => {
     expect(core.setOutput).toHaveBeenCalledWith("created_comment_id", 456)
   })
 
-  test("skips update when onlyCreateComment is true and previous exists", async () => {
+  test("skips update when onlyCreateComment is true and previous comment exists", async () => {
     mockConfig.onlyCreateComment = true
     const previous = {id: "existing-id", body: "old body"}
     const {comment} = await runMain(({comment}) => {
@@ -197,6 +237,14 @@ describe("run", () => {
     expect(comment.updateComment).not.toHaveBeenCalled()
   })
 
+  test("skips when hideOldComment is true and no previous comment exists", async () => {
+    mockConfig.hideOldComment = true
+    const {comment} = await runMain()
+    expect(comment.minimizeComment).not.toHaveBeenCalled()
+    expect(comment.createComment).not.toHaveBeenCalled()
+    expect(comment.updateComment).not.toHaveBeenCalled()
+  })
+
   test("skips update when skipUnchanged is true and body is unchanged", async () => {
     mockConfig.skipUnchanged = true
     const previous = {id: "existing-id", body: "old body"}

src/main.ts

@@ -50,6 +50,14 @@ async function run(): Promise<undefined> {
       throw new Error("delete and recreate cannot be both set to true")
     }
 
+    if (deleteOldComment && onlyCreateComment) {
+      throw new Error("delete and only_create cannot be both set to true")
+    }
+
+    if (deleteOldComment && hideOldComment) {
+      throw new Error("delete and hide cannot be both set to true")
+    }
+
     if (onlyCreateComment && onlyUpdateComment) {
       throw new Error("only_create and only_update cannot be both set to true")
     }
@@ -63,15 +71,8 @@ async function run(): Promise<undefined> {
 
     core.setOutput("previous_comment_id", previous?.id)
 
-    if (deleteOldComment) {
-      if (previous) {
-        await deleteComment(octokit, previous.id)
-      }
-      return
-    }
-
     if (!previous) {
-      if (onlyUpdateComment) {
+      if (onlyUpdateComment || hideOldComment || deleteOldComment) {
         return
       }
       const created = await createComment(octokit, repo, pullRequestNumber, body, header)
@@ -90,6 +91,11 @@ async function run(): Promise<undefined> {
       return
     }
 
+    if (deleteOldComment) {
+      await deleteComment(octokit, previous.id)
+      return
+    }
+
     if (skipUnchanged && commentsEqual(body, previous.body || "", header)) {
       // don't recreate or update if the message is unchanged
       return