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-7776-42ac6fef0eb47766cbe60467986dac6bacbba7e9 in repository https://gitbox.apache.org/repos/asf/texera.git
commit d5d1db38e0bcd6b6d59ff34bc56d99d7ffc8a461 Author: Xinyuan Lin <[email protected]> AuthorDate: Wed Aug 19 09:05:02 2026 +0000 test(frontend): cover the operator label drag, computing-unit rows and menu clipboard (#7776) ### What changes were proposed in this PR? Four workspace and dashboard files whose handlers and bindings were never driven. 12 tests added; suite 4739 -> **4751**, 201 files green. Measured from a **full-suite** run, because three of these files are also touched by other specs and a targets-only run misreports them: | File | Lines | Branches | Functions | |---|---|---|---| | `operator-label.component.ts` | 72.7% -> **100%** | 10/12 -> **12/12** | 4/6 -> **6/6** | | `user-computing-unit.component.html` | 81.2% -> **100%** | 2/2 | 2/4 -> **4/4** | | `operator-menu.service.ts` | 98.0% -> **100%** | 35/36 -> **36/36** | 29/32 -> **32/32** | | `dynamic-schema.service.ts` | 96.4% -> **98.2%** | 23/25 -> **25/25** | 16/17 (one dead method) | **None of these is an instance of #7458** — no spec here uses `TestBed.overrideComponent`, which the non-zero percentages already implied. Six merged PRs have applied that remedy; it would have been the wrong tool here, and these were simply untested bindings. Covered: the operator label's drag start/stop and its read-only guard, the computing-unit toolbar and per-row terminate, the clipboard's link and comment-box serialisation ordering and paste offsetting, and `dynamic-schema`'s boolean-subschema skips. The computing-unit rows sit in a CDK virtual viewport, and jsdom does no layout — the viewport measures 0px and renders nothing. They are reached via `viewport.setRenderedRange(...)`, following the existing precedent in `computing-unit-create-modal.component.spec.ts`. Row deletion is driven through the child's bound `deleted` output rather than a library's internal emit timing. ### Verification 12 mutations, **10 killed, 1 equivalent, 2 discarded as compile errors**. Each applied alone with its anchor asserted to occur exactly once, and the production diff verified empty after every revert. | Mutation | Killed by | |---|---| | `if (this.draggable)` -> `if (!this.draggable)` | announces the dragged operator's type… (+1) | | drag payload `operatorType` -> `additionalMetadata.userFriendlyName` | announces the dragged operator's type, not its display name | | **exchange** the drop point's x and y | forwards the drop point of a completed drag | | **exchange** the links comparator's operands | serializes the copied links by joint layer rather than by highlight order | | **exchange** the comment-box comparator's operands | serializes the copied comment boxes by joint layer… | | **exchange** `commentBoxPosition`'s x and y | shifts a pasted comment box clear of one already on the canvas | | pasted-link guard `===` -> `!==` | leaves both ends of a pasted link blank when neither operator was copied (+1) | | `(click)` -> `(dblclick)` on the toolbar | opens the create-unit modal from its toolbar button | | `[entry]="entry.computingUnit"` -> `entries[0]…` | renders one row per computing unit, in the order the service reported them | | `terminateComputingUnit(entry…cuid)` -> `entries[0]…cuid` | terminates the unit belonging to the row that asked, not the first one | **Two mutations were discarded rather than counted as kills**, because they were TypeScript errors (TS2345 and TS2559) rather than behaviour changes — a mutation that only fails to compile proves nothing. Both were replaced: one by deleting the guard outright (with a cast to keep it compiling), one by inverting the branch so the boolean entry is acted on instead of skipped. **The one survivor is a genuine equivalent mutant.** `dynamic-schema.service.ts:187`'s tuple-item guard has `mutatePropertyRecurse(item)` as its entire body. Delete it and a boolean primitive reaches that function, which reads `.properties`, `.definitions` and `.items` — property access on a boolean yields `undefined` for all three, so every branch is false and the call is a no-op, behaviourally identical to skipping. The guard is a TypeScript narrowing device, not a behavioural one; removing it does not even compile without a cast. The companion mutation (inverting the branch to act on the boolean) *is* caught, which shows the assertion is not vacuous. Fixtures were kept deliberately distinguishable — two units `#7` and `#9`, a drop point of `x: 137, y: 421`, and layer order inverted against highlight order — precisely so the "wrong element" and "swapped operand" mutants above could not survive. ### Deliberately not included **`user-dataset-list-item.component.html` was rejected entirely: the component is orphaned.** Its selector `texera-user-dataset-list-item` appears repo-wide only in its own `@Component` declaration and its own spec's test host. The one other reference is a standalone entry in `app.module.ts:325` with no consumer — the dataset page moved to `texera-card-item`/`texera-search-results` in #5947. All 8 of its uncovered template listeners belong to a component nothing renders, so testing them would cement dead code. Deleting it is the right change, and belongs in its own PR. **`DynamicSchemaService.dynamicSchemaExists` is unused** — exactly one occurrence repo-wide, its own declaration, while every sibling method has 2 to 5 call sites. Its line is the one line left uncovered here, deliberately. No production bug was found, and no production file is touched. ### Any related issues, documentation, discussions? Closes #7775 ### How was this PR tested? ``` npx ng test --watch=false ``` ``` Test Files 201 passed (201) Tests 4751 passed | 1 skipped (4752) ``` `yarn format:ci` passes. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../user-computing-unit.component.spec.ts | 92 ++++++++++++ .../operator-label.component.spec.ts | 48 +++++++ .../dynamic-schema/dynamic-schema.service.spec.ts | 52 +++++++ .../operator-menu/operator-menu.service.spec.ts | 156 +++++++++++++++++++++ 4 files changed, 348 insertions(+) diff --git a/frontend/src/app/dashboard/component/user/user-computing-unit/user-computing-unit.component.spec.ts b/frontend/src/app/dashboard/component/user/user-computing-unit/user-computing-unit.component.spec.ts index 9cb56f32a4..de190dda42 100644 --- a/frontend/src/app/dashboard/component/user/user-computing-unit/user-computing-unit.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-computing-unit/user-computing-unit.component.spec.ts @@ -37,6 +37,8 @@ import { ComputingUnitActionsService } from "../../../../common/service/computin import { NotificationService } from "../../../../common/service/notification/notification.service"; import { of } from "rxjs"; import type { Mocked } from "vitest"; +import { CdkVirtualScrollViewport } from "@angular/cdk/scrolling"; +import { UserComputingUnitListItemComponent } from "./user-computing-unit-list-item/user-computing-unit-list-item.component"; describe("UserComputingUnitComponent", () => { let component: UserComputingUnitComponent; let fixture: ComponentFixture<UserComputingUnitComponent>; @@ -201,4 +203,94 @@ describe("UserComputingUnitComponent", () => { expect(terminateSpy).not.toHaveBeenCalled(); }); }); + + /** + * Everything above reaches the component through its methods, so the template's own wiring — the + * toolbar button and the row bindings inside the virtual-scroll list — never ran. jsdom performs + * no layout, which is why the list looked empty: the viewport measures 0px and renders no rows. + */ + describe("rendered page", () => { + function makeUnit(cuid: number): DashboardWorkflowComputingUnit { + return { + computingUnit: { + cuid, + uid: 1, + name: `unit-${cuid}`, + creationTime: 0, + terminateTime: undefined, + type: "kubernetes", + uri: `uri-${cuid}`, + resource: { + cpuLimit: "1", + memoryLimit: "1Gi", + gpuLimit: "0", + jvmMemorySize: "1Gi", + shmSize: "64Mi", + nodeAddresses: [], + }, + }, + status: "Running", + metrics: { cpuUsage: "N/A", memoryUsage: "N/A" }, + isOwner: true, + accessPrivilege: "WRITE", + ownerAvatar: "", + ownerName: "owner", + } as DashboardWorkflowComputingUnit; + } + + /** Renders the page with the given units and forces the virtual list to materialize its rows. */ + function renderUnits(units: DashboardWorkflowComputingUnit[]): void { + const statusService = TestBed.inject(ComputingUnitStatusService); + vi.spyOn(statusService, "getAllComputingUnits").mockReturnValue(of(units)); + fixture.detectChanges(); + + const viewport = fixture.debugElement.query(By.css("cdk-virtual-scroll-viewport")) + .componentInstance as CdkVirtualScrollViewport; + viewport.setRenderedRange({ start: 0, end: units.length }); + fixture.detectChanges(); + } + + afterEach(() => { + // ngOnInit starts a 1s poll; destroying the fixture unsubscribes it. + fixture.destroy(); + }); + + it("opens the create-unit modal from its toolbar button", () => { + fixture.detectChanges(); + const modal = fixture.debugElement.query(By.directive(ComputingUnitCreateModalComponent)) + .componentInstance as ComputingUnitCreateModalComponent; + expect(modal.visible).toBe(false); + + fixture.nativeElement.querySelector(".create-btn").click(); + fixture.detectChanges(); + + expect(modal.visible).toBe(true); + }); + + it("renders one row per computing unit, in the order the service reported them", () => { + renderUnits([makeUnit(7), makeUnit(9)]); + + const rendered = Array.from((fixture.nativeElement as HTMLElement).querySelectorAll<HTMLElement>(".unit-id")).map( + el => el.textContent?.trim() + ); + expect(rendered).toEqual(["#7", "#9"]); + }); + + it("terminates the unit belonging to the row that asked, not the first one", () => { + const actions = TestBed.inject(ComputingUnitActionsService); + const terminateSpy = vi.spyOn(actions, "confirmAndTerminate").mockImplementation(() => {}); + renderUnits([makeUnit(7), makeUnit(9)]); + + const rows = fixture.debugElement.queryAll(By.directive(UserComputingUnitListItemComponent)); + expect(rows.length).toBe(2); + // the second row's delete must reach unit 9; two rows are rendered so a binding that always + // resolved to the first entry could not pass. + (rows[1].componentInstance as UserComputingUnitListItemComponent).deleted.emit(); + + expect(terminateSpy).toHaveBeenCalledTimes(1); + const [cuid, unit] = terminateSpy.mock.calls[0]; + expect(cuid).toBe(9); + expect(unit.computingUnit.cuid).toBe(9); + }); + }); }); diff --git a/frontend/src/app/workspace/component/left-panel/operator-menu/operator-label/operator-label.component.spec.ts b/frontend/src/app/workspace/component/left-panel/operator-menu/operator-label/operator-label.component.spec.ts index 2cef5f1d20..b6b95d0088 100644 --- a/frontend/src/app/workspace/component/left-panel/operator-menu/operator-label/operator-label.component.spec.ts +++ b/frontend/src/app/workspace/component/left-panel/operator-menu/operator-label/operator-label.component.spec.ts @@ -30,6 +30,7 @@ import { WorkflowActionService } from "../../../../service/workflow-graph/model/ import { UndoRedoService } from "../../../../service/undo-redo/undo-redo.service"; import { RouterTestingModule } from "@angular/router/testing"; import { commonTestProviders } from "../../../../../common/testing/test-utils"; +import { CdkDrag, CdkDragDrop, CdkDragStart } from "@angular/cdk/drag-drop"; describe("OperatorLabelComponent", () => { const mockOperatorData = mockScanSourceSchema; @@ -71,4 +72,51 @@ describe("OperatorLabelComponent", () => { const element = <HTMLElement>fixture.debugElement.query(By.css(".text")).nativeElement; expect(element.textContent?.trim()).toEqual(mockOperatorData.additionalMetadata.userFriendlyName); }); + + /** + * Dragging a label onto the canvas is the whole point of this component, and both of its handlers + * ran uncounted: the suite above only renders the label. These drive the two CdkDrag outputs the + * template binds rather than calling the methods directly, so the bindings are exercised too. + */ + describe("drag handling", () => { + let dragDropService: DragDropService; + let cdkDrag: CdkDrag; + + beforeEach(() => { + dragDropService = TestBed.inject(DragDropService); + cdkDrag = fixture.debugElement.query(By.directive(CdkDrag)).injector.get(CdkDrag); + }); + + it("announces the dragged operator's type, not its display name", () => { + // The canvas needs the type to instantiate an operator; "Source: Scan" would not resolve. + const dragStartedSpy = vi.spyOn(dragDropService, "dragStarted").mockImplementation(() => {}); + + cdkDrag.started.emit({ source: cdkDrag } as unknown as CdkDragStart); + + expect(dragStartedSpy).toHaveBeenCalledExactlyOnceWith(mockOperatorData.operatorType); + }); + + it("refuses to start a drag while the workflow is read-only", () => { + const dragStartedSpy = vi.spyOn(dragDropService, "dragStarted").mockImplementation(() => {}); + TestBed.inject(WorkflowActionService).disableWorkflowModification(); + fixture.detectChanges(); + + // the label is visibly marked undraggable ... + const label = <HTMLElement>fixture.debugElement.query(By.css(".operator-label")).nativeElement; + expect(label.classList.contains("disable-drag-drop")).toBe(true); + + // ... and a drag that fires anyway is dropped on the floor + cdkDrag.started.emit({ source: cdkDrag } as unknown as CdkDragStart); + expect(dragStartedSpy).not.toHaveBeenCalled(); + }); + + it("forwards the drop point of a completed drag", () => { + const dragDroppedSpy = vi.spyOn(dragDropService, "dragDropped").mockImplementation(() => {}); + + // x and y differ so a handler that swapped them would not go unnoticed + cdkDrag.dropped.emit({ dropPoint: { x: 137, y: 421 } } as unknown as CdkDragDrop<unknown>); + + expect(dragDroppedSpy).toHaveBeenCalledExactlyOnceWith({ x: 137, y: 421 }); + }); + }); }); diff --git a/frontend/src/app/workspace/service/dynamic-schema/dynamic-schema.service.spec.ts b/frontend/src/app/workspace/service/dynamic-schema/dynamic-schema.service.spec.ts index e5997027c9..2bfde18542 100644 --- a/frontend/src/app/workspace/service/dynamic-schema/dynamic-schema.service.spec.ts +++ b/frontend/src/app/workspace/service/dynamic-schema/dynamic-schema.service.spec.ts @@ -241,6 +241,58 @@ describe("DynamicSchemaService.mutateProperty", () => { expect((itemSchema.properties!.target as CustomJSONSchema7).description).toEqual("mutated"); }); + /** + * JSON Schema lets a subschema be the bare boolean `true`/`false` instead of an object, and + * mutateProperty has to step over those: they carry no property name to match and no children to + * walk. Both guards were reached only on their object side, so neither skip was pinned. + */ + it("should step over a boolean subschema without matching or mutating it", () => { + const original = { + type: "object", + properties: { + anything: true, + target: { type: "string", description: "original" }, + }, + } as unknown as CustomJSONSchema7; + const matchSpy = vi.fn((propertyName: string, _: CustomJSONSchema7) => propertyName !== "unreachable"); + + const result = DynamicSchemaService.mutateProperty(original, matchSpy, markMutated); + + // the boolean survives as a boolean rather than being handed to the mutation function + expect(result.properties!.anything).toBe(true); + expect(matchSpy.mock.calls.map(([propertyName]) => propertyName)).toEqual(["target"]); + // the walk continues past it, so the object sibling is still mutated + expect((result.properties!.target as CustomJSONSchema7).description).toEqual("mutated"); + }); + + it("should step over a boolean entry in a tuple items array", () => { + const original = { + type: "object", + properties: { + listTuple: { + type: "array", + items: [ + false, + { + type: "object", + properties: { + target: { type: "string", description: "original" }, + }, + }, + ], + }, + }, + } as unknown as CustomJSONSchema7; + + const result = DynamicSchemaService.mutateProperty(original, matchByName("target"), markMutated); + + const items = (result.properties!.listTuple as CustomJSONSchema7).items as unknown[]; + // the boolean entry is left exactly as it was ... + expect(items[0]).toBe(false); + // ... and its object sibling further along the array is still reached + expect(((items[1] as CustomJSONSchema7).properties!.target as CustomJSONSchema7).description).toEqual("mutated"); + }); + it("should not invoke the mutation function when nothing matches", () => { const original = { type: "object", diff --git a/frontend/src/app/workspace/service/operator-menu/operator-menu.service.spec.ts b/frontend/src/app/workspace/service/operator-menu/operator-menu.service.spec.ts index a8d2070abb..0f1282a4a5 100644 --- a/frontend/src/app/workspace/service/operator-menu/operator-menu.service.spec.ts +++ b/frontend/src/app/workspace/service/operator-menu/operator-menu.service.spec.ts @@ -34,7 +34,9 @@ import { mockScanPredicate, mockScanSentimentLink, mockSentimentPredicate, + mockSentimentResultLink, } from "../workflow-graph/model/mock-workflow-data"; +import { CommentBox } from "../../types/workflow-common.interface"; import { NotificationService } from "../../../common/service/notification/notification.service"; import { ExecuteWorkflowService } from "../execute-workflow/execute-workflow.service"; import { Subscription } from "rxjs"; @@ -458,4 +460,158 @@ describe("OperatorMenuService", () => { ); }); }); + + /** + * Copying and pasting a *single* link, comment box, or operator never reaches the code that + * relates two of them: JS skips a one-element sort comparator entirely, and the overlap scan has + * nothing to scan against. The suite above copies exactly one of each, so the layer sorting and + * the collision check against elements already on the canvas both went unexercised. + */ + describe("ordering and collision across multiple elements", () => { + let originalClipboard: PropertyDescriptor | undefined; + let writeText: ReturnType<typeof vi.fn>; + let readText: ReturnType<typeof vi.fn>; + + beforeEach(() => { + originalClipboard = Object.getOwnPropertyDescriptor(navigator, "clipboard"); + writeText = vi.fn().mockResolvedValue(undefined); + readText = vi.fn(); + Object.defineProperty(navigator, "clipboard", { value: { writeText, readText }, configurable: true }); + }); + + afterEach(() => { + if (originalClipboard) { + Object.defineProperty(navigator, "clipboard", originalClipboard); + } else { + delete (navigator as any).clipboard; + } + }); + + /** + * Adds the three mock operators and the two links between them. Multi-select is deliberately + * left off: adding an element turns it back off, so each test enables it once everything it + * needs is on the canvas. + */ + function buildChain() { + workflowActionService.addOperatorsAndLinks( + [ + { op: mockScanPredicate, pos: mockPoint }, + { op: mockSentimentPredicate, pos: mockPoint }, + { op: mockResultPredicate, pos: mockPoint }, + ], + [mockScanSentimentLink, mockSentimentResultLink] + ); + return workflowActionService.getJointGraphWrapper(); + } + + it("serializes the copied links by joint layer rather than by highlight order", () => { + const wrapper = buildChain(); + wrapper.unhighlightOperators(...wrapper.getCurrentHighlightedOperatorIDs()); + wrapper.setMultiSelectMode(true); + wrapper.highlightLinks(mockScanSentimentLink.linkID, mockSentimentResultLink.linkID); + + // Send the link highlighted *second* to the back, so layer order is the reverse of highlight + // order: a comparator that never ran, or that subtracted the other way round, would show up. + workflowActionService.getJointGraph().getCell(mockSentimentResultLink.linkID).toBack(); + expect(wrapper.getCellLayer(mockSentimentResultLink.linkID)).toBeLessThan( + wrapper.getCellLayer(mockScanSentimentLink.linkID) + ); + expect(wrapper.getCurrentHighlightedLinkIDs()).toEqual([ + mockScanSentimentLink.linkID, + mockSentimentResultLink.linkID, + ]); + + service.saveHighlightedElements(); + + const serialized = JSON.parse(writeText.mock.calls[0][0]); + expect(serialized.links.map((link: any) => link.linkID)).toEqual([ + mockSentimentResultLink.linkID, + mockScanSentimentLink.linkID, + ]); + }); + + it("serializes the copied comment boxes by joint layer rather than by highlight order", () => { + const wrapper = buildChain(); + const boxA = { ...mockCommentBox, commentBoxID: "comment-box-a" }; + const boxB = { ...mockCommentBox, commentBoxID: "comment-box-b" }; + workflowActionService.addCommentBox(boxA); + workflowActionService.addCommentBox(boxB); + wrapper.unhighlightOperators(...wrapper.getCurrentHighlightedOperatorIDs()); + wrapper.unhighlightCommentBoxes(...wrapper.getCurrentHighlightedCommentBoxIDs()); + wrapper.setMultiSelectMode(true); + wrapper.highlightCommentBoxes(boxA.commentBoxID, boxB.commentBoxID); + + workflowActionService.getJointGraph().getCell(boxB.commentBoxID).toBack(); + expect(wrapper.getCellLayer(boxB.commentBoxID)).toBeLessThan(wrapper.getCellLayer(boxA.commentBoxID)); + expect(wrapper.getCurrentHighlightedCommentBoxIDs()).toEqual([boxA.commentBoxID, boxB.commentBoxID]); + + service.saveHighlightedElements(); + + const serialized = JSON.parse(writeText.mock.calls[0][0]); + expect(serialized.commentBoxes.map((box: any) => box.commentBoxID)).toEqual([ + boxB.commentBoxID, + boxA.commentBoxID, + ]); + }); + + it("shifts a pasted comment box clear of one already on the canvas", async () => { + // One COPY_OFFSET puts the pasted box on {320, 420}, where a box already sits, so the overlap + // scan has to push it one offset further. Were canvas boxes left out of that scan the paste + // would stop at {320, 420} and land on top of the existing one. + const existingBox: CommentBox = { + commentBoxID: "existing-box", + comments: [], + commentBoxPosition: { x: 320, y: 420 }, + }; + workflowActionService.addCommentBox(existingBox); + readText.mockResolvedValue( + JSON.stringify({ + operators: [], + operatorPositions: {}, + links: [], + commentBoxes: [{ commentBoxID: "clipboard-box", comments: [], commentBoxPosition: { x: 300, y: 400 } }], + }) + ); + + service.performPasteOperation(); + await flushAsync(); + + const pasted = workflowActionService + .getTexeraGraph() + .getAllCommentBoxes() + .filter(box => box.commentBoxID !== existingBox.commentBoxID); + expect(pasted.length).toBe(1); + expect(pasted[0].commentBoxPosition).toEqual({ x: 340, y: 440 }); + }); + + it("leaves both ends of a pasted link blank when neither of its operators was copied", async () => { + const addSpy = vi.spyOn(workflowActionService, "addOperatorsAndLinks").mockImplementation(() => {}); + readText.mockResolvedValue( + JSON.stringify({ + operators: [mockScanPredicate], + operatorPositions: { [mockScanPredicate.operatorID]: { x: 100, y: 100 } }, + // the first link starts at the copied operator; the second joins two operators that were not + links: [mockScanSentimentLink, mockSentimentResultLink], + commentBoxes: [], + }) + ); + + service.performPasteOperation(); + await flushAsync(); + + const [operatorsAndPositions, maybeLinks] = addSpy.mock.calls[0]; + const pastedOperatorID = operatorsAndPositions[0].op.operatorID; + const links = maybeLinks!; + expect(links.length).toBe(2); + // the link starting at the copied operator is rewired on its source side, port kept as-is + expect(links[0].source).toEqual({ + operatorID: pastedOperatorID, + portID: mockScanSentimentLink.source.portID, + }); + expect(links[0].target).toEqual({ operatorID: "", portID: "" }); + // the link touching neither copied operator is rewired on neither side + expect(links[1].source).toEqual({ operatorID: "", portID: "" }); + expect(links[1].target).toEqual({ operatorID: "", portID: "" }); + }); + }); });
