From a6dda68d50325f6ac73d083b4f14cd8a363e3f9c Mon Sep 17 00:00:00 2001 From: dominik-zeglen Date: Mon, 6 Jul 2020 16:24:54 +0200 Subject: [PATCH 1/5] Handle task failure and fix task duplication --- .../BackgroundTasksProvider.test.tsx | 125 ++++++++++++------ .../BackgroundTasksProvider.tsx | 46 ++++--- src/containers/BackgroundTasks/tasks.ts | 15 ++- src/containers/BackgroundTasks/types.ts | 17 ++- 4 files changed, 137 insertions(+), 66 deletions(-) diff --git a/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx b/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx index 51e20fb1f..bd86862c2 100644 --- a/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx +++ b/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx @@ -1,22 +1,34 @@ import { renderHook } from "@testing-library/react-hooks"; +import { createMockClient } from "mock-apollo-client"; import { backgroundTasksRefreshTime, useBackgroundTasks } from "./BackgroundTasksProvider"; -import { Task, TaskData } from "./types"; +import { OnCompletedTaskData, Task, TaskData, TaskStatus } from "./types"; jest.useFakeTimers(); +function renderBackgroundTasks() { + const mockClient = createMockClient(); + const intl = { + formatMessage: ({ defaultMessage }) => defaultMessage + }; + + return renderHook(() => + useBackgroundTasks(mockClient, jest.fn(), intl as any) + ); +} + describe("Background task provider", () => { it("can queue a task", done => { - const handle = jest.fn, []>( - () => new Promise(resolve => resolve(true)) + const handle = jest.fn, []>( + () => new Promise(resolve => resolve(TaskStatus.SUCCESS)) ); const onCompleted = jest.fn(); const onError = jest.fn(); - const { result } = renderHook(useBackgroundTasks); + const { result } = renderBackgroundTasks(); const taskId = result.current.queue(Task.CUSTOM, { handle, @@ -24,23 +36,23 @@ describe("Background task provider", () => { onError }); expect(taskId).toBe(1); - expect(handle).not.toHaveBeenCalled(); - expect(onCompleted).not.toHaveBeenCalled(); - expect(onError).not.toHaveBeenCalled(); + expect(handle).toHaveBeenCalledTimes(0); + expect(onCompleted).toHaveBeenCalledTimes(0); + expect(onError).toHaveBeenCalledTimes(0); jest.runOnlyPendingTimers(); setImmediate(() => { - expect(handle).toHaveBeenCalled(); - expect(onCompleted).toHaveBeenCalled(); - expect(onError).not.toHaveBeenCalled(); + expect(handle).toHaveBeenCalledTimes(1); + expect(onCompleted).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledTimes(0); done(); }); }); it("can handle task error", done => { - const handle = jest.fn, []>( + const handle = jest.fn, []>( () => new Promise(() => { throw new Error("dummy error"); @@ -49,7 +61,7 @@ describe("Background task provider", () => { const onCompleted = jest.fn(); const onError = jest.fn(); - const { result } = renderHook(useBackgroundTasks); + const { result } = renderBackgroundTasks(); result.current.queue(Task.CUSTOM, { handle, @@ -60,9 +72,36 @@ describe("Background task provider", () => { jest.runOnlyPendingTimers(); setImmediate(() => { - expect(handle).toHaveBeenCalled(); - expect(onCompleted).not.toHaveBeenCalled(); - expect(onError).toHaveBeenCalled(); + expect(handle).toHaveBeenCalledTimes(1); + expect(onCompleted).toHaveBeenCalledTimes(0); + expect(onError).toHaveBeenCalledTimes(1); + + done(); + }); + }); + + it("can handle task failure", done => { + const handle = jest.fn, []>( + () => new Promise(resolve => resolve(TaskStatus.FAILURE)) + ); + const onCompleted = jest.fn((data: OnCompletedTaskData) => + expect(data.status).toBe(TaskStatus.FAILURE) + ); + const onError = jest.fn(); + + const { result } = renderBackgroundTasks(); + + result.current.queue(Task.CUSTOM, { + handle, + onCompleted, + onError + }); + + jest.runOnlyPendingTimers(); + + setImmediate(() => { + expect(handle).toHaveBeenCalledTimes(1); + expect(onCompleted).toHaveBeenCalledTimes(1); done(); }); @@ -71,10 +110,10 @@ describe("Background task provider", () => { it("can cancel task", done => { const onCompleted = jest.fn(); - const { result } = renderHook(useBackgroundTasks); + const { result } = renderBackgroundTasks(); const taskId = result.current.queue(Task.CUSTOM, { - handle: () => new Promise(resolve => resolve(true)), + handle: () => new Promise(resolve => resolve(TaskStatus.SUCCESS)), onCompleted }); @@ -85,51 +124,63 @@ describe("Background task provider", () => { jest.runOnlyPendingTimers(); setImmediate(() => { - expect(onCompleted).not.toHaveBeenCalled(); + expect(onCompleted).toHaveBeenCalledTimes(0); done(); }); }); it("can queue multiple tasks", done => { - const responses: Array> = [ - new Promise(resolve => - setTimeout(() => resolve(true), backgroundTasksRefreshTime * 1.4) - ), - new Promise(resolve => - setTimeout(() => resolve(true), backgroundTasksRefreshTime * 2.1) - ) + let cycle = 0; + + const tasks: TaskData[] = [ + { + handle: jest.fn(() => + Promise.resolve(cycle > 1 ? TaskStatus.SUCCESS : TaskStatus.PENDING) + ), + onCompleted: jest.fn() + }, + { + handle: jest.fn(() => + Promise.resolve(cycle > 2 ? TaskStatus.SUCCESS : TaskStatus.PENDING) + ), + onCompleted: jest.fn() + } ]; - const tasks: TaskData[] = responses.map(response => ({ - handle: () => response, - onCompleted: jest.fn() - })); - - const { result } = renderHook(useBackgroundTasks); + const { result } = renderBackgroundTasks(); tasks.forEach(task => result.current.queue(Task.CUSTOM, task)); // Set time to backgroundTasksRefreshTime + cycle += 1; jest.advanceTimersByTime(backgroundTasksRefreshTime + 100); setImmediate(() => { - expect(tasks[0].onCompleted).not.toHaveBeenCalled(); - expect(tasks[1].onCompleted).not.toHaveBeenCalled(); + expect(tasks[0].handle).toHaveBeenCalledTimes(1); + expect(tasks[1].handle).toHaveBeenCalledTimes(1); + expect(tasks[0].onCompleted).toHaveBeenCalledTimes(0); + expect(tasks[1].onCompleted).toHaveBeenCalledTimes(0); // Set time to backgroundTasksRefreshTime * 2 + cycle += 1; jest.advanceTimersByTime(backgroundTasksRefreshTime); setImmediate(() => { - expect(tasks[0].onCompleted).toHaveBeenCalled(); - expect(tasks[1].onCompleted).not.toHaveBeenCalled(); + expect(tasks[0].handle).toHaveBeenCalledTimes(2); + expect(tasks[1].handle).toHaveBeenCalledTimes(2); + expect(tasks[0].onCompleted).toHaveBeenCalledTimes(1); + expect(tasks[1].onCompleted).toHaveBeenCalledTimes(0); // Set time to backgroundTasksRefreshTime * 3 + cycle += 1; jest.advanceTimersByTime(backgroundTasksRefreshTime); setImmediate(() => { - expect(tasks[0].onCompleted).toHaveBeenCalled(); - expect(tasks[1].onCompleted).toHaveBeenCalled(); + expect(tasks[0].handle).toHaveBeenCalledTimes(2); + expect(tasks[1].handle).toHaveBeenCalledTimes(3); + expect(tasks[0].onCompleted).toHaveBeenCalledTimes(1); + expect(tasks[1].onCompleted).toHaveBeenCalledTimes(1); done(); }); diff --git a/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx b/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx index 7199956d9..cea93fc06 100644 --- a/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx +++ b/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx @@ -1,4 +1,9 @@ +import { IMessageContext } from "@saleor/components/messages"; +import useNotifier from "@saleor/hooks/useNotifier"; +import ApolloClient from "apollo-client"; import React from "react"; +import { useApolloClient } from "react-apollo"; +import { IntlShape, useIntl } from "react-intl"; import BackgroundTasksContext from "./context"; import { handleTask, queueCustom } from "./tasks"; @@ -6,31 +11,35 @@ import { QueuedTask, Task, TaskData, TaskStatus } from "./types"; export const backgroundTasksRefreshTime = 15 * 1000; -export function useBackgroundTasks() { +// TODO: Remove underscores when these arguments would be finally useful +export function useBackgroundTasks( + _apolloClient: ApolloClient, + _notify: IMessageContext, + _intl: IntlShape +) { const idCounter = React.useRef(0); const tasks = React.useRef([]); React.useEffect(() => { const intervalId = setInterval(() => { const queue = async () => { - tasks.current = tasks.current.filter( - task => task.status !== TaskStatus.ENDED - ); try { await Promise.all( tasks.current.map(async task => { - let hasFinished: boolean; + if (task.status === TaskStatus.PENDING) { + let status: TaskStatus; - try { - hasFinished = await handleTask(task); - } catch (error) { - throw error; - } - if (hasFinished) { - const taskIndex = tasks.current.findIndex( - t => t.id === task.id - ); - tasks.current[taskIndex].status = TaskStatus.ENDED; + try { + status = await handleTask(task); + } catch (error) { + throw error; + } + if (status !== TaskStatus.PENDING) { + const taskIndex = tasks.current.findIndex( + t => t.id === task.id + ); + tasks.current[taskIndex].status = status; + } } }) ); @@ -51,10 +60,10 @@ export function useBackgroundTasks() { function queue(type: Task, data?: TaskData) { idCounter.current += 1; - switch (type) { case Task.CUSTOM: queueCustom(idCounter.current, tasks, data); + break; } return idCounter.current; @@ -67,7 +76,10 @@ export function useBackgroundTasks() { } const BackgroundTasksProvider: React.FC = ({ children }) => { - const { cancel, queue } = useBackgroundTasks(); + const apolloClient = useApolloClient(); + const notify = useNotifier(); + const intl = useIntl(); + const { cancel, queue } = useBackgroundTasks(apolloClient, notify, intl); return ( { - let ok = false; +export async function handleTask(task: QueuedTask): Promise { + let status = TaskStatus.PENDING; try { - ok = await task.handle(); - if (ok) { - task.onCompleted(); + status = await task.handle(); + if (status !== TaskStatus.PENDING) { + task.onCompleted({ + status + }); } } catch (error) { task.onError(error); } - return ok; + return status; } export function handleError(error: Error) { @@ -28,7 +30,6 @@ export function queueCustom( .forEach(field => { throw new Error(`${field} is required when creating custom task`); }); - tasks.current = [ ...tasks.current, { diff --git a/src/containers/BackgroundTasks/types.ts b/src/containers/BackgroundTasks/types.ts index a4e328e0e..0eabb6f7c 100644 --- a/src/containers/BackgroundTasks/types.ts +++ b/src/containers/BackgroundTasks/types.ts @@ -2,21 +2,28 @@ export enum Task { CUSTOM } export enum TaskStatus { + FAILURE, PENDING, - ENDED + SUCCESS } +export interface OnCompletedTaskData { + status: TaskStatus; +} +export type OnCompletedTaskFn = (data: OnCompletedTaskData) => void; + export interface QueuedTask { id: number; - handle: () => Promise; + handle: () => Promise; status: TaskStatus; - onCompleted: () => void; + onCompleted: OnCompletedTaskFn; onError: (error: Error) => void; } export interface TaskData { - handle?: () => Promise; - onCompleted?: () => void; + id?: string; + handle?: () => Promise; + onCompleted?: OnCompletedTaskFn; onError?: () => void; } From d89d31d295dcd620cf0b3b02e45f8e7cd1ecd14e Mon Sep 17 00:00:00 2001 From: dominik-zeglen Date: Mon, 6 Jul 2020 16:35:33 +0200 Subject: [PATCH 2/5] Update dependencies --- package-lock.json | 6 ++++++ package.json | 1 + 2 files changed, 7 insertions(+) diff --git a/package-lock.json b/package-lock.json index 46769d8b4..beffc6dc7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16014,6 +16014,12 @@ } } }, + "mock-apollo-client": { + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/mock-apollo-client/-/mock-apollo-client-0.4.0.tgz", + "integrity": "sha512-cHznpkX8uUClkWWJMpgdDWzEgjacM85xt69S9gPLrssM8Vahas0QmEJkFUycrRQyBkaqxvRe58Bg3a5pOvj2zA==", + "dev": true + }, "moment": { "version": "2.24.0", "resolved": "https://registry.npmjs.org/moment/-/moment-2.24.0.tgz", diff --git a/package.json b/package.json index 9c25281c6..e94c6b0fa 100644 --- a/package.json +++ b/package.json @@ -131,6 +131,7 @@ "jest": "^24.8.0", "jest-file": "^1.0.0", "lint-staged": "^9.4.2", + "mock-apollo-client": "^0.4.0", "prettier": "^1.19.1", "react-intl-translations-manager": "^5.0.3", "react-test-renderer": "^16.12.0", From 48bb868a0142bcf9637e2e58f67a7e9010ca345c Mon Sep 17 00:00:00 2001 From: dominik-zeglen Date: Mon, 6 Jul 2020 16:36:00 +0200 Subject: [PATCH 3/5] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7598ac2d1..016819750 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ All notable, unreleased changes to this project will be documented in this file. - Handle untracked products - #523 by @dominik-zeglen - Display correct error if there were no graphql errors - #525 by @dominik-zeglen - Add background task manager - #574 by @dominik-zeglen +- Handle task failure and fix task duplication - #588 by @dominik-zeglen ## 2.0.0 From b1ac074b1ef23e529599908aa7afcff122fcf96a Mon Sep 17 00:00:00 2001 From: dominik-zeglen Date: Mon, 6 Jul 2020 17:06:01 +0200 Subject: [PATCH 4/5] Improve comment --- src/containers/BackgroundTasks/BackgroundTasksProvider.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx b/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx index cea93fc06..b6948caef 100644 --- a/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx +++ b/src/containers/BackgroundTasks/BackgroundTasksProvider.tsx @@ -11,7 +11,7 @@ import { QueuedTask, Task, TaskData, TaskStatus } from "./types"; export const backgroundTasksRefreshTime = 15 * 1000; -// TODO: Remove underscores when these arguments would be finally useful +// TODO: Remove underscores when working on #575 or similar PR export function useBackgroundTasks( _apolloClient: ApolloClient, _notify: IMessageContext, From b28bd31aaa2e710784a7be84d09f89cdfbbd3e84 Mon Sep 17 00:00:00 2001 From: dominik-zeglen Date: Mon, 6 Jul 2020 19:04:51 +0200 Subject: [PATCH 5/5] Commit review suggestioins --- .../BackgroundTasksProvider.test.tsx | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx b/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx index bd86862c2..0151816e0 100644 --- a/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx +++ b/src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx @@ -133,20 +133,22 @@ describe("Background task provider", () => { it("can queue multiple tasks", done => { let cycle = 0; - const tasks: TaskData[] = [ - { - handle: jest.fn(() => - Promise.resolve(cycle > 1 ? TaskStatus.SUCCESS : TaskStatus.PENDING) - ), - onCompleted: jest.fn() - }, - { - handle: jest.fn(() => - Promise.resolve(cycle > 2 ? TaskStatus.SUCCESS : TaskStatus.PENDING) - ), - onCompleted: jest.fn() - } - ]; + // Completed in two cycles + const shortTask = { + handle: jest.fn(() => + Promise.resolve(cycle > 1 ? TaskStatus.SUCCESS : TaskStatus.PENDING) + ), + onCompleted: jest.fn() + }; + + // Completed in three cycles + const longTask = { + handle: jest.fn(() => + Promise.resolve(cycle > 2 ? TaskStatus.SUCCESS : TaskStatus.PENDING) + ), + onCompleted: jest.fn() + }; + const tasks: TaskData[] = [shortTask, longTask]; const { result } = renderBackgroundTasks(); @@ -157,30 +159,30 @@ describe("Background task provider", () => { jest.advanceTimersByTime(backgroundTasksRefreshTime + 100); setImmediate(() => { - expect(tasks[0].handle).toHaveBeenCalledTimes(1); - expect(tasks[1].handle).toHaveBeenCalledTimes(1); - expect(tasks[0].onCompleted).toHaveBeenCalledTimes(0); - expect(tasks[1].onCompleted).toHaveBeenCalledTimes(0); + expect(shortTask.handle).toHaveBeenCalledTimes(1); + expect(longTask.handle).toHaveBeenCalledTimes(1); + expect(shortTask.onCompleted).toHaveBeenCalledTimes(0); + expect(longTask.onCompleted).toHaveBeenCalledTimes(0); // Set time to backgroundTasksRefreshTime * 2 cycle += 1; jest.advanceTimersByTime(backgroundTasksRefreshTime); setImmediate(() => { - expect(tasks[0].handle).toHaveBeenCalledTimes(2); - expect(tasks[1].handle).toHaveBeenCalledTimes(2); - expect(tasks[0].onCompleted).toHaveBeenCalledTimes(1); - expect(tasks[1].onCompleted).toHaveBeenCalledTimes(0); + expect(shortTask.handle).toHaveBeenCalledTimes(2); + expect(longTask.handle).toHaveBeenCalledTimes(2); + expect(shortTask.onCompleted).toHaveBeenCalledTimes(1); + expect(longTask.onCompleted).toHaveBeenCalledTimes(0); // Set time to backgroundTasksRefreshTime * 3 cycle += 1; jest.advanceTimersByTime(backgroundTasksRefreshTime); setImmediate(() => { - expect(tasks[0].handle).toHaveBeenCalledTimes(2); - expect(tasks[1].handle).toHaveBeenCalledTimes(3); - expect(tasks[0].onCompleted).toHaveBeenCalledTimes(1); - expect(tasks[1].onCompleted).toHaveBeenCalledTimes(1); + expect(shortTask.handle).toHaveBeenCalledTimes(2); + expect(longTask.handle).toHaveBeenCalledTimes(3); + expect(shortTask.onCompleted).toHaveBeenCalledTimes(1); + expect(longTask.onCompleted).toHaveBeenCalledTimes(1); done(); });