chore(site): clean up mocks after each test (#12805)

site/jest.polyfills.js

@@ -29,4 +29,16 @@ Object.defineProperties(globalThis, {
   FormData: { value: FormData },
   Request: { value: Request },
   Response: { value: Response },
+  matchMedia: {
+    value: (query) => ({
+      matches: false,
+      media: query,
+      onchange: null,
+      addListener: jest.fn(),
+      removeListener: jest.fn(),
+      addEventListener: jest.fn(),
+      removeEventListener: jest.fn(),
+      dispatchEvent: jest.fn(),
+    }),
+  },
 });

site/jest.setup.ts

@@ -63,7 +63,7 @@ beforeAll(() =>
 afterEach(() => {
   cleanup();
   server.resetHandlers();
-  jest.clearAllMocks();
+  jest.resetAllMocks();
 });
 
 // Clean up after the tests are finished.

site/src/hooks/useClipboard-http.test.ts

@@ -1,15 +0,0 @@
-/**
- * This test is for all useClipboard functionality, with the browser context
- * set to insecure (HTTP connections).
- *
- * See useClipboard.test-setup.ts for more info on why this file is set up the
- * way that it is.
- */
-import { useClipboard } from "./useClipboard";
-import { scheduleClipboardTests } from "./useClipboard.test-setup";
-
-describe(useClipboard.name, () => {
-  describe("HTTP (non-secure) connections", () => {
-    scheduleClipboardTests({ isHttps: false });
-  });
-});

site/src/hooks/useClipboard-https.test.ts

@@ -1,15 +0,0 @@
-/**
- * This test is for all useClipboard functionality, with the browser context
- * set to secure (HTTPS connections).
- *
- * See useClipboard.test-setup.ts for more info on why this file is set up the
- * way that it is.
- */
-import { useClipboard } from "./useClipboard";
-import { scheduleClipboardTests } from "./useClipboard.test-setup";
-
-describe(useClipboard.name, () => {
-  describe("HTTPS (secure/default) connections", () => {
-    scheduleClipboardTests({ isHttps: true });
-  });
-});

site/src/hooks/useClipboard.test.tsx

@@ -1,3 +1,22 @@
+import { act, renderHook } from "@testing-library/react";
+import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar";
+import { ThemeProvider } from "contexts/ThemeProvider";
+import {
+  type UseClipboardInput,
+  type UseClipboardResult,
+  useClipboard,
+} 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.
  *
@@ -41,25 +60,6 @@
  *    order of operations involving closure, but you have no idea why the code
  *    is working, and it's impossible to debug.
  */
-import { act, renderHook } from "@testing-library/react";
-import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar";
-import {
-  type UseClipboardInput,
-  type UseClipboardResult,
-  useClipboard,
-} from "./useClipboard";
-
-const initialExecCommand = global.document.execCommand;
-beforeAll(() => {
-  jest.useFakeTimers();
-});
-
-afterAll(() => {
-  jest.restoreAllMocks();
-  jest.useRealTimers();
-  global.document.execCommand = initialExecCommand;
-});
-
 type MockClipboardEscapeHatches = Readonly<{
   getMockText: () => string;
   setMockText: (newText: string) => void;
@@ -124,10 +124,10 @@ function renderUseClipboard(inputs: UseClipboardInput) {
     {
       initialProps: inputs,
       wrapper: ({ children }) => (
-        <>
-          <>{children}</>
+        <ThemeProvider>
+          {children}
           <GlobalSnackbar />
-        </>
+        </ThemeProvider>
       ),
     },
   );
@@ -137,9 +137,10 @@ type ScheduleConfig = Readonly<{ isHttps: boolean }>;
 
 export function scheduleClipboardTests({ isHttps }: ScheduleConfig) {
   const mockClipboardInstance = makeMockClipboard(isHttps);
-
   const originalNavigator = window.navigator;
-  beforeAll(() => {
+
+  beforeEach(() => {
+    jest.useFakeTimers();
     jest.spyOn(window, "navigator", "get").mockImplementation(() => ({
       ...originalNavigator,
       clipboard: mockClipboardInstance,
@@ -170,6 +171,7 @@ export function scheduleClipboardTests({ isHttps }: ScheduleConfig) {
   });
 
   afterEach(() => {
+    jest.useRealTimers();
     mockClipboardInstance.setMockText("");
     mockClipboardInstance.setSimulateFailure(false);
   });

site/src/pages/TerminalPage/TerminalAlerts.tsx

@@ -1,8 +1,66 @@
 import Button from "@mui/material/Button";
 import Link from "@mui/material/Link";
-import { type FC, useState } from "react";
+import { type FC, useState, useEffect, useRef } from "react";
+import type { WorkspaceAgent } from "api/typesGenerated";
 import { Alert, type AlertProps } from "components/Alert/Alert";
 import { docs } from "utils/docs";
+import type { ConnectionStatus } from "./types";
+
+type TerminalAlertsProps = {
+  agent: WorkspaceAgent | undefined;
+  status: ConnectionStatus;
+  onAlertChange: () => void;
+};
+
+export const TerminalAlerts = ({
+  agent,
+  status,
+  onAlertChange,
+}: TerminalAlertsProps) => {
+  const lifecycleState = agent?.lifecycle_state;
+  const prevLifecycleState = useRef(lifecycleState);
+  useEffect(() => {
+    prevLifecycleState.current = lifecycleState;
+  }, [lifecycleState]);
+
+  // We want to observe the children of the wrapper to detect when the alert
+  // changes. So the terminal page can resize itself.
+  //
+  // Would it be possible to just always call fit() when this component
+  // re-renders instead of using an observer?
+  //
+  // This is a good question and the why this does not work is that the .fit()
+  // needs to run after the render so in this case, I just think the mutation
+  // observer is more reliable. I could use some hacky setTimeout inside of
+  // useEffect to do that, I guess, but I don't think it would be any better.
+  const wrapperRef = useRef<HTMLDivElement>(null);
+  useEffect(() => {
+    if (!wrapperRef.current) {
+      return;
+    }
+    const observer = new MutationObserver(onAlertChange);
+    observer.observe(wrapperRef.current, { childList: true });
+
+    return () => {
+      observer.disconnect();
+    };
+  }, [onAlertChange]);
+
+  return (
+    <div ref={wrapperRef}>
+      {status === "disconnected" ? (
+        <DisconnectedAlert />
+      ) : lifecycleState === "start_error" ? (
+        <ErrorScriptAlert />
+      ) : lifecycleState === "starting" ? (
+        <LoadingScriptsAlert />
+      ) : lifecycleState === "ready" &&
+        prevLifecycleState.current === "starting" ? (
+        <LoadedScriptsAlert />
+      ) : null}
+    </div>
+  );
+};
 
 export const ErrorScriptAlert: FC = () => {
   return (

site/src/pages/TerminalPage/TerminalPage.test.tsx

@@ -8,27 +8,10 @@ import {
   MockWorkspace,
   MockWorkspaceAgent,
 } from "testHelpers/entities";
-import {
-  renderWithAuth,
-  waitForLoaderToBeRemoved,
-} from "testHelpers/renderHelpers";
+import { renderWithAuth } from "testHelpers/renderHelpers";
 import { server } from "testHelpers/server";
 import TerminalPage, { Language } from "./TerminalPage";
 
-Object.defineProperty(window, "matchMedia", {
-  writable: true,
-  value: jest.fn().mockImplementation((query) => ({
-    matches: false,
-    media: query,
-    onchange: null,
-    addListener: jest.fn(), // deprecated
-    removeListener: jest.fn(), // deprecated
-    addEventListener: jest.fn(),
-    removeEventListener: jest.fn(),
-    dispatchEvent: jest.fn(),
-  })),
-});
-
 const renderTerminal = async (
   route = `/${MockUser.username}/${MockWorkspace.name}/terminal`,
 ) => {
@@ -36,7 +19,16 @@ const renderTerminal = async (
     route,
     path: "/:username/:workspace/terminal",
   });
-  await waitForLoaderToBeRemoved();
+  await waitFor(() => {
+    // To avoid 'act' errors during testing, we ensure the component is
+    // completely rendered without any outstanding state updates. This is
+    // accomplished by incorporating a 'data-status' attribute into the
+    // component. We then observe this attribute for any changes, as we cannot
+    // rely on other screen elements to indicate completion.
+    const wrapper =
+      utils.container.querySelector<HTMLDivElement>("[data-status]")!;
+    expect(wrapper.dataset.state).not.toBe("initializing");
+  });
   return utils;
 };
 
@@ -58,11 +50,15 @@ const expectTerminalText = (container: HTMLElement, text: string) => {
 };
 
 describe("TerminalPage", () => {
+  afterEach(() => {
+    WS.clean();
+  });
+
   it("loads the right workspace data", async () => {
-    const spy = jest
+    jest
       .spyOn(API, "getWorkspaceByOwnerAndName")
       .mockResolvedValue(MockWorkspace);
-    const ws = new WS(
+    new WS(
       `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`,
     );
     await renderTerminal(
@@ -75,57 +71,45 @@ describe("TerminalPage", () => {
         { include_deleted: true },
       );
     });
-    spy.mockRestore();
-    ws.close();
   });
 
   it("shows an error if fetching workspace fails", async () => {
-    // Given
     server.use(
       http.get("/api/v2/users/:userId/workspace/:workspaceName", () => {
         return HttpResponse.json({ id: "workspace-id" }, { status: 500 });
       }),
     );
 
-    // When
     const { container } = await renderTerminal();
 
-    // Then
     await expectTerminalText(container, Language.workspaceErrorMessagePrefix);
   });
 
   it("shows an error if the websocket fails", async () => {
-    // Given
     server.use(
       http.get("/api/v2/workspaceagents/:agentId/pty", () => {
         return HttpResponse.json({}, { status: 500 });
       }),
     );
 
-    // When
     const { container } = await renderTerminal();
 
-    // Then
     await expectTerminalText(container, Language.websocketErrorMessagePrefix);
   });
 
   it("renders data from the backend", async () => {
-    // Given
     const ws = new WS(
       `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`,
     );
     const text = "something to render";
 
-    // When
     const { container } = await renderTerminal();
-
-    // Then
     // Ideally we could use ws.connected but that seems to pause React updates.
     // For now, wait for the initial resize message instead.
     await ws.nextMessage;
     ws.send(text);
+
     await expectTerminalText(container, text);
-    ws.close();
   });
 
   // Ideally we could just pass the correct size in the web socket URL without
@@ -134,40 +118,32 @@ describe("TerminalPage", () => {
   // in the other tests since ws.connected appears to pause React updates.  So
   // for now the initial resize message (and this test) are here to stay.
   it("resizes on connect", async () => {
-    // Given
     const ws = new WS(
       `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`,
     );
 
-    // When
     await renderTerminal();
 
-    // Then
     const msg = await ws.nextMessage;
     const req = JSON.parse(new TextDecoder().decode(msg as Uint8Array));
     expect(req.height).toBeGreaterThan(0);
     expect(req.width).toBeGreaterThan(0);
-    ws.close();
   });
 
   it("supports workspace.agent syntax", async () => {
-    // Given
     const ws = new WS(
       `ws://localhost/api/v2/workspaceagents/${MockWorkspaceAgent.id}/pty`,
     );
     const text = "something to render";
 
-    // When
     const { container } = await renderTerminal(
       `/some-user/${MockWorkspace.name}.${MockWorkspaceAgent.name}/terminal`,
     );
 
-    // Then
     // Ideally we could use ws.connected but that seems to pause React updates.
     // For now, wait for the initial resize message instead.
     await ws.nextMessage;
     ws.send(text);
     await expectTerminalText(container, text);
-    ws.close();
   });
 });

site/src/pages/TerminalPage/TerminalPage.tsx

@@ -13,7 +13,6 @@ import { WebLinksAddon } from "xterm-addon-web-links";
 import { WebglAddon } from "xterm-addon-webgl";
 import { deploymentConfig } from "api/queries/deployment";
 import { workspaceByOwnerAndName } from "api/queries/workspaces";
-import type { WorkspaceAgent } from "api/typesGenerated";
 import { useProxy } from "contexts/ProxyContext";
 import { ThemeOverride } from "contexts/ThemeProvider";
 import themes from "theme";
@@ -22,12 +21,8 @@ import { pageTitle } from "utils/page";
 import { openMaybePortForwardedURL } from "utils/portForward";
 import { terminalWebsocketUrl } from "utils/terminal";
 import { getMatchingAgentOrFirst } from "utils/workspace";
-import {
-  DisconnectedAlert,
-  ErrorScriptAlert,
-  LoadedScriptsAlert,
-  LoadingScriptsAlert,
-} from "./TerminalAlerts";
+import { TerminalAlerts } from "./TerminalAlerts";
+import type { ConnectionStatus } from "./types";
 
 export const Language = {
   workspaceErrorMessagePrefix: "Unable to fetch workspace: ",
@@ -35,8 +30,6 @@ export const Language = {
   websocketErrorMessagePrefix: "WebSocket failed: ",
 };
 
-type TerminalState = "connected" | "disconnected" | "initializing";
-
 const TerminalPage: FC = () => {
   // Maybe one day we'll support a light themed terminal, but terminal coloring
   // is notably a pain because of assumptions certain programs might make about your
@@ -46,10 +39,12 @@ const TerminalPage: FC = () => {
   const { proxy, proxyLatencies } = useProxy();
   const params = useParams() as { username: string; workspace: string };
   const username = params.username.replace("@", "");
-  const xtermRef = useRef<HTMLDivElement>(null);
-  const [terminal, setTerminal] = useState<XTerm.Terminal | null>(null);
-  const [terminalState, setTerminalState] =
-    useState<TerminalState>("initializing");
+  const terminalWrapperRef = useRef<HTMLDivElement>(null);
+  // The terminal is maintained as a state to trigger certain effects when it
+  // updates.
+  const [terminal, setTerminal] = useState<XTerm.Terminal>();
+  const [connectionStatus, setConnectionStatus] =
+    useState<ConnectionStatus>("initializing");
   const [searchParams] = useSearchParams();
   const isDebugging = searchParams.has("debug");
   // The reconnection token is a unique token that identifies
@@ -93,7 +88,7 @@ const TerminalPage: FC = () => {
   // Create the terminal!
   const fitAddonRef = useRef<FitAddon>();
   useEffect(() => {
-    if (!xtermRef.current || config.isLoading) {
+    if (!terminalWrapperRef.current || config.isLoading) {
       return;
     }
     const terminal = new XTerm.Terminal({
@@ -122,7 +117,7 @@ const TerminalPage: FC = () => {
       }),
     );
 
-    terminal.open(xtermRef.current);
+    terminal.open(terminalWrapperRef.current);
 
     // We have to fit twice here. It's unknown why, but the first fit will
     // overflow slightly in some scenarios. Applying a second fit resolves this.
@@ -140,7 +135,7 @@ const TerminalPage: FC = () => {
       window.removeEventListener("resize", listener);
       terminal.dispose();
     };
-  }, [theme, renderer, config.isLoading, xtermRef, handleWebLinkRef]);
+  }, [config.isLoading, renderer, theme.palette.background.default]);
 
   // Updates the reconnection token into the URL if necessary.
   useEffect(() => {
@@ -156,7 +151,7 @@ const TerminalPage: FC = () => {
         replace: true,
       },
     );
-  }, [searchParams, navigate, reconnectionToken]);
+  }, [navigate, reconnectionToken, searchParams]);
 
   // Hook up the terminal through a web socket.
   useEffect(() => {
@@ -182,12 +177,14 @@ const TerminalPage: FC = () => {
       terminal.writeln(
         Language.workspaceErrorMessagePrefix + workspace.error.message,
       );
+      setConnectionStatus("disconnected");
       return;
     } else if (!workspaceAgent) {
       terminal.writeln(
         Language.workspaceAgentErrorMessagePrefix +
           "no agent found with ID, is the workspace started?",
       );
+      setConnectionStatus("disconnected");
       return;
     }
 
@@ -243,18 +240,18 @@ const TerminalPage: FC = () => {
               }),
             ),
           );
-          setTerminalState("connected");
+          setConnectionStatus("connected");
         });
         websocket.addEventListener("error", () => {
           terminal.options.disableStdin = true;
           terminal.writeln(
             Language.websocketErrorMessagePrefix + "socket errored",
           );
-          setTerminalState("disconnected");
+          setConnectionStatus("disconnected");
         });
         websocket.addEventListener("close", () => {
           terminal.options.disableStdin = true;
-          setTerminalState("disconnected");
+          setConnectionStatus("disconnected");
         });
         websocket.addEventListener("message", (event) => {
           if (typeof event.data === "string") {
@@ -271,7 +268,7 @@ const TerminalPage: FC = () => {
           return; // Unmounted while we waited for the async call.
         }
         terminal.writeln(Language.websocketErrorMessagePrefix + error.message);
-        setTerminalState("disconnected");
+        setConnectionStatus("disconnected");
       });
 
     return () => {
@@ -284,8 +281,8 @@ const TerminalPage: FC = () => {
     proxy.preferredPathAppURL,
     reconnectionToken,
     terminal,
-    workspace.isLoading,
     workspace.error,
+    workspace.isLoading,
     workspaceAgent,
   ]);
 
@@ -300,15 +297,22 @@ const TerminalPage: FC = () => {
             : ""}
         </title>
       </Helmet>
-      <div css={{ display: "flex", flexDirection: "column", height: "100vh" }}>
+      <div
+        css={{ display: "flex", flexDirection: "column", height: "100vh" }}
+        data-status={connectionStatus}
+      >
         <TerminalAlerts
           agent={workspaceAgent}
-          state={terminalState}
+          status={connectionStatus}
           onAlertChange={() => {
             fitAddonRef.current?.fit();
           }}
         />
-        <div css={styles.terminal} ref={xtermRef} data-testid="terminal" />
+        <div
+          css={styles.terminal}
+          ref={terminalWrapperRef}
+          data-testid="terminal"
+        />
       </div>
 
       {latency && isDebugging && (
@@ -328,62 +332,6 @@ const TerminalPage: FC = () => {
   );
 };
 
-type TerminalAlertsProps = {
-  agent: WorkspaceAgent | undefined;
-  state: TerminalState;
-  onAlertChange: () => void;
-};
-
-const TerminalAlerts = ({
-  agent,
-  state,
-  onAlertChange,
-}: TerminalAlertsProps) => {
-  const lifecycleState = agent?.lifecycle_state;
-  const prevLifecycleState = useRef(lifecycleState);
-  useEffect(() => {
-    prevLifecycleState.current = lifecycleState;
-  }, [lifecycleState]);
-
-  // We want to observe the children of the wrapper to detect when the alert
-  // changes. So the terminal page can resize itself.
-  //
-  // Would it be possible to just always call fit() when this component
-  // re-renders instead of using an observer?
-  //
-  // This is a good question and the why this does not work is that the .fit()
-  // needs to run after the render so in this case, I just think the mutation
-  // observer is more reliable. I could use some hacky setTimeout inside of
-  // useEffect to do that, I guess, but I don't think it would be any better.
-  const wrapperRef = useRef<HTMLDivElement>(null);
-  useEffect(() => {
-    if (!wrapperRef.current) {
-      return;
-    }
-    const observer = new MutationObserver(onAlertChange);
-    observer.observe(wrapperRef.current, { childList: true });
-
-    return () => {
-      observer.disconnect();
-    };
-  }, [onAlertChange]);
-
-  return (
-    <div ref={wrapperRef}>
-      {state === "disconnected" ? (
-        <DisconnectedAlert />
-      ) : lifecycleState === "start_error" ? (
-        <ErrorScriptAlert />
-      ) : lifecycleState === "starting" ? (
-        <LoadingScriptsAlert />
-      ) : lifecycleState === "ready" &&
-        prevLifecycleState.current === "starting" ? (
-        <LoadedScriptsAlert />
-      ) : null}
-    </div>
-  );
-};
-
 const styles = {
   terminal: (theme) => ({
     width: "100%",

site/src/pages/TerminalPage/types.ts

@@ -0,0 +1 @@
+export type ConnectionStatus = "connected" | "disconnected" | "initializing";

site/src/testHelpers/renderHelpers.tsx

@@ -22,6 +22,14 @@ export function createTestQueryClient() {
   // Helps create one query client for each test case, to make sure that tests
   // are isolated and can't affect each other
   return new QueryClient({
+    logger: {
+      ...console,
+      // Some tests are designed to throw errors as part of their functionality.
+      // To avoid unnecessary noise from these expected errors, the code is
+      // structured to suppress them. If this suppression becomes problematic,
+      // the code can be refactored to handle query errors on a per-test basis.
+      error: () => {},
+    },
     defaultOptions: {
       queries: {
         retry: false,