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-7241-a03782846338f0883d0c680328b35d41d9a2f7de in repository https://gitbox.apache.org/repos/asf/texera.git
commit 1ad5163f83fe3d48b4451bf76f04ec578f252645 Author: Xinyuan Lin <[email protected]> AuthorDate: Sat Aug 1 02:08:03 2026 -0700 test(frontend): extend workspace mini-map, jupyter-panel and preset specs (#7241) ### What changes were proposed in this PR? Three existing specs under `frontend/src/app/workspace/`, extended to cover what their subjects had left untested: | File | Before | Tests now | |---|---|---| | `mini-map.component.ts` | 42.9% | 16 | | `preset.service.ts` | 81.0% | 55 | | `jupyter-panel.service.ts` | 82.2% | 41 | Two of these have traps that make a naive extension pass for the wrong reason, which is most of why they were left half-covered: **jsdom has no layout.** `src/jsdom-svg-polyfill.ts` stubs `getScreenCTM`/`getCTM` to an identity matrix and `getBBox` to a zero rect, and `offsetWidth`/`offsetHeight` are 0. So `ngAfterViewInit` computes `scale = 0`, and a geometry assertion reads `"0px"` — true because there is no layout, not because the formula is right. The mini-map specs use a stub paper with explicit, mutually distinguishable geometry plus `Object.defineProperty`'d offset dimensions, and every expected value is a non-zero literal (`200px`, `100px`, `37.5px`, `matrix(0.25,0,0,0.25,240,135)`), so a regression back to zero-layout fails. **`catchError` swallows the difference between two branches.** `fetchNotebookAndMapping` wraps its `switchMap` in `catchError(() => of(0))`. The base mock has no `sendNotebookToJupyter`, so flushing `{exists:true}` without defining it throws a `TypeError` that becomes `0` — indistinguishable from the send-failed branch under test. Every spec on that path defines the mock explicitly *and* asserts it was called; the reject case additionally asserts `console.error` was not called, which is what separates it from the `catchError` path. Also handled: Angular's automatic fixture teardown fires `ngOnDestroy` after every test, which writes `localStorage["mini-map"]`. The key is cleared before `TestBed.createComponent` and again in `afterEach`, so ordering cannot decide a result. Two things deliberately left uncovered, with the reasoning in the files: - `updatePreset`'s splice/replace branches. The method has **no production caller** and carries a real bug — lodash `indexOf` against a list that was just `JSON.parse`'d always returns `-1`, so `splice(-1, 1)` deletes the *last* preset instead of the intended one, and `presets[-1] = replacement` writes a non-index property that `JSON.stringify` silently drops. Its sibling `updateOrCreatePreset` already uses the correct `findIndex(p => isEqual(p, original))`. Testing it would cement the bug; it wants a separate fix-or-delete. - The two `no default save preset info/warning message` throws — pinning them would assert a limitation that adding a default message would fix. Assertion strength was checked by mutation and every mutation reverted; the production diff is empty. ### Any related issues, documentation, discussions? Closes #7238 ### How was this PR tested? ``` npx ng test --watch=false --include="**/mini-map.component.spec.ts" --include="**/jupyter-panel.service.spec.ts" --include="**/preset.service.spec.ts" ``` ``` ✓ src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts (41 tests) ✓ src/app/workspace/service/preset/preset.service.spec.ts (55 tests) ✓ src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts (16 tests) Test Files 3 passed (3) ``` `yarn format:ci` passes (prettier-eslint + eslint), which Vitest does not cover on its own. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../mini-map/mini-map.component.spec.ts | 358 ++++++++++++++++++++- .../jupyter-panel/jupyter-panel.service.spec.ts | 239 ++++++++++++++ .../service/preset/preset.service.spec.ts | 60 ++++ 3 files changed, 655 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts index 3686454ca6..2087b5e72b 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts @@ -19,18 +19,100 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { ReplaySubject } from "rxjs"; +import * as joint from "jointjs"; import { MiniMapComponent } from "./mini-map.component"; +import { MAIN_CANVAS } from "../workflow-editor.component"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; +import { JointGraphWrapper } from "../../../service/workflow-graph/model/joint-graph-wrapper"; import { OperatorMetadataService } from "../../../service/operator-metadata/operator-metadata.service"; import { StubOperatorMetadataService } from "../../../service/operator-metadata/stub-operator-metadata.service"; import { JointUIService } from "../../../service/joint-ui/joint-ui.service"; +import { PanelService } from "../../../service/panel/panel.service"; import { UndoRedoService } from "../../../service/undo-redo/undo-redo.service"; import { WorkflowUtilService } from "../../../service/workflow-graph/util/workflow-util.service"; import { DragDropModule } from "@angular/cdk/drag-drop"; import { commonTestProviders } from "../../../../common/testing/test-utils"; +/** + * Stand-in for the main workflow-editor paper the mini-map mirrors. + * + * jsdom has no layout engine: `src/jsdom-svg-polyfill.ts` stubs `getScreenCTM` + * / `getCTM` to the identity matrix and `getBBox` to a zero rect, so a real + * jointjs paper reports zero-sized geometry and every navigator assertion would + * read "0px" — passing because there is no layout rather than because the + * formula is right. This stub reports explicit, distinguishable geometry + * instead, and records the calls the component makes against it. + */ +class StubPaper { + public readonly handlers: Record<string, () => void> = {}; + public readonly pageToLocalPointArgs: { x: number; y: number }[] = []; + public readonly translateArgs: [number, number][] = []; + /** Local-coordinate point the next `pageToLocalPoint` call resolves to. */ + public localPoint = { x: 0, y: 0 }; + /** Current paper offset reported by the no-arg `translate()` getter. */ + public offset = { tx: 0, ty: 0 }; + + constructor( + private readonly sx: number = 1, + private readonly sy: number = 1 + ) {} + + on(event: string, handler: () => void): void { + this.handlers[event] = handler; + } + + scale(): { sx: number; sy: number } { + return { sx: this.sx, sy: this.sy }; + } + + pageToLocalPoint(point: { x: number; y: number }): { x: number; y: number } { + this.pageToLocalPointArgs.push(point); + return this.localPoint; + } + + translate(tx?: number, ty?: number): { tx: number; ty: number } | void { + if (tx === undefined) return this.offset; + this.translateArgs.push([tx, ty as number]); + } +} + +/** + * Regression coverage for the mini-map: the persisted show/hide flag, the + * navigator overlay geometry (which mirrors the main paper's viewport onto the + * mini-map), drag-to-pan, and the zoom/center toolbar buttons. + * + * Breakage this catches: dropping/renaming the "mini-map" localStorage key so + * the collapsed state no longer survives a reload; unsubscribing the mini-map + * from the main paper's translate/scale/resize events so the navigator freezes; + * sign or scale errors in the navigator-position and drag-to-pan formulas; + * losing the zoom-limit guards so the toolbar can push the zoom ratio past + * ZOOM_MINIMUM / ZOOM_MAXIMUM; and the panel service's close/reset streams no + * longer hiding/showing the mini-map. + */ describe("MiniMapComponent", () => { let fixture: ComponentFixture<MiniMapComponent>; + let component: MiniMapComponent; + let workflowActionService: WorkflowActionService; + let panelService: PanelService; + let editorStub: HTMLDivElement | undefined; + let mainPaper$: ReplaySubject<joint.dia.Paper>; + + /** + * `localStorage` is a jsdom global shared by every test in this file, and + * Angular's automatic fixture teardown runs `ngOnDestroy` — which writes + * `mini-map` — after each one. Seed and clear it explicitly so the persisted + * flag under test is the one this test wrote, not the previous test's. + */ + beforeEach(() => { + localStorage.removeItem("mini-map"); + }); + + afterEach(() => { + localStorage.removeItem("mini-map"); + editorStub?.remove(); + editorStub = undefined; + }); beforeEach(async () => { await TestBed.configureTestingModule({ @@ -49,13 +131,285 @@ describe("MiniMapComponent", () => { }).compileComponents(); }); + // The fixture is created but NOT change-detected here: ngAfterViewInit reads + // the mini-map container's size and the persisted flag, so each test sets its + // own environment up before triggering it. beforeEach(() => { fixture = TestBed.createComponent(MiniMapComponent); - TestBed.inject(WorkflowActionService); - fixture.detectChanges(); + component = fixture.componentInstance; + workflowActionService = TestBed.inject(WorkflowActionService); + panelService = TestBed.inject(PanelService); + + // Own the paper stream rather than casting the wrapper's getter back to a Subject. + // `getMainJointPaperAttachedStream()` is declared `Observable<Paper>`, so calling + // `.next()` on its result only works because it happens to be a ReplaySubject today; + // switching it to `.asObservable()` would break the spec silently. Stubbing the + // getter depends on the declared type only. ReplaySubject(1) so a paper attached + // before the component subscribes in ngAfterViewInit is still delivered. + mainPaper$ = new ReplaySubject<joint.dia.Paper>(1); + vi.spyOn(workflowActionService.getJointGraphWrapper(), "getMainJointPaperAttachedStream").mockReturnValue( + mainPaper$.asObservable() + ); }); + /** Gives the mini-map container a size, which jsdom otherwise reports as 0. */ + function sizeMiniMapContainer(width: number, height: number): HTMLElement { + const map = fixture.nativeElement.querySelector("#mini-map") as HTMLElement; + Object.defineProperty(map, "offsetWidth", { value: width, configurable: true }); + Object.defineProperty(map, "offsetHeight", { value: height, configurable: true }); + return map; + } + + /** + * The mini-map reads the main editor's element out of the document by id, so + * mount a stand-in with an explicit size and viewport rect. + */ + function mountWorkflowEditorStub(width: number, height: number, left: number, top: number): HTMLDivElement { + const editor = document.createElement("div"); + editor.id = "workflow-editor"; + Object.defineProperty(editor, "offsetWidth", { value: width, configurable: true }); + Object.defineProperty(editor, "offsetHeight", { value: height, configurable: true }); + editor.getBoundingClientRect = () => ({ left, top, right: left + width, bottom: top + height }) as DOMRect; + document.body.appendChild(editor); + editorStub = editor; + return editor; + } + + /** Publishes `paper` on the stream the mini-map subscribes to in ngAfterViewInit. */ + function attachMainPaper(paper: StubPaper): void { + mainPaper$.next(paper as unknown as joint.dia.Paper); + } + it("should create", () => { + fixture.detectChanges(); expect(fixture.componentInstance).toBeTruthy(); }); + + describe("mini-map paper", () => { + it("fits the whole main canvas into the mini-map container", () => { + // 912 / (2688 - -960) == 0.25; the height (100) is deliberately different + // so a width/height mix-up in the scale formula cannot pass. + const map = sizeMiniMapContainer(912, 100); + + fixture.detectChanges(); + + expect(MAIN_CANVAS.xMax - MAIN_CANVAS.xMin).toBe(3648); + expect(component.scale).toBe(0.25); + // The paper is sized to the container it renders into ... + expect(map.style.width).toBe("912px"); + expect(map.style.height).toBe("100px"); + // ... and shifted so the canvas' top-left corner (-960, -540) lands on the + // container's origin: 960 * 0.25 == 240, 540 * 0.25 == 135. + expect(map.querySelector("g.joint-layers")?.getAttribute("transform")).toBe("matrix(0.25,0,0,0.25,240,135)"); + }); + }); + + describe("persisted hidden state", () => { + it("starts visible when nothing has been persisted", () => { + fixture.detectChanges(); + expect(component.hidden).toBe(false); + }); + + it("restores the collapsed state persisted by the previous session", () => { + localStorage.setItem("mini-map", "true"); + + // `hidden` is assigned inside ngAfterViewInit, i.e. after the template + // that reads it has been checked, so the dev-mode verification pass would + // report NG0100 for a state the component legitimately restores. Skip it. + fixture.detectChanges(false); + + expect(component.hidden).toBe(true); + }); + + it("persists the collapsed state on destroy and restores it into a fresh instance", () => { + fixture.detectChanges(); + component.hidden = true; + + fixture.destroy(); + + // Round-trip through the real storage key: a rename on either the write + // or the read side breaks this. + expect(localStorage.getItem("mini-map")).toBe("true"); + const reopened = TestBed.createComponent(MiniMapComponent); + reopened.detectChanges(false); + expect(reopened.componentInstance.hidden).toBe(true); + }); + + it("persists the collapsed state when the window unloads", () => { + fixture.detectChanges(); + component.hidden = true; + + // @HostListener("window:beforeunload") — the tab can close without Angular + // ever destroying the component, so the flag has to be written here too. + window.dispatchEvent(new Event("beforeunload")); + + expect(localStorage.getItem("mini-map")).toBe("true"); + }); + }); + + describe("panel service integration", () => { + it("hides on closePanels and shows again on resetPanels", () => { + fixture.detectChanges(); + expect(component.hidden).toBe(false); + + panelService.closePanels(); + expect(component.hidden).toBe(true); + + panelService.resetPanels(); + expect(component.hidden).toBe(false); + }); + }); + + describe("navigator overlay", () => { + const MINI_MAP_SCALE = 0.25; + + /** + * Wires a stub main paper to a change-detected component and pins an + * explicit mini-map scale (ngAfterViewInit computes 0 under jsdom, which + * would make every "0px" assertion vacuously true). + */ + function attachStubbedViewport(): { paper: StubPaper; navigator: HTMLElement } { + mountWorkflowEditorStub(800, 600, 30, 40); + fixture.detectChanges(); + component.scale = MINI_MAP_SCALE; + + const navigator = document.getElementById("mini-map-navigator") as HTMLElement; + // cdkDrag leaves a transform behind after a drag; the component must clear + // it before writing left/top, or the two offsets would stack. + navigator.style.transform = "translate3d(11px, 13px, 0)"; + + // sx and sy differ so a width/height mix-up cannot pass. + const paper = new StubPaper(2, 4); + paper.localPoint = { x: -160, y: -140 }; + attachMainPaper(paper); + + return { paper, navigator }; + } + + it("positions and sizes the navigator from the main paper's viewport", () => { + const { paper, navigator } = attachStubbedViewport(); + + expect(component.paper).toBe(paper as unknown as joint.dia.Paper); + // The viewport origin is the editor's top-left corner in page coordinates. + expect(paper.pageToLocalPointArgs).toEqual([{ x: 30, y: 40 }]); + // (-160 - -960) * 0.25 and (-140 - -540) * 0.25 + expect(navigator.style.left).toBe("200px"); + expect(navigator.style.top).toBe("100px"); + // (800 / 2) * 0.25 and (600 / 4) * 0.25 + expect(navigator.style.width).toBe("100px"); + expect(navigator.style.height).toBe("37.5px"); + expect(navigator.style.transform).toBe(""); + }); + + it("repositions the navigator when the main paper translates, scales or resizes", () => { + const { paper, navigator } = attachStubbedViewport(); + expect(Object.keys(paper.handlers).sort()).toEqual(["resize", "scale", "translate"]); + + const movements: [string, number, string][] = [ + ["translate", -560, "100px"], + ["scale", -360, "150px"], + ["resize", -60, "225px"], + ]; + for (const [event, localX, expectedLeft] of movements) { + paper.localPoint = { x: localX, y: -140 }; + paper.handlers[event](); + expect(navigator.style.left).toBe(expectedLeft); + } + }); + + it("leaves the navigator alone while the user is dragging it", () => { + const { paper, navigator } = attachStubbedViewport(); + expect(navigator.style.left).toBe("200px"); + + // The drag itself is already moving the navigator; echoing the paper's + // translate back onto it would fight the pointer. + component.dragging = true; + paper.localPoint = { x: -560, y: -140 }; + paper.handlers["translate"](); + + expect(navigator.style.left).toBe("200px"); + }); + }); + + describe("drag to pan", () => { + it("pans the main paper opposite the pointer, in main-canvas units", () => { + fixture.detectChanges(); + const paper = new StubPaper(); + paper.offset = { tx: 100, ty: 50 }; + component.paper = paper as unknown as joint.dia.Paper; + component.scale = 0.25; + + component.onDrag({ event: { movementX: 10, movementY: -20 } }); + + // A pointer delta on the mini-map is worth 1/scale as much on the main + // canvas, and the paper moves against the pointer. + expect(paper.translateArgs).toEqual([[100 - 40, 50 + 80]]); + }); + }); + + describe("zoom buttons", () => { + let jointGraphWrapper: JointGraphWrapper; + + beforeEach(() => { + fixture.detectChanges(); + jointGraphWrapper = workflowActionService.getJointGraphWrapper(); + }); + + it("zooms out by one click step", () => { + jointGraphWrapper.setZoomProperty(1); + + component.onClickZoomOut(); + + expect(jointGraphWrapper.getZoomRatio()).toBeCloseTo(1 - JointGraphWrapper.ZOOM_CLICK_DIFF, 10); + expect(jointGraphWrapper.getZoomRatio()).toBeLessThan(1); + }); + + it("does not zoom out past the minimum ratio", () => { + jointGraphWrapper.setZoomProperty(JointGraphWrapper.ZOOM_MINIMUM); + const setZoomProperty = vi.spyOn(jointGraphWrapper, "setZoomProperty"); + + component.onClickZoomOut(); + + expect(setZoomProperty).not.toHaveBeenCalled(); + expect(jointGraphWrapper.getZoomRatio()).toBe(JointGraphWrapper.ZOOM_MINIMUM); + }); + + it("zooms in by one click step", () => { + jointGraphWrapper.setZoomProperty(1); + + component.onClickZoomIn(); + + expect(jointGraphWrapper.getZoomRatio()).toBeCloseTo(1 + JointGraphWrapper.ZOOM_CLICK_DIFF, 10); + expect(jointGraphWrapper.getZoomRatio()).toBeGreaterThan(1); + }); + + it("does not zoom in past the maximum ratio", () => { + jointGraphWrapper.setZoomProperty(JointGraphWrapper.ZOOM_MAXIMUM); + const setZoomProperty = vi.spyOn(jointGraphWrapper, "setZoomProperty"); + + component.onClickZoomIn(); + + expect(setZoomProperty).not.toHaveBeenCalled(); + expect(jointGraphWrapper.getZoomRatio()).toBe(JointGraphWrapper.ZOOM_MAXIMUM); + }); + }); + + describe("center button", () => { + it("broadcasts a center event and resets the navigator's drag offset", () => { + fixture.detectChanges(); + const centerEvents: void[] = []; + workflowActionService + .getTexeraGraph() + .getCenterEventStream() + .subscribe(event => centerEvents.push(event)); + const reset = vi.spyOn(component.navigatorDrag, "reset"); + + component.triggerCenter(); + + expect(centerEvents).toHaveLength(1); + // Without the reset the navigator keeps the cdkDrag transform from the + // previous drag and lands off-centre. + expect(reset).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index c60b06d368..43347dfdf1 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -26,6 +26,20 @@ import { NotebookMigrationService } from "../notebook-migration/notebook-migrati import { GuiConfigService } from "src/app/common/service/gui-config.service"; import { firstValueFrom, of, throwError } from "rxjs"; +/** + * Regression coverage for the Jupyter notebook panel service: the per-workflow + * notebook fetch, the cell-to-operator highlight index, the iframe message + * bridge, and the feature-flag gate. + * + * Breakage this catches: reporting a notebook as present when Jupyter refused + * to load it (the toolbar would offer to expand a panel that has nothing in + * it); caching a failed Jupyter-origin lookup, which leaves the bridge dead for + * the rest of the session; dropping the guards that keep an unsaved workflow + * (no wid) or a workflow with no stored mapping from being looked up or posted + * to; leaving a previous cell's highlights on the canvas when an unmapped cell + * is clicked; and losing the feature-flag early-return in the window message + * listener, which is installed unconditionally in the constructor. + */ describe("JupyterPanelService", () => { let service: JupyterPanelService; let httpMock: HttpTestingController; @@ -95,6 +109,9 @@ describe("JupyterPanelService", () => { afterEach(() => { httpMock.verify(); + // Several specs below silence console.error/warn to keep the failure paths + // quiet; console is a shared global, so put the originals back. + vi.restoreAllMocks(); }); // Panel visibility @@ -227,6 +244,46 @@ describe("JupyterPanelService", () => { expect(await resultPromise).toBe(0); }); + // The fetch pipeline has two distinct ways to yield 0 — Jupyter refusing the + // notebook, and the request itself failing — and the second one is a + // catchError that swallows *any* throw inside the switchMap. Pinning them + // apart (via sendNotebookToJupyter and the console.error the catchError + // emits) is what keeps a broken happy path from masquerading as "send + // failed": the base mock has no sendNotebookToJupyter, so a spec that forgets + // to define it gets a TypeError converted into the very same 0. + it("returns 0 when Jupyter rejects the notebook, without going through the error path", async () => { + mockNotebook.sendNotebookToJupyter = vi.fn().mockResolvedValue(0); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + const mapping = { cell_to_operator: { cell1: ["A"] }, operator_to_cell: {} }; + const notebook = { cells: [] }; + + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1, 1)); + httpMock + .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) + .flush({ exists: true, mapping, notebook }); + + expect(await resultPromise).toBe(0); + // The mapping is stored before the notebook is handed to Jupyter, ... + expect(mockNotebook.setMapping).toHaveBeenCalledWith("mapping_wid_1", mapping); + // ... and the 0 came from Jupyter's own answer, not from a thrown error. + expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook); + expect(consoleError).not.toHaveBeenCalled(); + }); + + it("returns 0 and logs when the fetch request fails", async () => { + mockNotebook.sendNotebookToJupyter = vi.fn().mockResolvedValue(1); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1, 1)); + httpMock + .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) + .flush("migration service down", { status: 500, statusText: "Server Error" }); + + expect(await resultPromise).toBe(0); + expect(consoleError).toHaveBeenCalled(); + expect(mockNotebook.sendNotebookToJupyter).not.toHaveBeenCalled(); + }); + // jupyterNotebookExists$ starts false and flips true once init()'s fetch finds // a notebook for the workflow; the toolbar's expand button binds to this. it("sets jupyterNotebookExists$ true after a workflow's notebook is fetched", async () => { @@ -244,6 +301,26 @@ describe("JupyterPanelService", () => { expect(states.at(-1)).toBe(true); // true once the notebook is found }); + // The stored notebook exists but Jupyter refuses it: the toolbar must not + // advertise a notebook the panel cannot actually show. + it("leaves the panel closed when the stored notebook cannot be loaded into Jupyter", async () => { + mockNotebook.sendNotebookToJupyter = vi.fn().mockResolvedValue(0); + const exists: boolean[] = []; + const visible: boolean[] = []; + service.jupyterNotebookExists$.subscribe(v => exists.push(v)); + service.jupyterNotebookPanelVisible$.subscribe(v => visible.push(v)); + + service.init(); + httpMock + .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) + .flush({ exists: true, mapping: { cell_to_operator: {}, operator_to_cell: {} }, notebook: {} }); + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalled(); + expect(exists.at(-1)).toBe(false); + expect(visible.at(-1)).toBe(false); + }); + // init(): subscribes to workflow changes, drops the stale mapping for the // current workflow, and fetches the incoming workflow's notebook + mapping. it("init subscribes, drops the stale mapping, and fetches for the new workflow", () => { @@ -311,6 +388,20 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.highlightLinks).toHaveBeenCalledWith(true, "link1"); }); + // A cell that isn't in the index still has to clear the previous cell's + // highlights, otherwise clicking an unmapped cell leaves the old selection on + // the canvas. + it("clears existing highlights and highlights nothing for an unmapped cell", () => { + (service as any).cellToHighlightMapping = { cell1: { components: ["op1"], edges: ["link1"] } }; + + (service as any).highlightFromCell("unmappedCell"); + + expect(mockWorkflow.unhighlightOperators).toHaveBeenCalledWith("A", "B"); + expect(mockWorkflow.unhighlightLinks).toHaveBeenCalledWith("L1"); + expect(mockWorkflow.highlightOperators).not.toHaveBeenCalled(); + expect(mockWorkflow.highlightLinks).not.toHaveBeenCalled(); + }); + // handleNotebookMessage must only act on cellClicked messages that come from // our own iframe (event.source) AND carry the Jupyter origin. it("handleNotebookMessage highlights only for messages from the iframe at the Jupyter origin", async () => { @@ -332,6 +423,27 @@ describe("JupyterPanelService", () => { expect(highlightSpy).toHaveBeenCalledWith("c1"); }); + // postMessage can deliver a payload-less event; destructuring it must not + // throw out of the (async, unawaited) listener. + it("handleNotebookMessage ignores a message that carries no payload", async () => { + const iframeWindow = {} as Window; + service.setIframeRef({ contentWindow: iframeWindow } as any); + const highlightSpy = vi.spyOn(service as any, "highlightFromCell").mockImplementation(() => {}); + + await (service as any).handleNotebookMessage({ + source: iframeWindow, + origin: "http://jupyter";, + data: undefined, + }); + + // The awaited call above is what proves the payload destructuring survived the + // missing data: drop the `event.data ?? {}` fallback and the async handler rejects, + // failing this test before either assertion runs. These two then pin that the + // guards still passed (source + origin are ours) and that no highlight was issued. + expect(mockNotebook.getJupyterURL).toHaveBeenCalled(); + expect(highlightSpy).not.toHaveBeenCalled(); + }); + // A workflow with operators but no links is valid; precompute must still // record each cell's components (with empty edges) so cell clicks highlight. it("precomputes component mappings even when the graph has no links", () => { @@ -376,6 +488,49 @@ describe("JupyterPanelService", () => { }); }); + // An unsaved workflow has no wid, so there is no mapping key to look up; the + // precompute must drop the stale index instead of querying "mapping_wid_undefined". + it("drops the highlight index without a lookup when the workflow has no wid", () => { + mockWorkflow.getWorkflow.mockReturnValue({ wid: undefined }); + (service as any).cellToHighlightMapping = { stale: { components: ["X"], edges: [] } }; + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + (service as any).precomputeHighlightMapping(); + + expect(mockNotebook.getMapping).not.toHaveBeenCalled(); + expect((service as any).cellToHighlightMapping).toEqual({}); + expect(consoleWarn).toHaveBeenCalled(); + }); + + // The notebook was found but its mapping was never stored locally: leave the + // index empty rather than dereferencing the missing mapping. + it("drops the highlight index when no mapping is stored for the workflow", () => { + mockNotebook.getMapping.mockReturnValue(undefined); + (service as any).cellToHighlightMapping = { stale: { components: ["X"], edges: [] } }; + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + (service as any).precomputeHighlightMapping(); + + expect(mockNotebook.getMapping).toHaveBeenCalledWith("mapping_wid_1"); + expect((service as any).cellToHighlightMapping).toEqual({}); + expect(consoleWarn).toHaveBeenCalled(); + }); + + // A cell mapped to nothing must index as an empty component list; a raw + // undefined would blow up the later `.length` check in highlightFromCell. + it("indexes a cell with no mapped operators as an empty selection", () => { + mockNotebook.getMapping.mockReturnValue({ + cell_to_operator: { emptyCell: null }, + operator_to_cell: {}, + }); + + (service as any).precomputeHighlightMapping(); + + expect((service as any).cellToHighlightMapping).toEqual({ + emptyCell: { components: [], edges: [] }, + }); + }); + // onWorkflowComponentClick it("should postMessage when mapping exists", async () => { const mockIframe = { @@ -437,6 +592,73 @@ describe("JupyterPanelService", () => { expect(mockNotebook.getJupyterURL).toHaveBeenCalledTimes(1); }); + it("does not postMessage for an unsaved workflow (undefined wid)", async () => { + const mockIframe = { contentWindow: { postMessage: vi.fn() } } as any; + service.setIframeRef(mockIframe); + mockWorkflow.getWorkflow.mockReturnValue({ wid: undefined }); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + await service.onWorkflowComponentClick("op1"); + + // Bailing before the lookup keeps a "mapping_wid_undefined" key from being read. + expect(mockNotebook.getMapping).not.toHaveBeenCalled(); + expect(mockIframe.contentWindow.postMessage).not.toHaveBeenCalled(); + expect(consoleError).toHaveBeenCalled(); + }); + + it("does not postMessage when the workflow has no stored mapping", async () => { + const mockIframe = { contentWindow: { postMessage: vi.fn() } } as any; + service.setIframeRef(mockIframe); + mockNotebook.getMapping.mockReturnValue(undefined); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + await service.onWorkflowComponentClick("op1"); + + expect(mockNotebook.getMapping).toHaveBeenCalledWith("mapping_wid_1"); + expect(mockIframe.contentWindow.postMessage).not.toHaveBeenCalled(); + expect(consoleError).toHaveBeenCalled(); + }); + + // The Jupyter pod may not be reachable yet when the first click lands. A + // failed lookup must NOT be cached, or the panel stays dead for the rest of + // the session. + it("retries the origin lookup after an unavailable Jupyter URL", async () => { + const mockIframe = { contentWindow: { postMessage: vi.fn() } } as any; + service.setIframeRef(mockIframe); + mockNotebook.getMapping.mockReturnValue({ + cell_to_operator: {}, + operator_to_cell: { op1: ["cell1"] }, + }); + mockNotebook.getJupyterURL.mockResolvedValueOnce(null); + + await service.onWorkflowComponentClick("op1"); + expect(mockIframe.contentWindow.postMessage).not.toHaveBeenCalled(); + + await service.onWorkflowComponentClick("op1"); + + expect(mockNotebook.getJupyterURL).toHaveBeenCalledTimes(2); + expect(mockIframe.contentWindow.postMessage).toHaveBeenCalledWith( + { action: "triggerCellClick", operators: ["cell1"] }, + "http://jupyter"; + ); + }); + + it("treats a malformed Jupyter URL as unavailable and keeps retrying", async () => { + const mockIframe = { contentWindow: { postMessage: vi.fn() } } as any; + service.setIframeRef(mockIframe); + mockNotebook.getMapping.mockReturnValue({ + cell_to_operator: {}, + operator_to_cell: { op1: ["cell1"] }, + }); + mockNotebook.getJupyterURL.mockResolvedValue("://not-a-url"); + + await service.onWorkflowComponentClick("op1"); + await service.onWorkflowComponentClick("op1"); + + expect(mockIframe.contentWindow.postMessage).not.toHaveBeenCalled(); + expect(mockNotebook.getJupyterURL).toHaveBeenCalledTimes(2); + }); + // Feature flag gate (defence in depth). With the flag off, init must not // subscribe to workflow changes, and onWorkflowComponentClick must not // postMessage to the iframe. The window message listener is installed in @@ -491,5 +713,22 @@ describe("JupyterPanelService", () => { await service.onWorkflowComponentClick("cell1"); expect(mockIframe.contentWindow.postMessage).not.toHaveBeenCalled(); }); + + it("handleNotebookMessage ignores a cellClicked message from our own iframe", async () => { + const iframeWindow = {} as Window; + service.setIframeRef({ contentWindow: iframeWindow } as any); + const highlightSpy = vi.spyOn(service as any, "highlightFromCell").mockImplementation(() => {}); + + await (service as any).handleNotebookMessage({ + source: iframeWindow, + origin: "http://jupyter";, + data: { action: "cellClicked", cellUUID: "c1" }, + }); + + expect(highlightSpy).not.toHaveBeenCalled(); + // The message carried our own source and origin, so it was the flag check + // that stopped it — the origin was never even resolved. + expect(mockNotebook.getJupyterURL).not.toHaveBeenCalled(); + }); }); }); diff --git a/frontend/src/app/workspace/service/preset/preset.service.spec.ts b/frontend/src/app/workspace/service/preset/preset.service.spec.ts index b71c976515..51a24132a2 100644 --- a/frontend/src/app/workspace/service/preset/preset.service.spec.ts +++ b/frontend/src/app/workspace/service/preset/preset.service.spec.ts @@ -42,6 +42,17 @@ if (!ajvInstance.getKeyword("enable-presets")) { ajvInstance.addKeyword({ keyword: "enable-presets", schemaType: "boolean" }); } +/** + * Regression coverage for PresetService: the dictionary-backed read/write of + * operator presets, the toast the user sees for each write, preset validation + * against the operator's 'enable-presets' schema, and the static schema helpers. + * + * Breakage this catches: losing the empty-list default so the very first preset + * saved for an operator type parses `null` and throws; routing a save/delete + * toast to the wrong NzMessageService severity, or dropping the caller's + * message so the user sees the generic default; and accepting an operator + * schema that has no `properties` at all instead of rejecting it. + */ describe("PresetService", () => { let userConfigStub: { fetchKey: ReturnType<typeof vi.fn>; @@ -163,6 +174,37 @@ describe("PresetService", () => { expect(messageStub.error).not.toHaveBeenCalled(); }); + it("routes an explicitly passed message to the NzMessageService method for its severity", () => { + const severities = ["error", "info", "success", "warning"] as const; + + for (const severity of severities) { + presetService.savePresets( + presetType, + presetTarget, + [{ presetProperty: "v1" }], + `saved as ${severity}`, + severity + ); + } + + for (const severity of severities) { + // exactly one toast per severity: a mis-wired case would show up here as + // a missing call on one method and a doubled call on another. + expect(messageStub[severity]).toHaveBeenCalledTimes(1); + expect(messageStub[severity]).toHaveBeenCalledWith(`saved as ${severity}`); + } + }); + + it("deletePreset reports the caller's message as an error toast", () => { + userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{ presetProperty: "v1" }, { presetProperty: "v2" }]))); + + // PresetWrapperComponent.deletePreset passes exactly this shape. + presetService.deletePreset(presetType, presetTarget, { presetProperty: "v1" }, "Deleted preset: v1", "error"); + + expect(messageStub.error).toHaveBeenCalledWith("Deleted preset: v1"); + expect(messageStub.success).not.toHaveBeenCalled(); + }); + it("createPreset appends to existing presets and writes back", () => { const existing: Preset[] = [{ presetProperty: "v1" }]; userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify(existing))); @@ -175,6 +217,16 @@ describe("PresetService", () => { ); }); + it("createPreset stores the first preset when the dictionary has no entry yet", () => { + // The very first save for an operator type: fetchKey resolves to null, and + // the missing entry has to read as an empty list rather than being parsed. + userConfigStub.fetchKey.mockReturnValue(of(null)); + + presetService.createPreset(presetType, presetTarget, { presetProperty: "v1" }); + + expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey, JSON.stringify([{ presetProperty: "v1" }])); + }); + it("createPreset does not write the preset back when it already exists", async () => { userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{ presetProperty: "v1" }]))); @@ -394,6 +446,14 @@ describe("PresetService", () => { ).toThrow(); }); + it("getOperatorPresetSchema throws when the operator schema omits properties entirely", () => { + // Distinct from the empty-`properties` case above, which fails the later + // "no preset properties" check instead. + expect(() => PresetService.getOperatorPresetSchema(<CustomJSONSchema7>{ type: "object" })).toThrow( + /has no properties$/ + ); + }); + it("getOperatorPresetSchema throws when no property is preset-enabled", () => { expect(() => PresetService.getOperatorPresetSchema(<CustomJSONSchema7>{
