From e85cd96d85c1b2d45b827b43ef44538a6a509b33 Mon Sep 17 00:00:00 2001 From: Bethany Date: Tue, 8 Aug 2023 12:49:05 -0700 Subject: [PATCH] tests and fix bug for retry --- .../__tests__/artifact-http-client.test.ts | 190 +++++++++--------- .../internal/shared/artifact-twirp-client.ts | 52 +++-- 2 files changed, 136 insertions(+), 106 deletions(-) diff --git a/packages/artifact/__tests__/artifact-http-client.test.ts b/packages/artifact/__tests__/artifact-http-client.test.ts index 68495ce2..0477b731 100644 --- a/packages/artifact/__tests__/artifact-http-client.test.ts +++ b/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" - -const mockPost = jest.fn() -jest.mock("@actions/http-client", () => { - return { - HttpClient: jest.fn().mockImplementation(() => { - return { - post: mockPost - } - }) - } -}) +jest.mock("@actions/http-client") 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 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 mockPost = jest.fn(() => { 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.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 mockPost = jest + .fn(() => { 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"}`)} } }) - - /* - mockPost.mockImplementationOnce(() => { - const msg = new http.IncomingMessage(new net.Socket()) - msg.statusCode = 500 + .mockImplementationOnce(() => { + const msgFailed = new http.IncomingMessage(new net.Socket()) + msgFailed.statusCode = 500 + msgFailed.statusMessage = "Internal Server Error" 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", () => { - return jest.fn().mockImplementation(() => { - return { - post: mockPost - } - }) - }) - jest.mock("@actions/http-client", () => { + const mockHttpClient = (HttpClient as unknown as jest.Mock).mockImplementation(() => { return { - HttpClient: jest.fn().mockImplementation(() => { - return { - post: mockPost - } - }) + 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) + }) }) diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index a8e989b0..97b1b5f6 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/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)) - const body = await response.readBody() - return JSON.parse(body) + try { + const response = await this.retryableRequest(() => this.httpClient.post(url, JSON.stringify(data), headers)) + const body = await response.readBody() + return JSON.parse(body) + } catch (error) { + throw new Error(error.message) + } } async retryableRequest( - operation: Promise + operation: () => Promise, ): Promise { 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}`) - break + throw new Error(`Failed to make request after ${this.maxAttempts} attempts: ${errorMessage}`) } 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 { - const client = new ArtifactHttpClient(`@actions/artifact-${type}`) +export function createArtifactTwirpClient( + type: "upload" | "download", + maxAttempts?: number, + baseRetryIntervalMilliseconds?: number, + retryMultiplier?: number +): ArtifactServiceClientJSON { + const client = new ArtifactHttpClient( + `@actions/artifact-${type}`, + maxAttempts, + baseRetryIntervalMilliseconds, + retryMultiplier + ) return new ArtifactServiceClientJSON(client) }