From 85c9bcb392ec0f971278c0cf57241a57f6138d83 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 5 Jun 2020 03:29:23 +0000 Subject: [PATCH 1/9] chore: rename maxRetries to maxAttempts --- .../middleware-retry/src/retryMiddleware.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/middleware-retry/src/retryMiddleware.ts b/packages/middleware-retry/src/retryMiddleware.ts index 2bc454e80506..bbd72ec957f4 100644 --- a/packages/middleware-retry/src/retryMiddleware.ts +++ b/packages/middleware-retry/src/retryMiddleware.ts @@ -9,15 +9,14 @@ import { } from "@aws-sdk/types"; import { RetryResolvedConfig } from "./configurations"; -export function retryMiddleware(options: RetryResolvedConfig) { - return ( - next: FinalizeHandler - ): FinalizeHandler => async ( - args: FinalizeHandlerArguments - ): Promise> => { - return options.retryStrategy.retry(next, args); - }; -} +export const retryMiddleware = (options: RetryResolvedConfig) => < + Output extends MetadataBearer = MetadataBearer +>( + next: FinalizeHandler +): FinalizeHandler => async ( + args: FinalizeHandlerArguments +): Promise> => + options.retryStrategy.retry(next, args); export const retryMiddlewareOptions: FinalizeRequestHandlerOptions & AbsoluteLocation = { From 6a0851e6be05c840890c2211229a0de9385f42f8 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 5 Jun 2020 04:54:37 +0000 Subject: [PATCH 2/9] test: retryMiddleware --- .../src/retryMiddleware.spec.ts | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 packages/middleware-retry/src/retryMiddleware.spec.ts diff --git a/packages/middleware-retry/src/retryMiddleware.spec.ts b/packages/middleware-retry/src/retryMiddleware.spec.ts new file mode 100644 index 000000000000..4472e02b3d03 --- /dev/null +++ b/packages/middleware-retry/src/retryMiddleware.spec.ts @@ -0,0 +1,66 @@ +import { + getRetryPlugin, + retryMiddleware, + retryMiddlewareOptions +} from "./retryMiddleware"; +import { + MiddlewareStack, + RetryStrategy, + FinalizeHandlerArguments +} from "@aws-sdk/types"; + +describe("getRetryPlugin", () => { + const mockClientStack = { + add: jest.fn() + }; + + afterEach(() => { + jest.clearAllMocks(); + }); + + it("adds retryMiddleware if maxRetries > 0", () => { + const maxAttempts = 1; + getRetryPlugin({ + maxAttempts, + retryStrategy: {} as RetryStrategy + }).applyToStack((mockClientStack as unknown) as MiddlewareStack); + expect(mockClientStack.add).toHaveBeenCalledTimes(1); + expect(mockClientStack.add.mock.calls[0][1]).toEqual( + retryMiddlewareOptions + ); + }); + + it("skips adding retryMiddleware if maxRetries = 0", () => { + const maxAttempts = 0; + getRetryPlugin({ + maxAttempts, + retryStrategy: {} as RetryStrategy + }).applyToStack((mockClientStack as unknown) as MiddlewareStack); + expect(mockClientStack.add).toHaveBeenCalledTimes(0); + }); +}); + +describe("retryMiddleware", () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it("calls retryStrategy.retry with next and args", async () => { + const maxAttempts = 1; + const next = jest.fn(); + const args = { + request: {} + }; + const mockRetryStrategy = { + maxAttempts, + retry: jest.fn() + }; + + await retryMiddleware({ + maxAttempts, + retryStrategy: mockRetryStrategy + })(next)(args as FinalizeHandlerArguments); + expect(mockRetryStrategy.retry).toHaveBeenCalledTimes(1); + expect(mockRetryStrategy.retry).toHaveBeenCalledWith(next, args); + }); +}); From f9405c923260f302d73e61828a4e7ee5d2ec1c1a Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 5 Jun 2020 15:30:50 +0000 Subject: [PATCH 3/9] test: use toHaveBeenCalledTimes in retryDecider.spec.ts --- .../middleware-retry/src/retryDecider.spec.ts | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/middleware-retry/src/retryDecider.spec.ts b/packages/middleware-retry/src/retryDecider.spec.ts index 15ed0732cfee..271f22baba0b 100644 --- a/packages/middleware-retry/src/retryDecider.spec.ts +++ b/packages/middleware-retry/src/retryDecider.spec.ts @@ -24,53 +24,53 @@ describe("defaultRetryDecider", () => { it("should return false when the provided error is falsy", () => { expect(defaultRetryDecider(null as any)).toBe(false); - expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(0); - expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(0); - expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(0); - expect((isTransientError as jest.Mock).mock.calls.length).toBe(0); + expect(isRetryableByTrait as jest.Mock).toHaveBeenCalledTimes(0); + expect(isClockSkewError as jest.Mock).toHaveBeenCalledTimes(0); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(0); + expect(isTransientError as jest.Mock).toHaveBeenCalledTimes(0); }); it("should return true for RetryableByTrait error", () => { (isRetryableByTrait as jest.Mock).mockReturnValueOnce(true); expect(defaultRetryDecider(createMockError())).toBe(true); - expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1); - expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(0); - expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(0); - expect((isTransientError as jest.Mock).mock.calls.length).toBe(0); + expect(isRetryableByTrait as jest.Mock).toHaveBeenCalledTimes(1); + expect(isClockSkewError as jest.Mock).toHaveBeenCalledTimes(0); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(0); + expect(isTransientError as jest.Mock).toHaveBeenCalledTimes(0); }); it("should return true for ClockSkewError", () => { (isClockSkewError as jest.Mock).mockReturnValueOnce(true); expect(defaultRetryDecider(createMockError())).toBe(true); - expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1); - expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1); - expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(0); - expect((isTransientError as jest.Mock).mock.calls.length).toBe(0); + expect(isRetryableByTrait as jest.Mock).toHaveBeenCalledTimes(1); + expect(isClockSkewError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(0); + expect(isTransientError as jest.Mock).toHaveBeenCalledTimes(0); }); it("should return true for ThrottlingError", () => { (isThrottlingError as jest.Mock).mockReturnValueOnce(true); expect(defaultRetryDecider(createMockError())).toBe(true); - expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1); - expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1); - expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(1); - expect((isTransientError as jest.Mock).mock.calls.length).toBe(0); + expect(isRetryableByTrait as jest.Mock).toHaveBeenCalledTimes(1); + expect(isClockSkewError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isTransientError as jest.Mock).toHaveBeenCalledTimes(0); }); it("should return true for TransientError", () => { (isTransientError as jest.Mock).mockReturnValueOnce(true); expect(defaultRetryDecider(createMockError())).toBe(true); - expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1); - expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1); - expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(1); - expect((isTransientError as jest.Mock).mock.calls.length).toBe(1); + expect(isRetryableByTrait as jest.Mock).toHaveBeenCalledTimes(1); + expect(isClockSkewError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isTransientError as jest.Mock).toHaveBeenCalledTimes(1); }); it("should return false for other errors", () => { expect(defaultRetryDecider(createMockError())).toBe(false); - expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1); - expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1); - expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(1); - expect((isTransientError as jest.Mock).mock.calls.length).toBe(1); + expect(isRetryableByTrait as jest.Mock).toHaveBeenCalledTimes(1); + expect(isClockSkewError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isTransientError as jest.Mock).toHaveBeenCalledTimes(1); }); }); From ff18dcc877c03504d87c8735fa8563ee9eb57ca6 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 5 Jun 2020 16:05:46 +0000 Subject: [PATCH 4/9] chore: rename ExponentialBackOffStrategy to StandardRetryStrategy --- packages/middleware-retry/src/configurations.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/middleware-retry/src/configurations.ts b/packages/middleware-retry/src/configurations.ts index 8a5eb0178551..a52a18b83420 100644 --- a/packages/middleware-retry/src/configurations.ts +++ b/packages/middleware-retry/src/configurations.ts @@ -17,13 +17,13 @@ export interface RetryResolvedConfig { retryStrategy: RetryStrategy; } -export function resolveRetryConfig( +export const resolveRetryConfig = ( input: T & RetryInputConfig -): T & RetryResolvedConfig { - const maxAttempts = input.maxAttempts === undefined ? 3 : input.maxAttempts; +): T & RetryResolvedConfig => { + const maxAttempts = input.maxAttempts ?? 3; return { ...input, maxAttempts, retryStrategy: input.retryStrategy || new StandardRetryStrategy(maxAttempts) }; -} +}; From 3978376bd38f6dfcc0635451b4e1d72293e4d328 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 5 Jun 2020 18:05:55 +0000 Subject: [PATCH 5/9] test: wip configurations.spec.ts --- .../src/configurations.spec.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 packages/middleware-retry/src/configurations.spec.ts diff --git a/packages/middleware-retry/src/configurations.spec.ts b/packages/middleware-retry/src/configurations.spec.ts new file mode 100644 index 000000000000..89db172b3566 --- /dev/null +++ b/packages/middleware-retry/src/configurations.spec.ts @@ -0,0 +1,25 @@ +import { resolveRetryConfig } from "./configurations"; + +describe("resolveRetryConfig", () => { + describe("maxAttempts", () => { + it("uses passed maxAttempts value if present", () => { + [1, 2, 3].forEach(maxAttempts => { + expect(resolveRetryConfig({ maxAttempts }).maxAttempts).toEqual( + maxAttempts + ); + }); + }); + + it("assigns default value of 3 if maxAttempts not passed", () => { + expect(resolveRetryConfig({}).maxAttempts).toEqual(3); + }); + }); + + describe("retryStrategy", () => { + it("uses passed retryStrategy if present", () => {}); + describe("creates StandardRetryStrategy if retryStrategy not present", () => { + it("uses maxAttempts if present", () => {}); + it("uses default 3 if maxAttempts is not present", () => {}); + }); + }); +}); From 74401fef13a9ef6da080784d924564f69e047ea8 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 9 Jun 2020 01:20:06 +0000 Subject: [PATCH 6/9] test: fix tests for maxAttempts --- .../src/retryMiddleware.spec.ts | 46 +++++++++++-------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/packages/middleware-retry/src/retryMiddleware.spec.ts b/packages/middleware-retry/src/retryMiddleware.spec.ts index 4472e02b3d03..08a49b3708e5 100644 --- a/packages/middleware-retry/src/retryMiddleware.spec.ts +++ b/packages/middleware-retry/src/retryMiddleware.spec.ts @@ -18,25 +18,35 @@ describe("getRetryPlugin", () => { jest.clearAllMocks(); }); - it("adds retryMiddleware if maxRetries > 0", () => { - const maxAttempts = 1; - getRetryPlugin({ - maxAttempts, - retryStrategy: {} as RetryStrategy - }).applyToStack((mockClientStack as unknown) as MiddlewareStack); - expect(mockClientStack.add).toHaveBeenCalledTimes(1); - expect(mockClientStack.add.mock.calls[0][1]).toEqual( - retryMiddlewareOptions - ); + describe("adds retryMiddleware if maxAttempts > 1", () => { + [2, 3, 4].forEach(maxAttempts => { + it(`when maxAttempts=${maxAttempts}`, () => { + getRetryPlugin({ + maxAttempts, + retryStrategy: {} as RetryStrategy + }).applyToStack( + (mockClientStack as unknown) as MiddlewareStack + ); + expect(mockClientStack.add).toHaveBeenCalledTimes(1); + expect(mockClientStack.add.mock.calls[0][1]).toEqual( + retryMiddlewareOptions + ); + }); + }); }); - it("skips adding retryMiddleware if maxRetries = 0", () => { - const maxAttempts = 0; - getRetryPlugin({ - maxAttempts, - retryStrategy: {} as RetryStrategy - }).applyToStack((mockClientStack as unknown) as MiddlewareStack); - expect(mockClientStack.add).toHaveBeenCalledTimes(0); + describe("skips adding retryMiddleware if maxAttempts <= 1", () => { + [0, 1].forEach(maxAttempts => { + it(`when maxAttempts=${maxAttempts}`, () => { + getRetryPlugin({ + maxAttempts, + retryStrategy: {} as RetryStrategy + }).applyToStack( + (mockClientStack as unknown) as MiddlewareStack + ); + expect(mockClientStack.add).toHaveBeenCalledTimes(0); + }); + }); }); }); @@ -46,7 +56,7 @@ describe("retryMiddleware", () => { }); it("calls retryStrategy.retry with next and args", async () => { - const maxAttempts = 1; + const maxAttempts = 2; const next = jest.fn(); const args = { request: {} From a283d539593ddb581aca25f3855dc6418615da10 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 9 Jun 2020 01:36:51 +0000 Subject: [PATCH 7/9] test: complete tests for configurations.ts --- .../src/configurations.spec.ts | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/middleware-retry/src/configurations.spec.ts b/packages/middleware-retry/src/configurations.spec.ts index 89db172b3566..2559251b6483 100644 --- a/packages/middleware-retry/src/configurations.spec.ts +++ b/packages/middleware-retry/src/configurations.spec.ts @@ -1,4 +1,5 @@ import { resolveRetryConfig } from "./configurations"; +import { StandardRetryStrategy } from "./defaultStrategy"; describe("resolveRetryConfig", () => { describe("maxAttempts", () => { @@ -16,10 +17,31 @@ describe("resolveRetryConfig", () => { }); describe("retryStrategy", () => { - it("uses passed retryStrategy if present", () => {}); + it("uses passed retryStrategy if present", () => { + const mockRetryStrategy = { + maxAttempts: 2, + retry: jest.fn() + }; + const { retryStrategy } = resolveRetryConfig({ + retryStrategy: mockRetryStrategy + }); + expect(retryStrategy).toEqual(mockRetryStrategy); + }); + describe("creates StandardRetryStrategy if retryStrategy not present", () => { - it("uses maxAttempts if present", () => {}); - it("uses default 3 if maxAttempts is not present", () => {}); + describe("uses maxAttempts if present", () => { + [1, 2, 3].forEach(maxAttempts => { + const { retryStrategy } = resolveRetryConfig({ maxAttempts }); + expect(retryStrategy).toBeInstanceOf(StandardRetryStrategy); + expect(retryStrategy.maxAttempts).toBe(maxAttempts); + }); + }); + + it("uses default 3 if maxAttempts is not present", () => { + const { retryStrategy } = resolveRetryConfig({}); + expect(retryStrategy).toBeInstanceOf(StandardRetryStrategy); + expect(retryStrategy.maxAttempts).toBe(3); + }); }); }); }); From 390d8b5d6c0bb897f719fa88858879bec2056a97 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 9 Jun 2020 04:00:26 +0000 Subject: [PATCH 8/9] test: defaultStrategy --- .../src/defaultStrategy.spec.ts | 201 ++++++++++++++++++ packages/middleware-retry/src/index.spec.ts | 92 -------- 2 files changed, 201 insertions(+), 92 deletions(-) create mode 100644 packages/middleware-retry/src/defaultStrategy.spec.ts delete mode 100644 packages/middleware-retry/src/index.spec.ts diff --git a/packages/middleware-retry/src/defaultStrategy.spec.ts b/packages/middleware-retry/src/defaultStrategy.spec.ts new file mode 100644 index 000000000000..626c8177f5d3 --- /dev/null +++ b/packages/middleware-retry/src/defaultStrategy.spec.ts @@ -0,0 +1,201 @@ +import { + DEFAULT_RETRY_DELAY_BASE, + THROTTLING_RETRY_DELAY_BASE +} from "./constants"; +import { isThrottlingError } from "@aws-sdk/service-error-classification"; +import { defaultDelayDecider } from "./delayDecider"; +import { defaultRetryDecider } from "./retryDecider"; +import { StandardRetryStrategy } from "./defaultStrategy"; + +jest.mock("@aws-sdk/service-error-classification", () => ({ + isThrottlingError: jest.fn().mockReturnValue(true) +})); + +jest.mock("./delayDecider", () => ({ + defaultDelayDecider: jest.fn().mockReturnValue(0) +})); + +jest.mock("./retryDecider", () => ({ + defaultRetryDecider: jest.fn().mockReturnValue(true) +})); + +describe("defaultStrategy", () => { + const maxAttempts = 2; + + afterEach(() => { + jest.clearAllMocks(); + }); + + it("sets maxAttempts as class member variable", () => { + [1, 2, 3].forEach(maxAttempts => { + const retryStrategy = new StandardRetryStrategy(maxAttempts); + expect(retryStrategy.maxAttempts).toBe(maxAttempts); + }); + }); + + describe("retryDecider", () => { + it("sets defaultRetryDecider if options is undefined", () => { + const retryStrategy = new StandardRetryStrategy(maxAttempts); + expect(retryStrategy["retryDecider"]).toBe(defaultRetryDecider); + }); + + it("sets defaultRetryDecider if options.retryDecider is undefined", () => { + const retryStrategy = new StandardRetryStrategy(maxAttempts, {}); + expect(retryStrategy["retryDecider"]).toBe(defaultRetryDecider); + }); + + it("sets options.retryDecider if defined", () => { + const retryDecider = jest.fn(); + const retryStrategy = new StandardRetryStrategy(maxAttempts, { + retryDecider + }); + expect(retryStrategy["retryDecider"]).toBe(retryDecider); + }); + }); + + describe("delayDecider", () => { + it("sets defaultDelayDecider if options is undefined", () => { + const retryStrategy = new StandardRetryStrategy(maxAttempts); + expect(retryStrategy["delayDecider"]).toBe(defaultDelayDecider); + }); + + it("sets defaultDelayDecider if options.delayDecider undefined", () => { + const retryStrategy = new StandardRetryStrategy(maxAttempts, {}); + expect(retryStrategy["delayDecider"]).toBe(defaultDelayDecider); + }); + + it("sets options.delayDecider if defined", () => { + const delayDecider = jest.fn(); + const retryStrategy = new StandardRetryStrategy(maxAttempts, { + delayDecider + }); + expect(retryStrategy["delayDecider"]).toBe(delayDecider); + }); + }); + + describe("delayBase passed to delayDecider", () => { + const testDelayBasePassed = async ( + delayBaseToTest: number, + mockThrottlingError: boolean + ) => { + (isThrottlingError as jest.Mock).mockReturnValueOnce(mockThrottlingError); + + const mockError = new Error("mockError"); + const mockResponse = { + response: "mockResponse", + output: { $metadata: {} } + }; + + const next = jest + .fn() + .mockRejectedValueOnce(mockError) + .mockResolvedValueOnce(mockResponse); + + const retryStrategy = new StandardRetryStrategy(maxAttempts); + await retryStrategy.retry(next, {} as any); + + expect(isThrottlingError as jest.Mock).toHaveBeenCalledTimes(1); + expect(isThrottlingError as jest.Mock).toHaveBeenCalledWith(mockError); + expect(defaultDelayDecider as jest.Mock).toHaveBeenCalledTimes(1); + expect(defaultDelayDecider as jest.Mock).toHaveBeenCalledWith( + delayBaseToTest, + 1 + ); + }; + + it("should be equal to THROTTLING_RETRY_DELAY_BASE if error is throttling error", async () => { + return testDelayBasePassed(THROTTLING_RETRY_DELAY_BASE, true); + }); + + it("should be equal to DEFAULT_RETRY_DELAY_BASE in error is not a throttling error", async () => { + return testDelayBasePassed(DEFAULT_RETRY_DELAY_BASE, false); + }); + }); + + it("should not retry when the handler completes successfully", async () => { + const mockResponse = { + response: "mockResponse", + output: { $metadata: {} } + }; + + const next = jest.fn().mockResolvedValueOnce(mockResponse); + + const retryStrategy = new StandardRetryStrategy(maxAttempts); + const { response, output } = await retryStrategy.retry(next, {} as any); + + expect(response).toStrictEqual(mockResponse.response); + expect(output.$metadata.attempts).toBe(1); + expect(output.$metadata.totalRetryDelay).toBe(0); + expect(defaultRetryDecider as jest.Mock).not.toHaveBeenCalled(); + expect(defaultDelayDecider as jest.Mock).not.toHaveBeenCalled(); + }); + + it("should not retry when retryDecider returns false", async () => { + (defaultRetryDecider as jest.Mock).mockReturnValueOnce(false); + + const mockError = new Error("mockError"); + const next = jest.fn().mockRejectedValueOnce(mockError); + + const retryStrategy = new StandardRetryStrategy(maxAttempts); + try { + await retryStrategy.retry(next, {} as any); + } catch (error) { + expect(error).toStrictEqual(mockError); + } + + expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledTimes(1); + expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledWith(mockError); + }); + + it("should stop retrying when the the maximum number of attempts is reached", async () => { + const mockError = new Error("mockError"); + const next = jest.fn().mockRejectedValue(mockError); + + const retryStrategy = new StandardRetryStrategy(maxAttempts); + try { + await retryStrategy.retry(next, {} as any); + } catch (error) { + expect(error).toStrictEqual(mockError); + } + expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledTimes( + maxAttempts - 1 + ); + }); + + it("should delay the value returned by delayDecider", async () => { + jest.spyOn(global, "setTimeout"); + + const FIRST_DELAY = 100; + const SECOND_DELAY = 200; + + (defaultDelayDecider as jest.Mock) + .mockReturnValueOnce(FIRST_DELAY) + .mockReturnValueOnce(SECOND_DELAY); + + const mockError = new Error("mockError"); + const next = jest.fn().mockRejectedValue(mockError); + + const retryStrategy = new StandardRetryStrategy(3); + try { + await retryStrategy.retry(next, {} as any); + } catch (error) { + expect(error).toStrictEqual(mockError); + expect(error.$metadata.totalRetryDelay).toEqual( + FIRST_DELAY + SECOND_DELAY + ); + } + + expect(defaultDelayDecider as jest.Mock).toHaveBeenCalledTimes(2); + expect(setTimeout).toHaveBeenCalledTimes(2); + expect(setTimeout).toHaveBeenNthCalledWith( + 1, + expect.any(Function), + FIRST_DELAY + ); + expect(setTimeout).toHaveBeenNthCalledWith( + 2, + expect.any(Function), + SECOND_DELAY + ); + }); +}); diff --git a/packages/middleware-retry/src/index.spec.ts b/packages/middleware-retry/src/index.spec.ts deleted file mode 100644 index 3c1f972b120a..000000000000 --- a/packages/middleware-retry/src/index.spec.ts +++ /dev/null @@ -1,92 +0,0 @@ -import { - DEFAULT_RETRY_DELAY_BASE, - THROTTLING_RETRY_DELAY_BASE -} from "./constants"; -import { retryMiddleware } from "./retryMiddleware"; -import { resolveRetryConfig } from "./configurations"; -import * as delayDeciderModule from "./delayDecider"; -import { StandardRetryStrategy, RetryDecider } from "./defaultStrategy"; -import { HttpRequest } from "@aws-sdk/protocol-http"; -import { SdkError } from "@aws-sdk/smithy-client"; - -describe("retryMiddleware", () => { - it("should not retry when the handler completes successfully", async () => { - const next = jest.fn().mockResolvedValue({ output: { $metadata: {} } }); - const retryHandler = retryMiddleware( - resolveRetryConfig({ maxAttempts: 0 }) - )(next); - - const { - output: { $metadata } - } = await retryHandler({ input: {}, request: new HttpRequest({}) }); - expect($metadata.attempts).toBe(1); - expect($metadata.totalRetryDelay).toBe(0); - - expect(next.mock.calls.length).toBe(1); - }); - - it("should stop retrying when the the maximum number of retries is reached", async () => { - const maxAttempts = 3; - const error = new Error(); - error.name = "ProvisionedThroughputExceededException"; - const next = jest.fn().mockRejectedValue(error); - const retryHandler = retryMiddleware(resolveRetryConfig({ maxAttempts }))( - next - ); - - await expect( - retryHandler({ input: {}, request: new HttpRequest({}) }) - ).rejects.toMatchObject(error); - - expect(next.mock.calls.length).toBe(maxAttempts); - }); - - it("should not retry if the error is not transient", async () => { - const error = new Error(); - error.name = "ValidationException"; - const next = jest.fn().mockRejectedValue(error); - const retryHandler = retryMiddleware( - resolveRetryConfig({ maxAttempts: 3 }) - )(next); - - await expect( - retryHandler({ input: {}, request: new HttpRequest({}) }) - ).rejects.toMatchObject(error); - - expect(next.mock.calls.length).toBe(1); - }); - - it("should use a higher base delay when a throttling error is encountered", async () => { - const next = jest.fn().mockResolvedValue({ output: { $metadata: {} } }); - - const validation = new Error(); - validation.name = "ValidationException"; - next.mockImplementationOnce(args => Promise.reject(validation)); - - const throttling = new Error(); - throttling.name = "RequestLimitExceeded"; - next.mockImplementationOnce(args => Promise.reject(throttling)); - - jest.mock("./delayDecider"); - - const maxAttempts = 3; - const delayDeciderMock = jest.spyOn( - delayDeciderModule, - "defaultDelayDecider" - ); - const retryDecider: RetryDecider = (error: SdkError) => true; - const strategy = new StandardRetryStrategy(maxAttempts, { retryDecider }); - const retryHandler = retryMiddleware({ - maxAttempts, - retryStrategy: strategy - })(next); - - await retryHandler({ input: {}, request: new HttpRequest({}) }); - - expect(next.mock.calls.length).toBe(3); - expect(delayDeciderMock.mock.calls).toEqual([ - [DEFAULT_RETRY_DELAY_BASE, 1], - [THROTTLING_RETRY_DELAY_BASE, 2] - ]); - }); -}); From 1866242251b9fd07d885d4f75c667f11f20cb3aa Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 9 Jun 2020 23:26:25 +0000 Subject: [PATCH 9/9] chore: moved shouldn't retry tests under describe --- .../src/defaultStrategy.spec.ts | 84 ++++++++++--------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/packages/middleware-retry/src/defaultStrategy.spec.ts b/packages/middleware-retry/src/defaultStrategy.spec.ts index 626c8177f5d3..0b9207a34eeb 100644 --- a/packages/middleware-retry/src/defaultStrategy.spec.ts +++ b/packages/middleware-retry/src/defaultStrategy.spec.ts @@ -112,57 +112,59 @@ describe("defaultStrategy", () => { }); }); - it("should not retry when the handler completes successfully", async () => { - const mockResponse = { - response: "mockResponse", - output: { $metadata: {} } - }; - - const next = jest.fn().mockResolvedValueOnce(mockResponse); + describe("should not retry", () => { + it("when the handler completes successfully", async () => { + const mockResponse = { + response: "mockResponse", + output: { $metadata: {} } + }; - const retryStrategy = new StandardRetryStrategy(maxAttempts); - const { response, output } = await retryStrategy.retry(next, {} as any); + const next = jest.fn().mockResolvedValueOnce(mockResponse); - expect(response).toStrictEqual(mockResponse.response); - expect(output.$metadata.attempts).toBe(1); - expect(output.$metadata.totalRetryDelay).toBe(0); - expect(defaultRetryDecider as jest.Mock).not.toHaveBeenCalled(); - expect(defaultDelayDecider as jest.Mock).not.toHaveBeenCalled(); - }); + const retryStrategy = new StandardRetryStrategy(maxAttempts); + const { response, output } = await retryStrategy.retry(next, {} as any); - it("should not retry when retryDecider returns false", async () => { - (defaultRetryDecider as jest.Mock).mockReturnValueOnce(false); + expect(response).toStrictEqual(mockResponse.response); + expect(output.$metadata.attempts).toBe(1); + expect(output.$metadata.totalRetryDelay).toBe(0); + expect(defaultRetryDecider as jest.Mock).not.toHaveBeenCalled(); + expect(defaultDelayDecider as jest.Mock).not.toHaveBeenCalled(); + }); - const mockError = new Error("mockError"); - const next = jest.fn().mockRejectedValueOnce(mockError); + it("when retryDecider returns false", async () => { + (defaultRetryDecider as jest.Mock).mockReturnValueOnce(false); - const retryStrategy = new StandardRetryStrategy(maxAttempts); - try { - await retryStrategy.retry(next, {} as any); - } catch (error) { - expect(error).toStrictEqual(mockError); - } + const mockError = new Error("mockError"); + const next = jest.fn().mockRejectedValueOnce(mockError); - expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledTimes(1); - expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledWith(mockError); - }); + const retryStrategy = new StandardRetryStrategy(maxAttempts); + try { + await retryStrategy.retry(next, {} as any); + } catch (error) { + expect(error).toStrictEqual(mockError); + } + + expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledTimes(1); + expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledWith(mockError); + }); - it("should stop retrying when the the maximum number of attempts is reached", async () => { - const mockError = new Error("mockError"); - const next = jest.fn().mockRejectedValue(mockError); + it("when the the maximum number of attempts is reached", async () => { + const mockError = new Error("mockError"); + const next = jest.fn().mockRejectedValue(mockError); - const retryStrategy = new StandardRetryStrategy(maxAttempts); - try { - await retryStrategy.retry(next, {} as any); - } catch (error) { - expect(error).toStrictEqual(mockError); - } - expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledTimes( - maxAttempts - 1 - ); + const retryStrategy = new StandardRetryStrategy(maxAttempts); + try { + await retryStrategy.retry(next, {} as any); + } catch (error) { + expect(error).toStrictEqual(mockError); + } + expect(defaultRetryDecider as jest.Mock).toHaveBeenCalledTimes( + maxAttempts - 1 + ); + }); }); - it("should delay the value returned by delayDecider", async () => { + it("should delay equal to the value returned by delayDecider", async () => { jest.spyOn(global, "setTimeout"); const FIRST_DELAY = 100;