20219: Improve useAsyncInterval testability, add unit test
authorStephen Smith <stephen@curii.com>
Wed, 2 Aug 2023 00:40:13 +0000 (20:40 -0400)
committerStephen Smith <stephen@curii.com>
Wed, 2 Aug 2023 00:40:13 +0000 (20:40 -0400)
* Combines useRefs for easier mocking
* Wraps callback execution in promise chain to prevent halting on non-async
  callbacks
* Clean up now unused async contexts
* Add explicit promise resolve/reject on PollProcessLogs
  * Add catch block to continue polling on error

Arvados-DCO-1.1-Signed-off-by: Stephen Smith <stephen@curii.com>

package.json
src/common/use-async-interval.test.tsx [new file with mode: 0644]
src/common/use-async-interval.ts
src/store/process-logs-panel/process-logs-panel-actions.ts
yarn.lock

index 0a10da786e5bc1621a0ea84721ffddaf8a634042..5f1f647d5702a308427a634ab6e676191dbdae1f 100644 (file)
@@ -9,6 +9,7 @@
     "@fortawesome/react-fontawesome": "0.1.9",
     "@material-ui/core": "3.9.3",
     "@material-ui/icons": "3.0.1",
+    "@sinonjs/fake-timers": "^10.3.0",
     "@types/debounce": "3.0.0",
     "@types/file-saver": "2.0.0",
     "@types/js-yaml": "3.11.2",
diff --git a/src/common/use-async-interval.test.tsx b/src/common/use-async-interval.test.tsx
new file mode 100644 (file)
index 0000000..188f184
--- /dev/null
@@ -0,0 +1,96 @@
+// Copyright (C) The Arvados Authors. All rights reserved.
+//
+// SPDX-License-Identifier: AGPL-3.0
+
+import React from 'react';
+import { useAsyncInterval } from './use-async-interval';
+import { configure, mount } from 'enzyme';
+import Adapter from 'enzyme-adapter-react-16';
+import FakeTimers from "@sinonjs/fake-timers";
+
+configure({ adapter: new Adapter() });
+const clock = FakeTimers.install();
+
+jest.mock('react', () => {
+    const originalReact = jest.requireActual('react');
+    const mUseRef = jest.fn();
+    return {
+        ...originalReact,
+        useRef: mUseRef,
+    };
+});
+
+const TestComponent = (props): JSX.Element => {
+    useAsyncInterval(props.callback, 2000);
+    return <span />;
+};
+
+describe('useAsyncInterval', () => {
+    it('should fire repeatedly after the interval', async () => {
+        const mockedReact = React as jest.Mocked<typeof React>;
+        const ref = { current: {} };
+        mockedReact.useRef.mockReturnValue(ref);
+
+        const syncCallback = jest.fn();
+        const testComponent = mount(<TestComponent
+            callback={syncCallback}
+        />);
+
+        // cb queued with interval but not called
+        expect(syncCallback).not.toHaveBeenCalled();
+
+        // wait for first tick
+        await clock.tickAsync(2000);
+        expect(syncCallback).toHaveBeenCalledTimes(1);
+
+        // wait for second tick
+        await clock.tickAsync(2000);
+        expect(syncCallback).toHaveBeenCalledTimes(2);
+
+        // wait for third tick
+        await clock.tickAsync(2000);
+        expect(syncCallback).toHaveBeenCalledTimes(3);
+    });
+
+    it('should wait for async callbacks to complete in between polling', async () => {
+        const mockedReact = React as jest.Mocked<typeof React>;
+        const ref = { current: {} };
+        mockedReact.useRef.mockReturnValue(ref);
+
+        const delayedCallback = jest.fn(() => (
+            new Promise<void>((resolve) => {
+                setTimeout(() => {
+                    resolve();
+                }, 2000);
+            })
+        ));
+        const testComponent = mount(<TestComponent
+            callback={delayedCallback}
+        />);
+
+        // cb queued with setInterval but not called
+        expect(delayedCallback).not.toHaveBeenCalled();
+
+        // Wait 2 seconds for first tick
+        await clock.tickAsync(2000);
+        // First cb called after 2 seconds
+        expect(delayedCallback).toHaveBeenCalledTimes(1);
+        // Wait for cb to resolve for 2 seconds
+        await clock.tickAsync(2000);
+        expect(delayedCallback).toHaveBeenCalledTimes(1);
+
+        // Wait 2 seconds for second tick
+        await clock.tickAsync(2000);
+        expect(delayedCallback).toHaveBeenCalledTimes(2);
+        // Wait for cb to resolve for 2 seconds
+        await clock.tickAsync(2000);
+        expect(delayedCallback).toHaveBeenCalledTimes(2);
+
+        // Wait 2 seconds for third tick
+        await clock.tickAsync(2000);
+        expect(delayedCallback).toHaveBeenCalledTimes(3);
+        // Wait for cb to resolve for 2 seconds
+        await clock.tickAsync(2000);
+        expect(delayedCallback).toHaveBeenCalledTimes(3);
+    });
+});
index 8951a9b0d3caa9d77d6f08e6eb9064eff6abbc79..3be7309a38d481ceb8eb0f5ac91007f8256641cf 100644 (file)
@@ -2,34 +2,44 @@
 //
 // SPDX-License-Identifier: AGPL-3.0
 
-var react = require("react");
+import React from "react";
 
 export const useAsyncInterval = function (callback, delay) {
-    const savedCallback = react.useRef();
-    const active = react.useRef(false);
+    const ref = React.useRef<{cb: () => Promise<any>, active: boolean}>({
+        cb: async () => {},
+        active: false}
+    );
 
     // Remember the latest callback.
-    react.useEffect(() => {
-        savedCallback.current = callback;
+    React.useEffect(() => {
+        ref.current.cb = callback;
     }, [callback]);
     // Set up the interval.
-    react.useEffect(() => {
-        // useEffect doesn't like async callbacks (https://github.com/facebook/react/issues/14326) so create nested async callback
-        (async () => {
-            // Make tick() async
-            async function tick() {
-                if (active.current) {
-                    // If savedCallback is not set yet, no-op until it is
-                    savedCallback.current && await savedCallback.current();
+    React.useEffect(() => {
+        function tick() {
+            if (ref.current.active) {
+                // Wrap execution chain with promise so that execution errors or
+                //   non-async callbacks still fall through to .finally, avoids breaking polling
+                new Promise((resolve) => {
+                    return resolve(ref.current.cb());
+                }).then(() => {
+                    // Promise succeeded
+                    // Possibly implement back-off reset
+                }).catch(() => {
+                    // Promise rejected
+                    // Possibly implement back-off in the future
+                }).finally(() => {
                     setTimeout(tick, delay);
-                }
+                });
             }
-            if (delay !== null) {
-                active.current = true;
-                setTimeout(tick, delay);
-            }
-        })(); // Call nested async function
-        // We return the teardown function here since we can't from inside the nested async callback
-        return () => {active.current = false;};
+        }
+        if (delay !== null) {
+            ref.current.active = true;
+            setTimeout(tick, delay);
+        }
+        // Suppress warning about cleanup function - can be ignored when variables are unrelated to dom elements
+        //   https://github.com/facebook/react/issues/15841#issuecomment-500133759
+        // eslint-disable-next-line
+        return () => {ref.current.active = false;};
     }, [delay]);
 };
index a0d071fd365a5904b1f5e5609dd58ade10ce205a..328ca4da25b889dc358386e5ba68dc8ef5f6aa08 100644 (file)
@@ -92,8 +92,9 @@ export const pollProcessLogs = (processUuid: string) =>
                     await dispatch(processLogsPanelActions.ADD_PROCESS_LOGS_PANEL_ITEM(groupedLogs));
                 }
             }
+            return Promise.resolve();
         } catch (e) {
-            // Failed to poll, ignore error
+            return Promise.reject();
         }
     };
 
index 20bbc46bfe1859e4b9321b024538c6ef84036c15..826baac8d0272f3efba02111cdde57a471387fb2 100644 (file)
--- a/yarn.lock
+++ b/yarn.lock
@@ -2233,6 +2233,24 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@sinonjs/commons@npm:^3.0.0":
+  version: 3.0.0
+  resolution: "@sinonjs/commons@npm:3.0.0"
+  dependencies:
+    type-detect: 4.0.8
+  checksum: b4b5b73d4df4560fb8c0c7b38c7ad4aeabedd362f3373859d804c988c725889cde33550e4bcc7cd316a30f5152a2d1d43db71b6d0c38f5feef71fd8d016763f8
+  languageName: node
+  linkType: hard
+
+"@sinonjs/fake-timers@npm:^10.3.0":
+  version: 10.3.0
+  resolution: "@sinonjs/fake-timers@npm:10.3.0"
+  dependencies:
+    "@sinonjs/commons": ^3.0.0
+  checksum: 614d30cb4d5201550c940945d44c9e0b6d64a888ff2cd5b357f95ad6721070d6b8839cd10e15b76bf5e14af0bcc1d8f9ec00d49a46318f1f669a4bec1d7f3148
+  languageName: node
+  linkType: hard
+
 "@sinonjs/formatio@npm:^3.2.1":
   version: 3.2.2
   resolution: "@sinonjs/formatio@npm:3.2.2"
@@ -3729,6 +3747,7 @@ __metadata:
     "@fortawesome/react-fontawesome": 0.1.9
     "@material-ui/core": 3.9.3
     "@material-ui/icons": 3.0.1
+    "@sinonjs/fake-timers": ^10.3.0
     "@types/classnames": 2.2.6
     "@types/debounce": 3.0.0
     "@types/enzyme": 3.1.14