coder · Parkreiner · May 15, 2024 · May 12, 2024 · May 12, 2024 · May 12, 2024
diff --git a/site/src/hooks/useClipboard.test.tsx b/site/src/hooks/useClipboard.test.tsx
@@ -1,129 +1,131 @@
-import { act, renderHook } from "@testing-library/react";
+/**
+ * @file The test setup for this file is a little funky because of how React
+ * Testing Library works.
+ *
+ * When you call user.setup to make a new user session, it will make a mock
+ * clipboard instance that will always succeed. It also can't be removed after
+ * it's been added, and it will persist across test cases. This actually makes
+ * testing useClipboard properly impossible because any call to user.setup
+ * immediately pollutes the tests with false negatives. Even if something should
+ * fail, it won't.
+ */
+import { act, renderHook, screen } from "@testing-library/react";
 import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar";
 import { ThemeProvider } from "contexts/ThemeProvider";
 import {
   type UseClipboardInput,
   type UseClipboardResult,
+  COPY_FAILED_MESSAGE,
   useClipboard,
+  HTTP_FALLBACK_DATA_ID,
 } from "./useClipboard";
 
-describe(useClipboard.name, () => {
-  describe("HTTP (non-secure) connections", () => {
-    scheduleClipboardTests({ isHttps: false });
-  });
-
-  describe("HTTPS (secure/default) connections", () => {
-    scheduleClipboardTests({ isHttps: true });
-  });
-});
-
-/**
- * @file This is a very weird test setup.
- *
- * There are two main things that it's fighting against to insure that the
- * clipboard functionality is working as expected:
- * 1. userEvent.setup's default global behavior
- * 2. The fact that we need to reuse the same set of test cases for two separate
- *    contexts (secure and insecure), each with their own version of global
- *    state.
- *
- * The goal of this file is to provide a shared set of test behavior that can
- * be imported into two separate test files (one for HTTP, one for HTTPS),
- * without any risk of global state conflicts.
- *
- * ---
- * For (1), normally you could call userEvent.setup to enable clipboard mocking,
- * but userEvent doesn't expose a teardown function. It also modifies the global
- * scope for the whole test file, so enabling just one userEvent session will
- * make a mock clipboard exist for all other tests, even though you didn't tell
- * them to set up a session. The mock also assumes that the clipboard API will
- * always be available, which is not true on HTTP-only connections
- *
- * Since these tests need to split hairs and differentiate between HTTP and
- * HTTPS connections, setting up a single userEvent is disastrous. It will make
- * all the tests pass, even if they shouldn't. Have to avoid that by creating a
- * custom clipboard mock.
- *
- * ---
- * For (2), we're fighting against Jest's default behavior, which is to treat
- * the test file as the main boundary for test environments, with each test case
- * able to run in parallel. That works if you have one single global state, but
- * we need two separate versions of the global state, while repeating the exact
- * same test cases for each one.
- *
- * If both tests were to be placed in the same file, Jest would not isolate them
- * and would let their setup steps interfere with each other. This leads to one
- * of two things:
- * 1. One of the global mocks overrides the other, making it so that one
- *    connection type always fails
- * 2. The two just happen not to conflict each other, through some convoluted
- *    order of operations involving closure, but you have no idea why the code
- *    is working, and it's impossible to debug.
- */
-type MockClipboardEscapeHatches = Readonly<{
-  getMockText: () => string;
-  setMockText: (newText: string) => void;
-  simulateFailure: boolean;
-  setSimulateFailure: (failureMode: boolean) => void;
+// execCommand is the workaround for copying text to the clipboard on HTTP-only
+// connections
+const originalExecCommand = global.document.execCommand;
+const originalNavigator = window.navigator;
+
+// Need to mock console.error because we deliberately need to trigger errors in
+// the code to assert that it can recover from them, but we also don't want them
+// logged if they're expected
+const originalConsoleError = console.error;
+
+type SetupMockClipboardResult = Readonly<{
+  mockClipboard: Clipboard;
+  mockExecCommand: typeof originalExecCommand;
+  getClipboardText: () => string;
+  setSimulateFailure: (shouldFail: boolean) => void;
+  resetClipboardMocks: () => void;
 }>;
 
-type MockClipboard = Readonly<Clipboard & MockClipboardEscapeHatches>;
-function makeMockClipboard(isSecureContext: boolean): MockClipboard {
-  let mockClipboardValue = "";
-  let shouldFail = false;
-
-  return {
-    get simulateFailure() {
-      return shouldFail;
-    },
-    setSimulateFailure: (value) => {
-      shouldFail = value;
-    },
+function setupMockClipboard(isSecure: boolean): SetupMockClipboardResult {
+  let mockClipboardText = "";
+  let shouldSimulateFailure = false;
 
+  const mockClipboard: Clipboard = {
     readText: async () => {
-      if (shouldFail) {
-        throw new Error("Clipboard deliberately failed");
-      }
-
-      if (!isSecureContext) {
+      if (!isSecure) {
         throw new Error(
-          "Trying to read from clipboard outside secure context!",
+          "Not allowed to access clipboard outside of secure contexts",
         );
       }
 
-      return mockClipboardValue;
+      if (shouldSimulateFailure) {
+        throw new Error("Failed to read from clipboard");
+      }
+
+      return mockClipboardText;
     },
+
     writeText: async (newText) => {
-      if (shouldFail) {
-        throw new Error("Clipboard deliberately failed");
+      if (!isSecure) {
+        throw new Error(
+          "Not allowed to access clipboard outside of secure contexts",
+        );
       }
 
-      if (!isSecureContext) {
-        throw new Error("Trying to write to clipboard outside secure context!");
+      if (shouldSimulateFailure) {
+        throw new Error("Failed to write to clipboard");
       }
 
-      mockClipboardValue = newText;
-    },
-
-    getMockText: () => mockClipboardValue,
-    setMockText: (newText) => {
-      mockClipboardValue = newText;
+      mockClipboardText = newText;
     },
 
+    // Don't need these other methods for any of the tests; read and write are
+    // both synchronous and slower than the promise-based methods, so ideally
+    // we won't ever need to call them in the hook logic
     addEventListener: jest.fn(),
     removeEventListener: jest.fn(),
     dispatchEvent: jest.fn(),
     read: jest.fn(),
     write: jest.fn(),
   };
+
+  return {
+    mockClipboard,
+    getClipboardText: () => mockClipboardText,
+    setSimulateFailure: (newShouldFailValue) => {
+      shouldSimulateFailure = newShouldFailValue;
+    },
+    resetClipboardMocks: () => {
+      shouldSimulateFailure = false;
+      mockClipboardText = "";
+    },
+    mockExecCommand: (commandId) => {
+      if (commandId !== "copy") {
+        return false;
+      }
+
+      if (shouldSimulateFailure) {
+        throw new Error("Failed to execute command 'copy'");
+      }
+
+      const dummyInput = document.querySelector(
+        `input[data-testid=${HTTP_FALLBACK_DATA_ID}]`,
+      );
+
+      const inputIsFocused =
+        dummyInput instanceof HTMLInputElement &&
+        document.activeElement === dummyInput;
+
+      let copySuccessful = false;
+      if (inputIsFocused) {
+        mockClipboardText = dummyInput.value;
+        copySuccessful = true;
+      }
+
+      return copySuccessful;
+    },
+  };
 }
 
-function renderUseClipboard(inputs: UseClipboardInput) {
-  return renderHook<UseClipboardResult, UseClipboardInput>(
+function renderUseClipboard<TInput extends UseClipboardInput>(inputs: TInput) {
+  return renderHook<UseClipboardResult, TInput>(
     (props) => useClipboard(props),
     {
       initialProps: inputs,
       wrapper: ({ children }) => (
+        // Need ThemeProvider because GlobalSnackbar uses theme
         <ThemeProvider>
           {children}
           <GlobalSnackbar />
@@ -133,88 +135,120 @@ function renderUseClipboard(inputs: UseClipboardInput) {
   );
 }
 
-type ScheduleConfig = Readonly<{ isHttps: boolean }>;
+const secureContextValues: readonly boolean[] = [true, false];
 
-export function scheduleClipboardTests({ isHttps }: ScheduleConfig) {
-  const mockClipboardInstance = makeMockClipboard(isHttps);
-  const originalNavigator = window.navigator;
+// Not a big fan of describe.each most of the time, but since we need to test
+// the exact same test cases against different inputs, and we want them to run
+// as sequentially as possible to minimize flakes, they make sense here
+describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
+  const {
+    mockClipboard,
+    mockExecCommand,
+    getClipboardText,
+    setSimulateFailure,
+    resetClipboardMocks,
+  } = setupMockClipboard(isSecure);
 
   beforeEach(() => {
     jest.useFakeTimers();
+    global.document.execCommand = mockExecCommand;
     jest.spyOn(window, "navigator", "get").mockImplementation(() => ({
       ...originalNavigator,
-      clipboard: mockClipboardInstance,
+      clipboard: mockClipboard,
     }));
 
-    if (!isHttps) {
-      // Not the biggest fan of exposing implementation details like this, but
-      // making any kind of mock for execCommand is really gnarly in general
-      global.document.execCommand = jest.fn(() => {
-        if (mockClipboardInstance.simulateFailure) {
-          return false;
-        }
-
-        const dummyInput = document.querySelector("input[data-testid=dummy]");
-        const inputIsFocused =
-          dummyInput instanceof HTMLInputElement &&
-          document.activeElement === dummyInput;
-
-        let copySuccessful = false;
-        if (inputIsFocused) {
-          mockClipboardInstance.setMockText(dummyInput.value);
-          copySuccessful = true;
-        }
-
-        return copySuccessful;
-      });
-    }
+    console.error = (errorValue, ...rest) => {
+      const canIgnore =
+        errorValue instanceof Error &&
+        errorValue.message === COPY_FAILED_MESSAGE;
+
+      if (!canIgnore) {
+        originalConsoleError(errorValue, ...rest);
+      }
+    };
   });
 
   afterEach(() => {
+    jest.runAllTimers();
     jest.useRealTimers();
-    mockClipboardInstance.setMockText("");
-    mockClipboardInstance.setSimulateFailure(false);
+    jest.resetAllMocks();
+
+    resetClipboardMocks();
+    console.error = originalConsoleError;
+    global.document.execCommand = originalExecCommand;
   });
 
-  const assertClipboardTextUpdate = async (
+  const assertClipboardUpdateLifecycle = async (
     result: ReturnType<typeof renderUseClipboard>["result"],
     textToCheck: string,
   ): Promise<void> => {
     await act(() => result.current.copyToClipboard());
     expect(result.current.showCopiedSuccess).toBe(true);
 
-    const clipboardText = mockClipboardInstance.getMockText();
+    // Because of timing trickery, any timeouts for flipping the copy status
+    // back to false will usually trigger before any test cases calling this
+    // assert function can complete. This will never be an issue in the real
+    // world, but it will kick up 'act' warnings in the console, which makes
+    // tests more annoying. Getting around that by waiting for all timeouts to
+    // wrap up, but note that the value of showCopiedSuccess will become false
+    // after runAllTimersAsync finishes
+    await act(() => jest.runAllTimersAsync());
+
+    const clipboardText = getClipboardText();
     expect(clipboardText).toEqual(textToCheck);
   };
 
-  /**
-   * Start of test cases
-   */
   it("Copies the current text to the user's clipboard", async () => {
     const textToCopy = "dogs";
     const { result } = renderUseClipboard({ textToCopy });
-    await assertClipboardTextUpdate(result, textToCopy);
+    await assertClipboardUpdateLifecycle(result, textToCopy);
   });
 
   it("Should indicate to components not to show successful copy after a set period of time", async () => {
     const textToCopy = "cats";
     const { result } = renderUseClipboard({ textToCopy });
-    await assertClipboardTextUpdate(result, textToCopy);
-
-    setTimeout(() => {
-      expect(result.current.showCopiedSuccess).toBe(false);
-    }, 10_000);
-
-    await jest.runAllTimersAsync();
+    await assertClipboardUpdateLifecycle(result, textToCopy);
+    expect(result.current.showCopiedSuccess).toBe(false);
   });
 
   it("Should notify the user of an error using the provided callback", async () => {
     const textToCopy = "birds";
     const onError = jest.fn();
     const { result } = renderUseClipboard({ textToCopy, onError });
 
-    mockClipboardInstance.setSimulateFailure(true);
+    setSimulateFailure(true);
     await act(() => result.current.copyToClipboard());
     expect(onError).toBeCalled();
   });
-}
+
+  it("Should dispatch a new toast message to the global snackbar when errors happen if no error callback is provided to the hook", async () => {
+    const textToCopy = "crow";
+    const { result } = renderUseClipboard({ textToCopy });
+
+    /**
+     * @todo Look into why deferring error-based state updates to the global
+     * snackbar still kicks up act warnings, even after wrapping copyToClipboard
+     * in act. copyToClipboard should be the main source of the state
+     * transitions, but it looks like extra state changes are still getting
+     * flushed through the GlobalSnackbar component afterwards
+     */
+    setSimulateFailure(true);
+    await act(() => result.current.copyToClipboard());
+
+    const errorMessageNode = screen.queryByText(COPY_FAILED_MESSAGE);
+    expect(errorMessageNode).not.toBeNull();
+  });
+
+  it("Should expose the error as a value when a copy fails", async () => {
+    // Using empty onError callback to silence any possible act warnings from
+    // Snackbar state transitions that you might get if the hook uses the
+    // default
+    const textToCopy = "hamster";
+    const { result } = renderUseClipboard({ textToCopy, onError: jest.fn() });
+
+    setSimulateFailure(true);
+    await act(() => result.current.copyToClipboard());
+
+    expect(result.current.error).toBeInstanceOf(Error);
+  });
+});