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-7877-246d86c1e4ccf8189d9b5b78c909626dc082178c in repository https://gitbox.apache.org/repos/asf/texera.git
commit a33ece9ab11bf47f4bab0fcb1cf3f4aee5ea2989 Author: Meng Wang <[email protected]> AuthorDate: Sun Aug 23 07:48:46 2026 +0000 test(frontend): close the remaining branch arms in three workspace files (#7877) ### What changes were proposed in this PR? 15 new tests take the missing arm of the branches #7849 lists, moving the three files from 294/317 branches to 309/317 and closing the one unreached line. | file | statements | branches | | --- | --- | --- | | `hugging-face.component.ts` | 295/295 | 127/134 -> 130/134 | | `workflow-action.service.ts` | 300/300 | 66/75 -> 72/75 | | `shared-model-change-handler.ts` | 198/199 -> **199/199** | 101/108 -> 107/108 | **SharedModelChangeHandler** — a delete arriving from another client (the local-only unhighlight is skipped, the removal is not); a position update that carries no value; display name and property changes on an **output** port, which take the other side of the `isInput` ternaries; a property change carrying only one half of the partition/dependency pair, which must stay silent; removing a port from a side that has none; and the defensive `throw new Error("undefined port operation …")`, reached by handing the handler an event whose path is neither the add/delete shape nor a property path — Yjs cannot produce one through the graph API, so that one call goes in directly. **WorkflowActionService** — `addOperatorsAndLinks` with neither links nor comment boxes; enabling modification while it is already enabled (which must not re-announce it); a three-operator layout where the x and y comparisons each decide the result in both directions; and `setTempWorkflow` with the shared-editing provider both meant to connect and not. **HuggingFaceComponent** — a teardown with no pending init timeout; the fallback to the last selected task when the form carries none; and restoring a snapshot that does not carry every task-scoped key. No production code was changed. ### The 8 arms left Five cannot be reached at all: - **`workflow-action.service.ts:372`, `:379` and `:872`** — all three read `elementPositionMap.get(id) !== newPosition`, and `getElementPosition()` returns a fresh `{ x, y }` on every call, so the comparison is between two distinct objects and is always true. The "position unchanged, skip the write" guard these three implement therefore never fires — worth a look on its own; a value comparison would make it real (and give the arm something to take). - **`hugging-face.component.ts:636`** — `defaults[key] ?? ""`. The `defaults` literal supplies every one of the twelve `taskScopedKeys`, and `??` does not fall back on `""`, so the fallback is dead. - **`shared-model-change-handler.ts:376`** — needs a port delta that is neither an insert nor a delete; the graph API cannot produce one. Three are reachable but only by waiting on a real timer, which this repo's specs avoid and the issue's own determinism note rules out: - **`hugging-face.component.ts:181`** — the `??` inside the `setTimeout(…, 0)` that `ngOnInit` schedules. - **`hugging-face.component.ts:235` and `:347`** — the `else if` arms in the task and model polling loops, which need a fetch to be cancelled while a poll is in flight. ### Any related issues, documentation, discussions? Closes #7849. ### How was this PR tested? `ng test --watch=false` over the three specs — 187 passed (172 existing + 15 new), run 3x for determinism. Coverage (`--coverage`) gives the table above. The failure path was verified by breaking one assertion in two of the specs (red, non-zero exit) and restoring them. `yarn --cwd frontend format:ci`, the repo's own lint step, is clean. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8 [1M context]) --- .../hugging-face/hugging-face.component.spec.ts | 48 +++++++++ .../model/shared-model-change-handler.spec.ts | 115 +++++++++++++++++++++ .../model/workflow-action.service.spec.ts | 57 ++++++++++ 3 files changed, 220 insertions(+) diff --git a/frontend/src/app/workspace/component/hugging-face/hugging-face.component.spec.ts b/frontend/src/app/workspace/component/hugging-face/hugging-face.component.spec.ts index c3a7935cf9..dd199b8987 100644 --- a/frontend/src/app/workspace/component/hugging-face/hugging-face.component.spec.ts +++ b/frontend/src/app/workspace/component/hugging-face/hugging-face.component.spec.ts @@ -1618,4 +1618,52 @@ describe("HuggingFaceComponent (TestBed)", () => { expect(fixture.debugElement.query(By.css("div.alert-danger"))).toBeNull(); }); }); + + /* + * The half-taken arms left on this component: a teardown with nothing pending, the fallback + * used when the form has no task selected, and a snapshot that does not carry every key. + */ + describe("remaining guards", () => { + // This suite's shared afterEach does not restore mocks, and one test below spies on a + // global, so the block cleans up after itself rather than leaving it installed for the + // rest of the file. + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("clears no timeout when initialization already finished", () => { + const clearSpy = vi.spyOn(globalThis, "clearTimeout"); + (component as any).initTimeout = null; + + component.ngOnDestroy(); + + // The interval teardown may still call clearInterval; only the timeout arm is asserted. + expect(clearSpy).not.toHaveBeenCalled(); + }); + + it("falls back to the last selected task when the form carries none", () => { + const snapshot = vi.spyOn(component as any, "snapshotTaskState").mockImplementation(() => {}); + vi.spyOn(component as any, "getCurrentTaskTag").mockReturnValue(null); + // Only the fallback is under test; what the selection then does is covered elsewhere. + vi.spyOn(component as any, "syncTaskSelection").mockImplementation(() => {}); + vi.spyOn(component as any, "restoreTaskState").mockImplementation(() => {}); + vi.spyOn(component as any, "loadAllModels").mockImplementation(() => {}); + (component as any).selectedTaskTag = "text-generation"; + + component.onTaskSelected("image-classification"); + + // The task being snapshotted is the fallback, not the (absent) form value. + expect(snapshot).toHaveBeenCalledWith("text-generation"); + }); + + it("restores only the keys a snapshot actually carries", () => { + const write = vi.spyOn(component as any, "writeFieldValue").mockImplementation(() => {}); + (component as any).taskStateByTag.set("image-classification", { modelId: "m1" }); + + (component as any).restoreTaskState("image-classification"); + + const writtenKeys = write.mock.calls.map(call => call[0]); + expect(writtenKeys).toEqual(["modelId"]); + }); + }); }); diff --git a/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts index c363a9e456..72e1902acf 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts @@ -218,6 +218,18 @@ describe("SharedModelChangeHandler", () => { }); }); + describe("element position change with no value", () => { + it("leaves the joint element alone when the update carries no position", () => { + addOperatorWithPosition(mockScanPredicate); + const setAbsolute = vi.spyOn(jointGraphWrapper, "setAbsolutePosition"); + + // An update whose value is empty: the handler must not try to place the element. + texeraGraph.sharedModel.elementPositionMap.set(mockScanPredicate.operatorID, null as never); + + expect(setAbsolute).not.toHaveBeenCalled(); + }); + }); + describe("comment box add and delete", () => { it("should emit commentBoxAddSubject and add a joint comment element when a comment box is added", () => { let addedCommentBox: any; @@ -441,6 +453,91 @@ describe("SharedModelChangeHandler", () => { expect(change.newProperty.dependencies).toEqual([{ id: 0, internal: false }]); sub.unsubscribe(); }); + + it("reports a display-name change on an output port too", () => { + // The input side is covered above; the handler picks the port list from an `isInput` + // ternary, so the output side is a separate arm. + const operatorWithNamedOutput: OperatorPredicate = { + ...mockScanPredicate, + outputPorts: [{ portID: "output-0", displayName: "orig" }], + }; + addOperatorWithPosition(operatorWithNamedOutput); + + let change: { operatorID: string; portID: string; newDisplayName: string } | undefined; + const sub = texeraGraph.getPortDisplayNameChangedSubject().subscribe(v => (change = v)); + + const portType = texeraGraph.getSharedPortDescriptionType({ + operatorID: operatorWithNamedOutput.operatorID, + portID: "output-0", + }); + (portType!.get("displayName") as any).insert(0, "new-"); + + expect(change).toBeDefined(); + expect(change!.portID).toEqual("output-0"); + expect(change!.newDisplayName).toEqual("new-orig"); + sub.unsubscribe(); + }); + + it("reports a property change on an output port too", () => { + addOperatorWithPosition(mockScanPredicate); + + let change: any; + const sub = texeraGraph.getPortPropertyChangedStream().subscribe(v => (change = v)); + + texeraGraph.setPortProperty( + { operatorID: mockScanPredicate.operatorID, portID: "output-0" }, + { partitionInfo: { type: "none" }, dependencies: [] } + ); + + expect(change).toBeDefined(); + expect(change.operatorPortID.portID).toEqual("output-0"); + sub.unsubscribe(); + }); + + it("stays quiet for a port property change that carries no partition or dependencies", () => { + addOperatorWithPosition(mockSentimentPredicate); + + let emitted = false; + const sub = texeraGraph.getPortPropertyChangedStream().subscribe(() => (emitted = true)); + + // Only one half of the pair the handler requires before announcing a change. + texeraGraph.setPortProperty( + { operatorID: mockSentimentPredicate.operatorID, portID: "input-0" }, + { partitionInfo: { type: "hash", hashAttributeNames: ["col"] } } + ); + + expect(emitted).toBe(false); + sub.unsubscribe(); + }); + + it("removes nothing from the joint element when that side has no ports", () => { + // mockScanPredicate has output ports only; the input side has nothing to strip, which + // is the arm the port-removal helper guards against. + addOperatorWithPosition(mockScanPredicate); + const handler = (texeraGraph as any).sharedModelChangeHandler ?? (service as any).sharedModelChangeHandler; + const element = jointGraph.getCell(mockScanPredicate.operatorID) as joint.dia.Element; + const portsBefore = element.getPorts().length; + + (handler as any).onPortRemoved(mockScanPredicate.operatorID, true); + + expect(element.getPorts().length).toEqual(portsBefore); + }); + + it("refuses a port event that is neither a display name nor a property change", () => { + addOperatorWithPosition(mockSentimentPredicate); + const handler = (texeraGraph as any).sharedModelChangeHandler ?? (service as any).sharedModelChangeHandler; + + // A path of length 1 reaches neither the add/delete arm (length 2) nor either + // property arm, which is the guard's whole purpose. Yjs cannot produce such an event + // through the graph API, so it is handed to the handler directly. + expect(() => + (handler as any).handlePortEvent( + { path: ["ports"], delta: [], target: { toJSON: () => "" } }, + mockSentimentPredicate.operatorID, + true + ) + ).toThrow(/undefined port operation/); + }); }); describe("deep comment box changes", () => { @@ -578,6 +675,24 @@ describe("SharedModelChangeHandler", () => { sub.unsubscribe(); }); + it("deletes an operator that another client removed", () => { + addOperatorWithPosition(mockScanPredicate); + const deleted: string[] = []; + const sub = texeraGraph.getOperatorDeleteStream().subscribe(e => deleted.push(e.deletedOperatorID)); + + applyAsRemote(peerDoc => + peerDoc.transact(() => { + peerDoc.getMap("operatorIDMap").delete(mockScanPredicate.operatorID); + peerDoc.getMap("elementPositionMap").delete(mockScanPredicate.operatorID); + }) + ); + + // The local-only unhighlight is skipped for a remote transaction; the removal itself is not. + expect(deleted).toEqual([mockScanPredicate.operatorID]); + expect(jointGraph.getCell(mockScanPredicate.operatorID)).toBeFalsy(); + sub.unsubscribe(); + }); + it("applies a remote property change without updating local awareness", () => { addOperatorWithPosition(mockScanPredicate); // The awareness update needs both a local transaction and this operator being the one diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts index efa3ce65a2..7dbfaf788d 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts @@ -860,4 +860,61 @@ describe("WorkflowActionService", () => { y: sentimentBefore.y + offset.y, }); }); + + /* + * The remaining half-taken arms on this service: an optional argument left out, a guard whose + * "already in that state" side never ran, and the comparisons in the top-left calculation. + */ + it("adds operators without links or comment boxes when neither is supplied", () => { + service.addOperatorsAndLinks([ + { op: mockScanPredicate, pos: { x: 10, y: 20 } }, + { op: mockResultPredicate, pos: { x: 30, y: 40 } }, + ]); + + expect( + texeraGraph + .getAllOperators() + .map(o => o.operatorID) + .sort() + ).toEqual([mockScanPredicate.operatorID, mockResultPredicate.operatorID].sort()); + expect(texeraGraph.getAllLinks()).toEqual([]); + expect(texeraGraph.getAllCommentBoxes()).toEqual([]); + }); + + it("does nothing when modification is enabled again while already enabled", () => { + const emitted: boolean[] = []; + service.getWorkflowModificationEnabledStream().subscribe(v => emitted.push(v)); + // Starts enabled, so this call has nothing to do and must not re-announce it. + service.enableWorkflowModification(); + + expect(service.checkWorkflowModificationEnabled()).toBeTruthy(); + expect(emitted).toEqual([true]); + }); + + it("takes the smaller of each coordinate independently when computing the top-left", () => { + // The x guard is false for the second operator and the y guard is true, so each + // comparison decides the result once in both directions. + service.addOperator(mockScanPredicate, { x: 100, y: 400 }); + service.addOperator(mockResultPredicate, { x: 300, y: 250 }); + service.addOperator(mockSentimentPredicate, { x: 50, y: 500 }); + + service.calculateTopLeftOperatorPosition(); + + // x comes from the third operator and y from the second, so neither comparison + // decides both coordinates. + expect(service.getCenterPoint()).toEqual({ x: 50, y: 250 }); + }); + + it("disconnects the shared-editing provider only when it is meant to be connected", () => { + const disconnect = vi.spyOn(texeraGraph.sharedModel.wsProvider, "disconnect").mockImplementation(() => {}); + const workflow = service.getWorkflow(); + + (texeraGraph.sharedModel.wsProvider as any).shouldConnect = false; + service.setTempWorkflow(workflow); + expect(disconnect).not.toHaveBeenCalled(); + + (texeraGraph.sharedModel.wsProvider as any).shouldConnect = true; + service.setTempWorkflow(workflow); + expect(disconnect).toHaveBeenCalledTimes(1); + }); });
