This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/texera.git
commit bf1a4e7453c22169b0d89a8c6f51c2583381e889 Author: Xinyuan Lin <[email protected]> AuthorDate: Sun Aug 16 01:24:13 2026 +0000 test(frontend): cover the websocket handshake, port editor binding and quota charts (#7696) ### What changes were proposed in this PR? Three frontend files that sat below their neighbours, all with untested behaviour rather than tooling problems (these are `.ts` files, so #7458's template attribution does not apply). | File | Before | After | |---|---|---| | `user-quota.component.ts` | 90.1% | **140/140 lines, 34/34 branches, 36/36 functions** | | `port-property-edit-frame.component.ts` | 86.5% | **72/74 (97.3%)** | | `workflow-websocket.service.ts` | 76.0% | **45/48 (93.8%)**, 15/15 branches | Tests **47 -> 70**. Covered: the handshake URL's query construction and the heartbeat interval; the Yjs/Quill shared binding, its awareness and cursors module, and the three DOM triggers that drive it; and the quota component's per-day aggregation, its chart wiring, and the accumulator reset that makes a reload idempotent. ### Verification The build applied 26 mutations. Review then proposed 23 more, and **every one of the 23 was real -- none was refutable.** 33 distinct mutations were run in total (~54 runs), one at a time, anchor uniqueness asserted, reverted between each, with `git diff` on the production files confirmed empty after every revert. The recurring failure was **degenerate fixtures**, where two different inputs produce identical output so a swap cannot be seen: | Surviving mutation | Why nothing noticed | |---|---| | chart the workflow series into `datasetLineChart` | both divs received identical-looking data | | exchange the two chart div ids | same | | `sizePieChart` -> `datasetLineChart`, and `dataset.size` -> `dataset.did` | the pie series was unpinned entirely | | `getSharedModelAwareness()` -> `undefined`, and `cursors: true` -> `false` | nothing read the binding's arguments back | | the descriptor read for a different port id | only one port existed in the fixture | | `(click)` loses `connectQuillToText()`; `(focusout)` and `(keyup.enter)` dropped | handlers were called directly, never through the DOM | **One error I caught in my own repair, and only because mutations were run individually.** The first idempotence test asserted `workflows.length`, which is *unchanged* when `this.workflows = []` is dropped -- executions simply get re-filed under the existing panels. Running the two resets as separate mutations exposed it; the test now pins the per-workflow execution ids. ### Deliberately not included Two defects are reported rather than pinned, so neither is cemented as a contract: - **The handshake drops `cuid` on a live path.** `WorkflowWebsocketResource.myOnOpen` does `session.getRequestParameterMap.get("cuid").get(0).toInt` with no `Option` guard, while `admin-execution.component.ts:327, :340, :353` all call `socket.openWebsocket(wid)` with no computing-unit id -- so admin kill/pause/resume dial a URL the server cannot accept. The test that observes today's URL is named descriptively rather than approvingly and carries a note pointing at the gap. - **`sortBySize` is inverted relative to the `NzTableSortFn` contract.** `user-quota.component.ts:344-345` returns `b - a`, but ng-zorro applies `sortOrder === 'ascend' ? compareResult : -compareResult` and `nzSortDirections` defaults to `['ascend','descend',null]`, which the `<th>` does not override -- so the first click lights the up-caret while rendering largest-first. `admin-user.component.ts:268` uses the contract-correct form, so this is not a house convention. The test clicks twice and asserts row order *and* which caret is active in both directions, with a comment stating the mismatch. No production file is touched. ### Any related issues, documentation, discussions? Closes #7695 ### How was this PR tested? ``` npx ng test --watch=false --include="**/user-quota.component.spec.ts" --include="**/port-property-edit-frame.component.spec.ts" --include="**/workflow-websocket.service.spec.ts" ``` ``` Test Files 3 passed (3) Tests 70 passed (70) ``` Coverage measured with `--coverage` on the same run. `yarn format:ci` passes. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../user/user-quota/user-quota.component.spec.ts | 295 ++++++++++++++++++++- .../port-property-edit-frame.component.spec.ts | 122 +++++++++ .../workflow-websocket.service.spec.ts | 189 ++++++++++++- 3 files changed, 601 insertions(+), 5 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts b/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts index e1e3dc92f4..84ea37a482 100644 --- a/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-quota/user-quota.component.spec.ts @@ -25,8 +25,10 @@ import { commonTestProviders } from "../../../../common/testing/test-utils"; import { of } from "rxjs"; import { By } from "@angular/platform-browser"; import type { Mocked } from "vitest"; -import { ExecutionQuota, WorkflowQuota } from "../../../../common/type/user"; +import { ExecutionQuota, Workflow, WorkflowQuota } from "../../../../common/type/user"; import { DatasetQuota } from "../../../type/quota-statistic.interface"; +import { AdminUserService } from "../../../service/admin/user/admin-user.service"; +import { NZ_MODAL_DATA } from "ng-zorro-antd/modal"; // Real Plotly renders into a DOM element by id; create one and assert on the // `data`/`layout` Plotly attaches to that graph div. (Module-level mocking of @@ -70,6 +72,13 @@ function execution(eid: number, workflowId: number, result: number, runtime: num const sumValues = (data: Array<[string, number]>): number => data.reduce((acc, [, v]) => acc + v, 0); +// Fixtures for the Cache Size comparator. The three byte triples are picked so that ordering by +// any single count — or by the sum with any one of the three terms dropped — yields a different +// sequence than the full sum does, and so that no two of the six orderings share a tie. +const SIZE_SORT_BIG = execution(30, 1, 1, 450, 452); // 903 +const SIZE_SORT_MIDDLE = execution(10, 1, 800, 1, 1); // 802 +const SIZE_SORT_SMALL = execution(20, 1, 2, 700, 2); // 704 + describe("UserQuotaComponent", () => { let component: UserQuotaComponent; let fixture: ComponentFixture<UserQuotaComponent>; @@ -104,6 +113,29 @@ describe("UserQuotaComponent", () => { expect(component).toBeTruthy(); }); + /** + * The mirror of the modal test at the bottom of this file. The constructor's two branches differ + * in exactly four things — the user id, the two header colours and the fixed height — and only the + * modal half of each was pinned, so the inline page could be made to look like the modal (or to + * read someone else's quota) without a failure. + */ + it("paints the standalone page header and reads the signed-in user's own quota", () => { + fixture.detectChanges(); + + expect(component.userId).toBe(-1); // the sentinel that means "whoever is logged in" + expect(mockUserQuotaService.getExecutionQuota).toHaveBeenCalledWith(-1); + expect(component.backgroundColor).toBe("white"); + expect(component.textColor).toBe("Black"); + expect(component.dynamicHeight).toBe(""); // the modal's fixed height must not apply inline + + const card = fixture.nativeElement.querySelector("nz-card") as HTMLElement; + expect(card.style.background).toBe("white"); + const heading = fixture.nativeElement.querySelector("h2.page-title") as HTMLElement; + expect(heading.style.color.toLowerCase()).toBe("black"); + const scroller = fixture.nativeElement.querySelector("div") as HTMLElement; + expect(scroller.style.height).toBe(""); + }); + describe("aggregateByMonth", () => { it("sums the values that share a 'YYYY-MM' prefix", () => { const result = component.aggregateByMonth([ @@ -167,12 +199,14 @@ describe("UserQuotaComponent", () => { }); describe("refreshData", () => { - it("loads datasets and executions, and groups executions by workflow", () => { + it("loads datasets, shared workflows and executions, and groups executions by workflow", () => { const datasets: DatasetQuota[] = [ { did: 1, name: "d1", creationTime: Date.now(), size: 100 }, { did: 2, name: "d2", creationTime: Date.now(), size: 200 }, ]; + const accessWorkflows = [7, 8, 9]; // three ids, so a dropped assignment cannot look like an empty load mockUserQuotaService.getCreatedDatasets.mockReturnValue(of(datasets)); + mockUserQuotaService.getAccessWorkflows.mockReturnValue(of(accessWorkflows)); mockUserQuotaService.getExecutionQuota.mockReturnValue( of([execution(10, 1, 100, 5, 10), execution(11, 1, 50, 5, 5), execution(12, 2, 20, 2, 3)]) ); @@ -185,11 +219,101 @@ describe("UserQuotaComponent", () => { expect(component.datasetList).toEqual(datasets); expect(component.totalUploadedDatasetCount).toBe(2); expect(component.totalUploadedDatasetSize).toBe(300); + // the "Workflows with Access" box counts these, so losing the assignment silently zeroes it + expect(component.accessWorkflows).toEqual(accessWorkflows); expect(component.totalQuotaSize).toBe(200); // 115 + 60 + 25 expect(component.workflows.map(w => w.workflowId)).toEqual([1, 2]); expect(component.workflows[0].executions.map(e => e.eid)).toEqual([10, 11]); expect(component.workflows[1].executions.map(e => e.eid)).toEqual([12]); }); + + it("counts the created datasets and workflows per calendar day, and charts each into its own div", () => { + // Derived from the same Date arithmetic the component uses, so the fixture holds in + // any timezone rather than only in UTC. + const recent = Date.now(); + const older = recent - 40 * 24 * 60 * 60 * 1000; + const recentDay = new Date(recent).toLocaleDateString(); + const olderDay = new Date(older).toLocaleDateString(); + expect(recentDay).not.toBe(olderDay); // the fixture has to straddle two days to be meaningful + + const createdWorkflow = (workflowId: number, creationTime: number): Workflow => ({ + userId: 1, + workflowId, + workflowName: `wf-${workflowId}`, + creationTime, + lastModifiedTime: creationTime, + }); + // The dataset and workflow subscribes each run their own copy of the same bucketing loop, so + // both need data — and the two tallies are deliberately *different* ([1, 2] vs [2, 1]) so that + // routing one series into the other's chart cannot masquerade as the right answer. + mockUserQuotaService.getCreatedDatasets.mockReturnValue( + of([ + { did: 1, name: "d1", creationTime: recent, size: 100 }, + { did: 2, name: "d2", creationTime: older, size: 200 }, + { did: 3, name: "d3", creationTime: older, size: 300 }, + ]) + ); + mockUserQuotaService.getCreatedWorkflows.mockReturnValue( + of([createdWorkflow(1, recent), createdWorkflow(2, recent), createdWorkflow(3, older)]) + ); + // Assert on what the per-day tally hands to the aggregator: that is the raw output of the + // bucketing loop, unaffected by the grouping/filtering stage tested above. + const aggregateSpy = vi.spyOn(component, "aggregateData"); + const pieChartSpy = vi.spyOn(component, "generatePieChart").mockImplementation(() => {}); + const lineChartSpy = vi.spyOn(component, "generateLineChart").mockImplementation(() => {}); + + component.refreshData(); + + expect(aggregateSpy.mock.calls[0][0]).toEqual([ + [recentDay, 1], + [olderDay, 2], // two datasets share a day and are tallied together + ]); + expect(aggregateSpy.mock.calls[1][0]).toEqual([ + [recentDay, 2], // two workflows share a day and are tallied together + [olderDay, 1], + ]); + + // The pie series is built in the same loop: name/size pairs, in that order. The three names and + // the three sizes are all distinct, so a slot swap cannot come out looking the same. + expect(pieChartSpy.mock.calls[0]).toEqual([ + [ + ["d1", 100], + ["d2", 200], + ["d3", 300], + ], + "Dataset Size Distribution", + "sizePieChart", + ]); + + // Each series has to reach its own graph div: the last argument selects the DOM node the chart + // is painted into, so getting it wrong blanks one chart and draws twice over the other. + expect(lineChartSpy.mock.calls[0][3]).toBe("Dataset Upload Overview"); + expect(lineChartSpy.mock.calls[0][4]).toBe("datasetLineChart"); + expect(lineChartSpy.mock.calls[1][3]).toBe("Workflow Upload Overview"); + expect(lineChartSpy.mock.calls[1][4]).toBe("workflowLineChart"); + }); + + it("clears the running totals so a reload does not double-count them", () => { + // refreshData is the reload path, so its resets only do work on the second call — without them + // the quota total and the workflow panels accumulate every time the page refreshes. + mockUserQuotaService.getExecutionQuota.mockReturnValue( + of([execution(10, 1, 100, 5, 10), execution(12, 2, 20, 2, 3)]) + ); + vi.spyOn(component, "generatePieChart").mockImplementation(() => {}); + vi.spyOn(component, "generateLineChart").mockImplementation(() => {}); + + component.refreshData(); + const quotaAfterFirst = component.totalQuotaSize; + const executionsAfterFirst = component.workflows.map(w => w.executions.map(e => e.eid)); + component.refreshData(); + + expect(quotaAfterFirst).toBe(140); // 115 + 25 + // Not just the panel count: dropping the `workflows = []` reset keeps the panel count at two + // and instead files every execution a second time under the workflow it already belongs to. + expect(executionsAfterFirst).toEqual([[10], [12]]); + expect(component.totalQuotaSize).toBe(quotaAfterFirst); + expect(component.workflows.map(w => w.executions.map(e => e.eid))).toEqual(executionsAfterFirst); + }); }); describe("deleteCollection", () => { @@ -216,6 +340,40 @@ describe("UserQuotaComponent", () => { expect(component.workflows).toEqual([]); }); + + it("skips the workflows that do not own the deleted execution and still prunes the emptied one", () => { + component.workflows = [ + { workflowId: 1, workflowName: "wf-1", executions: [execution(10, 1, 100, 5, 10)] }, + { workflowId: 2, workflowName: "wf-2", executions: [execution(20, 2, 7, 3, 1), execution(21, 2, 4, 2, 1)] }, + ]; + component.totalQuotaSize = 133; // 115 for wf-1, 11 + 7 for wf-2 + mockUserQuotaService.deleteExecutionCollection.mockReturnValue(of(undefined)); + + component.deleteCollection(10); + + // wf-2 owns nothing with this id, so it must be stepped over rather than reached into — + // and the sweep has to survive that far for the now-empty wf-1 to be pruned at all. + expect(component.workflows.map(w => w.workflowId)).toEqual([2]); + expect(component.workflows[0].executions.map(e => e.eid)).toEqual([20, 21]); + // wf-2's bytes differ from the deleted execution's, so a wrong subtrahend lands elsewhere. + expect(component.totalQuotaSize).toBe(18); + }); + }); + + /** + * The Cache Size column sorts on the sum of the three byte counts. The byte triples below are + * picked so that ordering by any single count — or by the sum with any one term dropped — + * produces a different sequence than the full sum does. + */ + describe("sortBySize", () => { + // Largest-first is what the comparator returns today. Note that this is inverted relative to the + // NzTableSortFn contract (`a - b`, which nz-table negates for 'descend'); see the header-click + // test below, which pins the resulting caret/order mismatch as the user actually sees it. + it("orders executions by total cache size, largest first", () => { + const scrambled = [SIZE_SORT_MIDDLE, SIZE_SORT_SMALL, SIZE_SORT_BIG]; + + expect([...scrambled].sort(component.sortBySize).map(e => e.eid)).toEqual([30, 10, 20]); + }); }); describe("chart generation", () => { @@ -257,6 +415,32 @@ describe("UserQuotaComponent", () => { expect(gd.layout.title).toMatchObject({ text: "Line Title" }); expect(gd.layout.xaxis.title).toMatchObject({ text: "X Label" }); expect(gd.layout.yaxis.title).toMatchObject({ text: "Y Label" }); + // y spans 3 - 1 = 2, within the narrow-range threshold, so the axis is forced onto whole + // numbers rather than letting Plotly pick fractional ticks for a series of counts. + expect(gd.layout.yaxis.tickmode).toBe("linear"); + expect(gd.layout.yaxis.dtick).toBe(1); + }); + + // The other leg of the same pair. On its own an "is undefined" assertion would also hold if the + // two keys were deleted outright; it is the whole-number assertion in the test above that makes + // this one mean "the threshold decides", rather than "the keys are never set". + it("generateLineChart leaves the y-axis tick density to Plotly when the series spans more than five", () => { + chartDiv("wideLineDiv"); + + component.generateLineChart( + [ + ["2024-01-01", 1], + ["2024-01-02", 20], + ], + "X Label", + "Y Label", + "Line Title", + "wideLineDiv" + ); + + const gd = document.getElementById("wideLineDiv") as unknown as { data: any[]; layout: any }; + expect(gd.layout.yaxis.tickmode).toBeUndefined(); + expect(gd.layout.yaxis.dtick).toBeUndefined(); }); }); /** @@ -351,5 +535,112 @@ describe("UserQuotaComponent", () => { expect(host.querySelectorAll("tbody tr").length).toBe(3); }); + + it("reorders the rows by total cache size when the Cache Size header is used", async () => { + // ngOnInit charts before the tab panes exist, so real Plotly rejects with "No DOM element + // with id ...". The synchronous tests above never give Node a turn to report those + // rejections; this one awaits a macrotask, so stub the charts out of the way. + vi.spyOn(component, "generatePieChart").mockImplementation(() => {}); + vi.spyOn(component, "generateLineChart").mockImplementation(() => {}); + const host = renderCache([workflow(1, [SIZE_SORT_MIDDLE, SIZE_SORT_SMALL, SIZE_SORT_BIG])]); + const eids = () => + Array.from(host.querySelectorAll("tbody tr")).map(r => r.querySelectorAll("td")[1]?.textContent?.trim()); + expect(eids()).toEqual(["10", "20", "30"]); // as supplied, before any sort + + const sizeHeader = Array.from(host.querySelectorAll<HTMLElement>("thead th")).find(h => + (h.textContent || "").includes("Cache Size") + )!; + const caretActive = (direction: "up" | "down") => + sizeHeader.querySelector(`.ant-table-column-sorter-${direction}`)!.classList.contains("active"); + const clickSort = async () => { + sizeHeader.click(); + // nz-table publishes a changed sort operator on a macrotask (`delay(0)`). + await new Promise(resolve => setTimeout(resolve, 0)); + fixture.detectChanges(); + }; + + await clickSort(); + // NOTE: the direction indicator and the row order disagree, and that is recorded here rather + // than blessed. nzSortDirections defaults to ['ascend', 'descend', null], so the first click + // selects 'ascend' and lights the up caret — but the rows come out LARGEST first, because + // sortBySize returns `b - a` while NzTableSortFn expects `a - b` (nz-table negates the result + // itself for 'descend'). Correcting the comparator is meant to flip both halves below. + expect(caretActive("up")).toBe(true); + expect(caretActive("down")).toBe(false); + expect(eids()).toEqual(["30", "10", "20"]); + + await clickSort(); + expect(caretActive("down")).toBe(true); + expect(caretActive("up")).toBe(false); + expect(eids()).toEqual(["20", "10", "30"]); + }); + }); +}); + +/** + * `admin-user.component` opens this same component inside a modal and hands it the target user + * through NZ_MODAL_DATA. That constructor branch needs the token provided, and the suite above + * has already instantiated its test module, so it gets its own TestBed here. + */ +describe("UserQuotaComponent (opened from the admin user modal)", () => { + const MODAL_UID = 42; + let fixture: ComponentFixture<UserQuotaComponent>; + let component: UserQuotaComponent; + let adminService: Mocked<AdminUserService>; + let regularService: Mocked<UserQuotaService>; + + function emptyQuotaService<T>(): Mocked<T> { + return { + getCreatedDatasets: vi.fn().mockReturnValue(of([])), + getCreatedWorkflows: vi.fn().mockReturnValue(of([])), + getAccessWorkflows: vi.fn().mockReturnValue(of([])), + getExecutionQuota: vi.fn().mockReturnValue(of([])), + deleteExecutionCollection: vi.fn().mockReturnValue(of(undefined)), + } as unknown as Mocked<T>; + } + + beforeEach(() => { + adminService = emptyQuotaService<AdminUserService>(); + regularService = emptyQuotaService<UserQuotaService>(); + + TestBed.configureTestingModule({ + providers: [ + { provide: AdminUserService, useValue: adminService }, + { provide: UserQuotaService, useValue: regularService }, + { provide: NZ_MODAL_DATA, useValue: { uid: MODAL_UID } }, + ...commonTestProviders, + ], + imports: [UserQuotaComponent, HttpClientTestingModule], + }); + + fixture = TestBed.createComponent(UserQuotaComponent); + component = fixture.componentInstance; + }); + + afterEach(() => vi.restoreAllMocks()); + + it("reads the quota of the user named by the modal, through the admin service", () => { + fixture.detectChanges(); + + expect(component.userId).toBe(MODAL_UID); + expect(adminService.getExecutionQuota).toHaveBeenCalledWith(MODAL_UID); + expect(adminService.getCreatedWorkflows).toHaveBeenCalledWith(MODAL_UID); + expect(regularService.getExecutionQuota).not.toHaveBeenCalled(); + }); + + it("paints the modal header instead of the standalone page header", () => { + fixture.detectChanges(); + + // The field initializers are "white"/"Black", so both of these differ from the inline mode. + expect(component.backgroundColor).toBe("lightcoral"); + expect(component.textColor).toBe("white"); + // The third difference: the modal keeps the fixed height the inline page clears. + expect(component.dynamicHeight).toBe("700px"); + const card = fixture.nativeElement.querySelector("nz-card") as HTMLElement; + expect(card.style.background).toBe("lightcoral"); + const heading = fixture.nativeElement.querySelector("h2.page-title") as HTMLElement; + expect(heading.style.color).toBe("white"); + const scroller = fixture.nativeElement.querySelector("div") as HTMLElement; + expect(scroller.style.height).toBe("700px"); }); }); diff --git a/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts index 8c6528397f..d12b316d33 100644 --- a/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/port-property-edit-frame/port-property-edit-frame.component.spec.ts @@ -33,6 +33,7 @@ import { FormlyNgZorroAntdModule } from "@ngx-formly/ng-zorro-antd"; import { LogicalPort, PortDescription } from "../../../types/workflow-common.interface"; import { mockPortSchema } from "../../../service/operator-metadata/mock-operator-metadata.data"; import { FORM_DEBOUNCE_TIME_MS } from "../../../service/execute-workflow/execute-workflow.service"; +import * as Y from "yjs"; describe("PortPropertyEditFrameComponent", () => { let component: PortPropertyEditFrameComponent; @@ -386,6 +387,21 @@ describe("PortPropertyEditFrameComponent", () => { expect(component.formTitle).toBe("old"); }); + + // The operator matches here, so the port half of the condition is the one doing the work — + // the test above short-circuits on the operator and never evaluates it. + it("should leave the form title unchanged for a sibling port of the same operator", () => { + component.currentPortID = inputPort; + component.formTitle = "old"; + + texeraGraph.portDisplayNameChangedSubject.next({ + operatorID: inputPort.operatorID, + portID: outputPort.portID, + newDisplayName: "renamed", + }); + + expect(component.formTitle).toBe("old"); + }); }); describe("quill title editing", () => { @@ -422,4 +438,110 @@ describe("PortPropertyEditFrameComponent", () => { expect(component.editingTitle).toBe(false); }); }); + + /** + * The two tests above stub `registerQuillBinding` away, so the collaborative half of the title + * editor — the Quill instance mounted on `#customName` and the y-quill binding that carries + * shared edits into it — was never run. These drive the real Quill and QuillBinding against a + * local Y.Doc. They come last in the file deliberately: Quill 2 has no `destroy()`, so the + * instance leaves a MutationObserver and document listeners behind for `fixture.destroy()` to + * detach, and they must not be combined with `fakeAsync` (zone.js patches MutationObserver). + */ + describe("quill collaborative binding", () => { + /** The `#customName` div the component mounts the editor into, via the global lookup it uses. */ + const editorHost = () => document.getElementById("customName") as HTMLElement; + + it("should create the shared display-name text and bind the editor to it", () => { + const sharedPortDescription = new Y.Doc().getMap("portDescription"); + component.currentPortID = inputPort; + const sharedSpy = vi.spyOn(texeraGraph, "getSharedPortDescriptionType").mockReturnValue(sharedPortDescription); + + component.connectQuillToText(); + + // The descriptor has to be looked up for the port being edited: inputPort and outputPort differ + // only in portID, so this also catches a sibling-port slot swap. + expect(sharedSpy).toHaveBeenCalledWith(inputPort); + + // The map genuinely lacked the key, so this Y.Text can only have come from the connect call. + const sharedTitle = sharedPortDescription.get("displayName"); + expect(sharedTitle).toBeInstanceOf(Y.Text); + // The published text has to start EMPTY — seeding it would give every freshly customised port + // a bogus initial display name that the DOM `toContain` below would happily tolerate. + expect((sharedTitle as Y.Text).toString()).toBe(""); + expect(component.quillBinding).toBeDefined(); + + // A remote edit to the shared text reaches the mounted editor through the binding. + (sharedTitle as Y.Text).insert(0, "renamed remotely"); + expect(editorHost().textContent).toContain("renamed remotely"); + }); + + // Text sync is only half of what `connectQuillToText` sets up; y-quill routes remote *cursors* + // through the awareness it is handed and the `cursors` Quill module, and both are guarded + // (`if (quillCursors !== null && awareness)`) so losing either fails silently while edits + // keep flowing. QuillBinding keeps both on the instance, so they can be read back directly. + it("should hand the binding the shared awareness and the cursors module", () => { + const sharedPortDescription = new Y.Doc().getMap("portDescription"); + component.currentPortID = inputPort; + vi.spyOn(texeraGraph, "getSharedPortDescriptionType").mockReturnValue(sharedPortDescription); + + component.connectQuillToText(); + + expect((component.quillBinding as any).awareness).toBe(texeraGraph.getSharedModelAwareness()); + // `quill.getModule("cursors") || null`, so this is null when the module is switched off. + expect((component.quillBinding as any).quillCursors).toBeTruthy(); + }); + + it("should open on the existing shared text rather than replacing it", () => { + const sharedPortDescription = new Y.Doc().getMap("portDescription"); + const existingTitle = new Y.Text(); + sharedPortDescription.set("displayName", existingTitle); + existingTitle.insert(0, "already named"); + component.currentPortID = inputPort; + vi.spyOn(texeraGraph, "getSharedPortDescriptionType").mockReturnValue(sharedPortDescription); + + component.connectQuillToText(); + + expect(sharedPortDescription.get("displayName")).toBe(existingTitle); + // The content that was already shared is what the editor shows. + expect(editorHost().textContent).toContain("already named"); + }); + + /** + * The tests above call `connectQuillToText`/`disconnectQuillFromText` directly, which leaves the + * only way a user actually reaches them — the template's `(click)`, `(keyup.enter)` and + * `(focusout)` bindings — unexercised. Without these the whole feature can be unwired from the + * UI without a single test noticing. + */ + function openEditorFromButton(): void { + const sharedPortDescription = new Y.Doc().getMap("portDescription"); + component.currentPortID = inputPort; + vi.spyOn(texeraGraph, "getSharedPortDescriptionType").mockReturnValue(sharedPortDescription); + + fixture.debugElement.query(By.css("#formly-title button")).nativeElement.click(); + fixture.detectChanges(); + + expect(component.editingTitle).toBe(true); + expect(component.quillBinding).toBeDefined(); + } + + it("should open the collaborative editor from the edit button and close it on Enter", () => { + openEditorFromButton(); + + editorHost().dispatchEvent(new KeyboardEvent("keyup", { key: "Enter", bubbles: true })); + fixture.detectChanges(); + + expect(component.editingTitle).toBe(false); + expect(component.quillBinding).toBeUndefined(); + }); + + it("should close the collaborative editor when the name field loses focus", () => { + openEditorFromButton(); + + editorHost().dispatchEvent(new FocusEvent("focusout", { bubbles: true })); + fixture.detectChanges(); + + expect(component.editingTitle).toBe(false); + expect(component.quillBinding).toBeUndefined(); + }); + }); }); diff --git a/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts b/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts index db6ecc6aad..30bfe8fe02 100644 --- a/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts @@ -17,10 +17,16 @@ * under the License. */ -import { TestBed } from "@angular/core/testing"; +import { discardPeriodicTasks, fakeAsync, flushMicrotasks, TestBed, tick } from "@angular/core/testing"; import { Subscription } from "rxjs"; -import { WorkflowWebsocketService } from "./workflow-websocket.service"; +import { + WorkflowWebsocketService, + WS_HEARTBEAT_INTERVAL_MS, + WS_RECONNECT_INTERVAL_MS, +} from "./workflow-websocket.service"; import { commonTestProviders } from "../../../common/testing/test-utils"; +import { AuthService } from "../../../common/service/user/auth.service"; +import { GuiConfigService } from "../../../common/service/gui-config.service"; /** Browser-like WebSocket test double used to verify websocket reopen and subscription cleanup behavior. */ class FakeWebSocket extends EventTarget { @@ -46,7 +52,12 @@ class FakeWebSocket extends EventTarget { public onerror: ((ev: Event) => unknown) | null = null; public onmessage: ((ev: MessageEvent) => unknown) | null = null; - public send() {} + /** Frames written to the socket, so what the service actually put on the wire can be asserted. */ + public readonly sent: string[] = []; + + public send(frame: string) { + this.sent.push(frame); + } public close() { if (this.readyState === FakeWebSocket.CLOSED) { @@ -60,6 +71,16 @@ class FakeWebSocket extends EventTarget { } } +/** Every socket the service constructs while `RecordingWebSocket` is installed, in creation order. */ +const openedSockets: RecordingWebSocket[] = []; + +class RecordingWebSocket extends FakeWebSocket { + constructor(url: string) { + super(url); + openedSockets.push(this); + } +} + describe("WorkflowWebsocketService", () => { let service: WorkflowWebsocketService; @@ -164,4 +185,166 @@ describe("WorkflowWebsocketService", () => { window.WebSocket = originalWebSocket; } }); + + /** + * The handshake URL is the only place the caller's identity reaches the server, and the + * retry pipeline is what keeps a dropped connection from ending the session. Both were + * previously unexercised: the suite above only ever opened a socket with every argument + * supplied and never faulted one. + */ + describe("handshake URL and reconnection", () => { + let originalWebSocket: typeof WebSocket; + + beforeEach(() => { + openedSockets.length = 0; + originalWebSocket = window.WebSocket; + // Without this the real jsdom WebSocket dials ws://localhost:3000 and the test would be + // asserting on whatever happens to be listening on this machine. + window.WebSocket = RecordingWebSocket as unknown as typeof WebSocket; + }); + + afterEach(() => { + service.closeWebsocket(); + window.WebSocket = originalWebSocket; + AuthService.removeAccessToken(); + vi.restoreAllMocks(); + }); + + it("defaults the user id to 1 and says so when openWebsocket is called without one", () => { + const logSpy = vi.spyOn(console, "log"); + + service.openWebsocket(7); + + expect(logSpy).toHaveBeenCalledWith("uId is undefined, defaulting to uId = 1"); + // The substituted default has to reach the server, not just the log line. Anchored at the + // endpoint rather than at `?` so a wrong TEXERA_WEBSOCKET_ENDPOINT is caught too; the host + // is deliberately left out, since getWebsocketUrl derives it from document.baseURI. + expect(openedSockets[0].url).toContain("/wsapi/workflow-websocket?wid=7&uid=1"); + }); + + it("carries the computing-unit id in the handshake URL, and leaves the parameter out when none is given", () => { + // 5 is distinct from both wid and uid, so a swapped slot cannot masquerade as the right value. + service.openWebsocket(3, 9, 5); + expect(openedSockets[0].url).toContain("/wsapi/workflow-websocket?wid=3&uid=9&cuid=5"); + + // The other leg. NOTE: this records what the frontend does today, not a handshake the server + // accepts — WorkflowWebsocketResource.myOnOpen reads `cuid` with no Option guard, and + // admin-execution.component calls openWebsocket(wid) with no computing unit at all. If that + // gap is closed, this half of the test is meant to change with it. + service.openWebsocket(3, 9); + expect(openedSockets[1].url).toContain("/wsapi/workflow-websocket?wid=3&uid=9"); + expect(openedSockets[1].url).not.toContain("cuid"); + }); + + it("appends the stored access token to the handshake URL, and nothing when there is none", () => { + AuthService.setAccessToken("tok-abc"); + service.openWebsocket(1, 1, 1); + expect(openedSockets[0].url).toContain("&access-token=tok-abc"); + + AuthService.removeAccessToken(); + service.openWebsocket(1, 1, 1); + expect(openedSockets[1].url).not.toContain("access-token"); + }); + + it("reports the drop, waits out the reconnect delay, then redials and resends a heartbeat", fakeAsync(() => { + const logSpy = vi.spyOn(console, "log"); + const statuses: boolean[] = []; + const statusSubscription = service.getConnectionStatusStream().subscribe(value => statuses.push(value)); + + try { + service.openWebsocket(1, 1, 1); + flushMicrotasks(); // the fake socket reaches OPEN on a microtask + + // an inbound frame is what marks the connection up, so drive one through first — + // otherwise the `false` below would be indistinguishable from the seeded state. + openedSockets[0].onmessage?.( + new MessageEvent("message", { data: JSON.stringify({ type: "WorkflowStateEvent", state: "RUNNING" }) }) + ); + expect(statuses).toEqual([false, true]); + + openedSockets[0].onerror?.(new Event("error")); + + // the drop is published immediately... + expect(statuses).toEqual([false, true, false]); + expect(logSpy).toHaveBeenCalledWith("websocket connection lost, reconnecting in 3 seconds"); + + // ...but the redial is held back until the delay elapses + tick(WS_RECONNECT_INTERVAL_MS - 1); + expect(openedSockets.length).toBe(1); + tick(1); + flushMicrotasks(); + expect(openedSockets.length).toBe(2); + + // the heartbeat queued on reconnect is flushed to the new socket once it opens + expect(openedSockets[1].sent).toContain(JSON.stringify({ type: "HeartBeatRequest" })); + } finally { + statusSubscription.unsubscribe(); + service.closeWebsocket(); + } + })); + + it("merges the request payload into the frame it puts on the wire", async () => { + service.openWebsocket(1, 1, 1); + await Promise.resolve(); // the fake socket reaches OPEN on a microtask + + service.send("RetryRequest", { workers: ["worker-a", "worker-b"] }); + + // The type alone is not enough: everything the caller passed has to survive into the frame. + expect(openedSockets[0].sent).toEqual([ + JSON.stringify({ type: "RetryRequest", workers: ["worker-a", "worker-b"] }), + ]); + }); + + it("keeps the connection alive by sending a heartbeat once every interval", fakeAsync(() => { + // The keepalive timer is started by the constructor, so the service has to be built inside + // the fake zone for `tick` to reach it — the instance injected in `beforeEach` was + // constructed in the real zone and its interval is invisible here. + const heartbeatService = new WorkflowWebsocketService(TestBed.inject(GuiConfigService)); + const heartbeats = () => + openedSockets[0].sent.filter(frame => frame === JSON.stringify({ type: "HeartBeatRequest" })).length; + + try { + heartbeatService.openWebsocket(1, 1, 1); + flushMicrotasks(); + + // nothing before the interval elapses... + tick(WS_HEARTBEAT_INTERVAL_MS - 1); + expect(heartbeats()).toBe(0); + + // ...one on the tick, and it keeps repeating rather than firing once + tick(1); + expect(heartbeats()).toBe(1); + tick(WS_HEARTBEAT_INTERVAL_MS); + expect(heartbeats()).toBe(2); + } finally { + heartbeatService.closeWebsocket(); + // the constructor's interval is never unsubscribed, so drain it or fakeAsync fails the test + discardPeriodicTasks(); + } + })); + + it("adopts the worker count carried by a ClusterStatusUpdateEvent", async () => { + service.openWebsocket(1, 1, 1); + await Promise.resolve(); + + // 7 is neither the -1 initializer nor the value any other test leaves behind. + openedSockets[0].onmessage?.( + new MessageEvent("message", { data: JSON.stringify({ type: "ClusterStatusUpdateEvent", numWorkers: 7 }) }) + ); + + expect(service.numWorkers).toBe(7); + }); + + it("leaves the worker count alone for events that are not cluster status updates", async () => { + service.openWebsocket(1, 1, 1); + await Promise.resolve(); + service.numWorkers = 7; + + openedSockets[0].onmessage?.( + new MessageEvent("message", { data: JSON.stringify({ type: "WorkflowStateEvent", numWorkers: 99 }) }) + ); + + expect(service.numWorkers).toBe(7); + }); + }); });
