(texera) 01/01: test(frontend): drive the virtual-environment socket (#7480)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7480-97cb8cf699e676e7d9ee1ca643da58d0b3bc8b84
in repository https://gitbox.apache.org/repos/asf/texera.git

commit 306944a3ecc3466cc514cd0d8c1f8711d0f4dbe0
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Sun Aug 9 18:17:13 2026 -0700

    test(frontend): drive the virtual-environment socket (#7480)
    
    ### What changes were proposed in this PR?
    
    The PVE block is the largest uncovered region in this component, and the
    existing suite stops exactly at its seams: it stubs `runPveWebSocket`
    and `deleteUserPackages` so it can assert name validation without
    opening a socket. These drive the other side of those seams.
    
    Adds 9 tests using a stand-in `WebSocket`, so `onmessage` and `onerror`
    can be fired by hand. Covered: the socket opening and locking the card,
    the name being trimmed before it reaches the URL, server lines appending
    to the pip output, the `__DONE__` sentinel closing the socket and
    clearing installing without printing itself, a dropped connection
    surfacing as output rather than a hang, a still-open socket being closed
    before another starts, and the create path chaining delete-then-install.
    
    **Verified by mutation**, all reverted (production diff empty):
    
    | Mutation | Result |
    |---|---|
    | name not trimmed for the socket URL | red |
    | previous socket left open | red |
    | card not locked while installing | red |
    | sentinel not recognised | red |
    | done leaves installing set | red |
    | done does not close the socket | red |
    | done skips the continuation | red |
    | error not surfaced in the output | red |
    | error leaves the card installing | red |
    
    The "done leaves installing set" mutation **survived a first pass**, and
    the reason is worth recording: the test let the real delete/install
    continuation run, which resets `isInstalling` downstream — so it was
    observing the continuation's state, not the sentinel branch's. Stubbing
    the continuation is what makes the test discriminate, and that is
    commented in the spec.
    
    No production file is touched.
    
    ### Any related issues, documentation, discussions?
    
    Closes #7477
    
    ### How was this PR tested?
    
    ```
    npx ng test --watch=false --include="**/computing-unit-selection.component.spec.ts"
    ```
    
    ```
     Test Files  1 passed (1)
          Tests  109 passed (109)
    ```
    
    9 new on top of the existing 100. `yarn format:ci` passes.
    
    ### Was this PR authored or co-authored using generative AI tooling?
    
    Generated-by: Claude Code (Opus 5)
    
    Co-authored-by: Meng Wang <me...@uci.edu>
---
 .../computing-unit-selection.component.spec.ts     | 175 +++++++++++++++++++++
 1 file changed, 175 insertions(+)

diff --git a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts
index 481f595dea..4d6777e0ff 100644
--- a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts
+++ b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts
@@ -666,6 +666,181 @@ describe("PowerButtonComponent", () => {
     });
   });
 
+  /**
+   * The PVE block is the largest uncovered region in the component, and the suite above stops
+   * exactly at its seams: it stubs out `runPveWebSocket` and `deleteUserPackages` so it can assert
+   * name validation without opening a socket. These drive the other side of those seams.
+   *
+   * The only environmental need is a stand-in for `WebSocket`, so `onmessage` / `onerror` can be
+   * fired by hand — jsdom has no WebSocket and the component never inspects anything but the
+   * handlers it assigns.
+   */
+  describe("the virtual-environment socket", () => {
+    /** The sockets opened during a test, newest last. */
+    let sockets: FakeSocket[];
+
+    class FakeSocket {
+      static opened: FakeSocket[] = [];
+      onmessage: ((e: { data: string }) => void) | null = null;
+      onerror: (() => void) | null = null;
+      close = vi.fn();
+      constructor(public url: string) {
+        FakeSocket.opened.push(this);
+      }
+      /** Delivers one server line to the component. */
+      say(data: string): void {
+        this.onmessage?.({ data });
+      }
+    }
+
+    /** Installs one unlocked environment and returns the component's view of it. */
+    function setPve(over: Record<string, unknown> = {}): void {
+      component.pves = [
+        {
+          name: "envone",
+          isLocked: false,
+          isInstalling: false,
+          userPackages: [],
+          newPackages: [],
+          deletingPackages: [],
+          pipOutput: "",
+          prettyPipOutput: "",
+          expanded: false,
+          ...over,
+        } as any,
+      ];
+    }
+
+    beforeEach(() => {
+      FakeSocket.opened = [];
+      sockets = FakeSocket.opened;
+      vi.stubGlobal("WebSocket", FakeSocket as unknown as typeof WebSocket);
+      vi.spyOn((component as any).notificationService, "error").mockImplementation(() => {});
+      // The socket URL is built from the selected unit's cuid, so a unit has to be selected.
+      component.selectedComputingUnit = makeComputingUnit({ name: "unit" });
+    });
+
+    afterEach(() => {
+      vi.unstubAllGlobals();
+    });
+
+    it("opens a socket for the environment and locks the card while it runs", () => {
+      setPve();
+
+      component.createVirtualEnvironment(0);
+
+      expect(sockets).toHaveLength(1);
+      expect(component.pves[0].isInstalling).toBe(true);
+      expect(component.pves[0].isLocked).toBe(true);
+      expect(component.pves[0].pipOutput).toContain("Creating virtual environment");
+    });
+
+    it("trims the environment name before it reaches the socket URL", () => {
+      // The name is user input from an inline editor; an untrimmed name produces a URL for an
+      // environment the backend does not have.
+      const urlSpy = vi.spyOn(TestBed.inject(WorkflowPveService), "getPveWebSocketUrl");
+      setPve({ name: "  spaced  " });
+
+      component.createVirtualEnvironment(0);
+
+      expect(component.pves[0].name).toBe("spaced");
+      expect(urlSpy).toHaveBeenCalledWith(expect.anything(), "spaced", "create", []);
+    });
+
+    it("appends each line the server sends to the pip output", () => {
+      setPve();
+      component.createVirtualEnvironment(0);
+
+      sockets[0].say("collecting numpy");
+      sockets[0].say("installed");
+
+      expect(component.pves[0].pipOutput).toContain("collecting numpy");
+      expect(component.pves[0].pipOutput).toContain("installed");
+      // Still running: the card stays locked until the sentinel arrives.
+      expect(component.pves[0].isInstalling).toBe(true);
+    });
+
+    it("closes the socket and unlocks installing on the __DONE__ sentinel", () => {
+      // Without this the spinner never stops and the card is stuck mid-install.
+      // The continuation is stubbed out deliberately: letting the real delete/install chain run
+      // would reset isInstalling downstream, so the assertion would pass even if this branch
+      // never cleared it.
+      vi.spyOn(component as any, "deleteUserPackages").mockImplementation(() => {});
+      setPve();
+      component.createVirtualEnvironment(0);
+
+      sockets[0].say("__DONE__");
+
+      expect(component.pves[0].isInstalling).toBe(false);
+      expect(component.pves[0].socket).toBeUndefined();
+      expect(sockets[0].close).toHaveBeenCalled();
+    });
+
+    it("does not print the sentinel as though it were output", () => {
+      setPve();
+      component.createVirtualEnvironment(0);
+
+      sockets[0].say("__DONE__");
+
+      expect(component.pves[0].pipOutput).not.toContain("__DONE__");
+    });
+
+    it("reports a dropped connection in the output rather than hanging", () => {
+      setPve();
+      component.createVirtualEnvironment(0);
+
+      sockets[0].onerror?.();
+
+      expect(component.pves[0].pipOutput).toContain("[WebSocket error]");
+      expect(component.pves[0].isInstalling).toBe(false);
+      expect(component.pves[0].socket).toBeUndefined();
+    });
+
+    it("closes any socket still open on the card before starting another", () => {
+      // Re-running a create on a card that is already streaming would otherwise leave the first
+      // socket writing into the same pipOutput.
+      setPve();
+      component.createVirtualEnvironment(0);
+      const first = sockets[0];
+
+      component.pves[0] = { ...component.pves[0], isLocked: false } as any;
+      component.createVirtualEnvironment(0);
+
+      expect(first.close).toHaveBeenCalled();
+      expect(sockets).toHaveLength(2);
+    });
+
+    it("reinstalls the recorded packages once the environment is created", () => {
+      // The create socket's completion chains delete-then-install, which is how a rebuilt
+      // environment gets its packages back.
+      const deleteSpy = vi
+        .spyOn(component as any, "deleteUserPackages")
+        .mockImplementation((...args: unknown[]) => (args[1] as (() => void) | undefined)?.());
+      const installSpy = vi.spyOn(component as any, "installUserPackages").mockImplementation(() => {});
+      setPve();
+
+      component.createVirtualEnvironment(0);
+      sockets[0].say("__DONE__");
+
+      expect(deleteSpy).toHaveBeenCalled();
+      expect(installSpy).toHaveBeenCalled();
+    });
+
+    it("skips creation entirely for a locked card and only syncs its packages", () => {
+      const deleteSpy = vi
+        .spyOn(component as any, "deleteUserPackages")
+        .mockImplementation((...args: unknown[]) => (args[1] as (() => void) | undefined)?.());
+      const installSpy = vi.spyOn(component as any, "installUserPackages").mockImplementation(() => {});
+      setPve({ isLocked: true });
+
+      component.createVirtualEnvironment(0);
+
+      expect(sockets).toHaveLength(0);
+      expect(deleteSpy).toHaveBeenCalled();
+      expect(installSpy).toHaveBeenCalled();
+    });
+  });
+
   describe("createVirtualEnvironment name validation", () => {
     const VALIDATION_MSG = "Environment name must contain only letters and numbers.";