tests and fix bug for retry

packages/artifact/__tests__/artifact-http-client.test.ts

@@ -5,17 +5,7 @@ import * as config from "../src/internal/shared/config"
 import { createArtifactTwirpClient } from "../src/internal/shared/artifact-twirp-client"
 import * as core from "@actions/core"
 
-
+jest.mock("@actions/http-client")
-const mockPost = jest.fn()
-jest.mock("@actions/http-client", () => {
-  return {
-    HttpClient: jest.fn().mockImplementation(() => {
-      return {
-        post: mockPost
-      }
-    })
-  }
-})
 
 describe("artifact-http-client", () => {
   beforeAll(() => {
@@ -29,8 +19,7 @@ describe("artifact-http-client", () => {
   })
 
   beforeEach(() => {
-    mockPost.mockClear();
+    jest.clearAllMocks()
-
   })
 
   it("should successfully create a client", () => {
@@ -39,21 +28,7 @@ describe("artifact-http-client", () => {
   })
 
   it("should make a request", async () => {
-    /*
+    const mockPost = jest.fn(() => {
-    const mockHttpClient = (HttpClient as unknown as jest.Mock).mockImplementation(() => {
-      return {
-        post: () => { 
-          const msg = new http.IncomingMessage(new net.Socket())
-          msg.statusCode = 200
-          return {
-            message: msg,
-            readBody: () => {return Promise.resolve(`{"ok": true, "signedUploadUrl": "http://localhost:8080/lol/upload"}`)}
-          } 
-        }
-      }
-    })
-    */
-    mockPost.mockImplementationOnce(() => {
       const msg = new http.IncomingMessage(new net.Socket())
       msg.statusCode = 200
       return {
@@ -61,6 +36,11 @@ describe("artifact-http-client", () => {
         readBody: () => {return Promise.resolve(`{"ok": true, "signedUploadUrl": "http://localhost:8080/upload"}`)}
       }
     })
+    const mockHttpClient = (HttpClient as unknown as jest.Mock).mockImplementation(() => {
+      return {
+        post: mockPost
+      }
+    })
 
     const client = createArtifactTwirpClient("upload")
     const artifact = await client.CreateArtifact(
@@ -72,6 +52,7 @@ describe("artifact-http-client", () => {
       }
     )
 
+    expect(mockHttpClient).toHaveBeenCalledTimes(1) 
     expect(mockPost).toHaveBeenCalledTimes(1)
     expect(artifact).toBeDefined()
     expect(artifact.ok).toBe(true)
@@ -79,32 +60,8 @@ describe("artifact-http-client", () => {
   })
 
   it("should retry if the request fails", async () => {
-    /*
+    const mockPost = jest
-    const mockPost = jest.fn(() => {
+    .fn(() => {
-      const msg = new http.IncomingMessage(new net.Socket())
-      if (mockPost.mock.calls.length > 1) {
-        msg.statusCode = 200
-        return {
-          message: msg,
-          readBody: () => {return Promise.resolve(`{"ok": true, "signedUploadUrl": "http://localhost:8080/upload"}`)}
-        } 
-      }
-      msg.statusCode = 500
-      return {
-        message: msg,
-        readBody: () => {return Promise.resolve(`{"ok": false}`)}
-      } 
-    })
-    */
-
-    mockPost.mockImplementationOnce(() => {
-      const msgFailed = new http.IncomingMessage(new net.Socket())
-      msgFailed.statusCode = 500
-      return {
-        message: msgFailed,
-        readBody: () => {return Promise.resolve(`{"ok": false}`)}
-      }
-    }).mockImplementationOnce(() => {
       const msgSucceeded = new http.IncomingMessage(new net.Socket())
       msgSucceeded.statusCode = 200
       return {
@@ -112,51 +69,27 @@ describe("artifact-http-client", () => {
         readBody: () => {return Promise.resolve(`{"ok": true, "signedUploadUrl": "http://localhost:8080/upload"}`)}
       }
     })
-
+    .mockImplementationOnce(() => {
-    /*
+      const msgFailed = new http.IncomingMessage(new net.Socket())
-    mockPost.mockImplementationOnce(() => {
+      msgFailed.statusCode = 500
-      const msg = new http.IncomingMessage(new net.Socket())
+      msgFailed.statusMessage = "Internal Server Error"
-      msg.statusCode = 500
       return {
-        message: msg,
+        message: msgFailed,
         readBody: () => {return Promise.resolve(`{"ok": false}`)}
-      } 
-    })
-
-    mockPost.mockImplementation(() => {
-      const msg = new http.IncomingMessage(new net.Socket())
-      msg.statusCode = 200
-      return {
-        message: msg,
-        readBody: () => {return Promise.resolve(`{"ok": true, "signedUploadUrl": "http://localhost:8080/lol/upload"}`)}
       }
     })
-    jest.mock("@actions/http-client", () => {
+    const mockHttpClient = (HttpClient as unknown as jest.Mock).mockImplementation(() => {
-      return jest.fn().mockImplementation(() => {
-        return {
-          post: mockPost
-        }
-      })
-    })
-    jest.mock("@actions/http-client", () => {
       return {
-        HttpClient: jest.fn().mockImplementation(() => {
+        post: mockPost
-          return {
-            post: mockPost
-          }
-        })
       }
     })
-    jest.mock("@actions/http-client", () => {
-      return jest.fn().mockImplementation(() => {
-        return {
-          post: mockPost
-        }
-      })
-    })
-    */
 
-    const client = createArtifactTwirpClient("upload")
+    const client = createArtifactTwirpClient(
+      "upload",
+      5, // retry 5 times
+      1, // wait 1 ms
+      1.5 // backoff factor
+    )
     const artifact = await client.CreateArtifact(
       {
         workflowRunBackendId: "1234", 
@@ -166,10 +99,85 @@ describe("artifact-http-client", () => {
       }
     )
 
+    expect(mockHttpClient).toHaveBeenCalledTimes(1)
     expect(artifact).toBeDefined()
     expect(artifact.ok).toBe(true)
     expect(artifact.signedUploadUrl).toBe("http://localhost:8080/upload")
     expect(mockPost).toHaveBeenCalledTimes(2)
   })
+
+  it("should fail if the request fails 5 times", async () => {
+    const mockPost = jest
+      .fn(() => {
+        const msgFailed = new http.IncomingMessage(new net.Socket())
+        msgFailed.statusCode = 500
+        msgFailed.statusMessage = "Internal Server Error"
+        return {
+          message: msgFailed,
+          readBody: () => {return Promise.resolve(`{"ok": false}`)}
+        }
+      })
+
+      const mockHttpClient = (HttpClient as unknown as jest.Mock).mockImplementation(() => {
+        return {
+          post: mockPost
+        }
+      })
+      const client = createArtifactTwirpClient(
+        "upload",
+        5, // retry 5 times
+        1, // wait 1 ms
+        1.5 // backoff factor
+      )
+    await expect(async () => {
+      await client.CreateArtifact(
+        {
+          workflowRunBackendId: "1234", 
+          workflowJobRunBackendId: "5678", 
+          name: "artifact", 
+          version: 4
+        }
+      )
+    }).rejects.toThrowError("Failed to make request after 5 attempts: Failed request: (500) Internal Server Error")
+    expect(mockHttpClient).toHaveBeenCalledTimes(1)
+    expect(mockPost).toHaveBeenCalledTimes(5)
+  })
+  
+  it("should fail immediately if there is a non-retryable error", async () => {
+    const mockPost = jest
+      .fn(() => {
+        const msgFailed = new http.IncomingMessage(new net.Socket())
+        msgFailed.statusCode = 401
+        msgFailed.statusMessage = "Unauthorized"
+        return {
+          message: msgFailed,
+          readBody: () => {return Promise.resolve(`{"ok": false}`)}
+        }
+      })
+
+      const mockHttpClient = (HttpClient as unknown as jest.Mock).mockImplementation(() => {
+        return {
+          post: mockPost
+        }
+      })
+      const client = createArtifactTwirpClient(
+        "upload",
+        5, // retry 5 times
+        1, // wait 1 ms
+        1.5 // backoff factor
+      )
+    await expect(async () => {
+      await client.CreateArtifact(
+        {
+          workflowRunBackendId: "1234", 
+          workflowJobRunBackendId: "5678", 
+          name: "artifact", 
+          version: 4
+        }
+      )
+    }).rejects.toThrowError("Received non-retryable error: Failed request: (401) Unauthorized")
+    expect(mockHttpClient).toHaveBeenCalledTimes(1)
+    expect(mockPost).toHaveBeenCalledTimes(1)
+  })
 })
 

packages/artifact/src/internal/shared/artifact-twirp-client.ts

@@ -21,9 +21,19 @@ class ArtifactHttpClient implements Rpc {
   private baseRetryIntervalMilliseconds: number = 3000
   private retryMultiplier: number = 1.5
 
-  constructor(userAgent: string) {
+  constructor(userAgent: string, maxAttempts?: number, baseRetryIntervalMilliseconds?: number, retryMultiplier?: number) {
     const token = getRuntimeToken()
     this.baseUrl = getResultsServiceUrl()
+    if (maxAttempts) {
+      this.maxAttempts = maxAttempts
+    }
+    if (baseRetryIntervalMilliseconds) {
+      this.baseRetryIntervalMilliseconds = baseRetryIntervalMilliseconds
+    }
+    if (retryMultiplier) {
+      this.retryMultiplier = retryMultiplier
+    }
+
     this.httpClient = new HttpClient(
       userAgent,
       [new BearerCredentialHandler(token)],
@@ -42,22 +52,27 @@ class ArtifactHttpClient implements Rpc {
     let headers = {
       "Content-Type": contentType,
     }
+    info(`Making request to ${url} with data: ${JSON.stringify(data)}`)
 
-    const response = await this.retryableRequest(this.httpClient.post(url, JSON.stringify(data), headers))
+    try {
-    const body = await response.readBody()
+      const response = await this.retryableRequest(() => this.httpClient.post(url, JSON.stringify(data), headers))
-    return JSON.parse(body)
+      const body = await response.readBody()
+      return JSON.parse(body)
+    } catch (error) {
+      throw new Error(error.message)
+    }
   }
 
   async retryableRequest(
-    operation: Promise<HttpClientResponse>
+    operation: () => Promise<HttpClientResponse>,
   ): Promise<HttpClientResponse> {
     let attempt = 0
+    let errorMessage = ""
     while (attempt < this.maxAttempts) {
       let isRetryable = false
-      let errorMessage = ""
 
       try {
-        const response = await operation
+        const response = await operation()
         const statusCode = response.message.statusCode
 
         if (this.isSuccessStatusCode(statusCode)) {
@@ -72,25 +87,22 @@ class ArtifactHttpClient implements Rpc {
       }
 
       if (!isRetryable) {
-        throw new Error(errorMessage)
+        throw new Error(`Received non-retryable error: ${errorMessage}`)
       }
 
       if (attempt + 1 === this.maxAttempts) {
-        info(`Final attempt failed with error: ${errorMessage}`)
+        throw new Error(`Failed to make request after ${this.maxAttempts} attempts: ${errorMessage}`)
-        break
       }
 
       const retryTimeMilliseconds = this.getExponentialRetryTimeMilliseconds(attempt)
-
       info(
         `Attempt ${attempt + 1} of ${this.maxAttempts} failed with error: ${errorMessage}. Retrying request in ${retryTimeMilliseconds} ms...`
       )
-
       await this.sleep(retryTimeMilliseconds)
       attempt++
     }
 
-    throw new Error(`Failed to make request after ${this.maxAttempts} attempts`)
+    throw new Error(`Request failed`)
   }
 
   isSuccessStatusCode(statusCode?: number): boolean {
@@ -134,7 +146,17 @@ class ArtifactHttpClient implements Rpc {
   }
 }
 
-export function createArtifactTwirpClient(type: "upload" | "download"): ArtifactServiceClientJSON {
+export function createArtifactTwirpClient(
-  const client = new ArtifactHttpClient(`@actions/artifact-${type}`)
+  type: "upload" | "download",
+  maxAttempts?: number,
+  baseRetryIntervalMilliseconds?: number,
+  retryMultiplier?: number
+): ArtifactServiceClientJSON {
+  const client = new ArtifactHttpClient(
+    `@actions/artifact-${type}`, 
+    maxAttempts, 
+    baseRetryIntervalMilliseconds, 
+    retryMultiplier
+  )
   return new ArtifactServiceClientJSON(client)
 }