Copilot commented on code in PR #6352:
URL: https://github.com/apache/texera/pull/6352#discussion_r3565275280
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -277,4 +279,305 @@ describe("WorkflowActionService", () => {
// expect(sentimentOpPos).toEqual(mockPoint);
// expect(resultOpPos).toEqual(mockPoint);
});
+
+ it("should reload a workflow, repopulating the graph and clearing the
undo/redo stacks", () => {
+ const settings: WorkflowSettings = {
+ dataTransferBatchSize: 250,
+ executionMode: ExecutionMode.MATERIALIZED,
+ };
+ const workflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ name: "Reloaded WF",
+ content: {
+ operators: [mockScanPredicate, mockResultPredicate],
+ operatorPositions: {
+ [mockScanPredicate.operatorID]: mockPoint,
+ [mockResultPredicate.operatorID]: mockPoint,
+ },
+ links: [mockScanResultLink],
+ commentBoxes: [{ ...mockCommentBox, commentBoxID: "commentBox-1" }],
+ settings,
+ },
+ };
+ const clearUndoSpy = vi.spyOn(undoRedo, "clearUndoStack");
+ const clearRedoSpy = vi.spyOn(undoRedo, "clearRedoStack");
+ const restoreSpy = vi.spyOn(service.getJointGraphWrapper(),
"restoreDefaultZoomAndOffset");
+
+ service.reloadWorkflow(workflow);
+
+ expect(texeraGraph.hasOperator(mockScanPredicate.operatorID)).toBeTruthy();
+
expect(texeraGraph.hasOperator(mockResultPredicate.operatorID)).toBeTruthy();
+ expect(texeraGraph.hasLinkWithID(mockScanResultLink.linkID)).toBeTruthy();
+ expect(texeraGraph.hasCommentBox("commentBox-1")).toBeTruthy();
+ expect(service.getWorkflowMetadata().name).toEqual("Reloaded WF");
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(250);
+ expect(clearUndoSpy).toHaveBeenCalled();
+ expect(clearRedoSpy).toHaveBeenCalled();
+ expect(restoreSpy).toHaveBeenCalled();
+ });
+
+ it("should throw when a reloaded operator is missing its position", () => {
+ const workflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ content: {
+ operators: [mockScanPredicate],
+ operatorPositions: {},
+ links: [],
+ commentBoxes: [],
+ settings: { dataTransferBatchSize: 100, executionMode:
ExecutionMode.PIPELINED },
+ },
+ };
+ expect(() => service.reloadWorkflow(workflow)).toThrowError(
+ new RegExp(`position error: ${mockScanPredicate.operatorID}`)
+ );
+ });
+
+ it("should empty the graph and skip viewport restore when reloading
undefined", () => {
+ service.addOperator(mockScanPredicate, mockPoint);
+ const restoreSpy = vi.spyOn(service.getJointGraphWrapper(),
"restoreDefaultZoomAndOffset");
+
+ service.reloadWorkflow(undefined, false, false);
+
+ expect(texeraGraph.getAllOperators().length).toEqual(0);
+ expect(restoreSpy).not.toHaveBeenCalled();
+ });
+
+ it("should clear the workflow back to its defaults", () => {
+ service.addOperator(mockScanPredicate, mockPoint);
+ service.setWorkflowName("Something");
+ service.setWorkflowDataTransferBatchSize(999);
+ service.setHighlightingEnabled(true);
+
+ service.clearWorkflow();
+
+ expect(service.getWorkflowMetadata()).toBe(DEFAULT_WORKFLOW);
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(100);
+
expect(service.getWorkflowSettings().executionMode).toEqual(ExecutionMode.PIPELINED);
+ expect(texeraGraph.getAllOperators().length).toEqual(0);
+ expect(service.getHighlightingEnabled()).toBeFalsy();
+ });
+
+ it("should emit on workflowChanged when resetting as a new workflow", () => {
+ let changed = false;
+ service.workflowChanged().subscribe(() => (changed = true));
+
+ service.resetAsNewWorkflow();
+
+ expect(changed).toBeTruthy();
+ });
+
+ it("should only emit metadata changes when the reference changes", () => {
+ const current = service.getWorkflowMetadata();
+ let emitCount = 0;
+ service.workflowMetaDataChanged().subscribe(() => (emitCount += 1));
+
+ // identical reference -> no emission
+ service.setWorkflowMetadata(current);
+ expect(emitCount).toEqual(0);
+
+ // new metadata -> emission
+ service.setWorkflowMetadata({ ...DEFAULT_WORKFLOW, name: "renamed" });
+ expect(emitCount).toEqual(1);
+ expect(service.getWorkflowMetadata().name).toEqual("renamed");
+ });
+
+ it("should set the workflow name, defaulting blank names", () => {
+ service.setWorkflowName(" ");
+ expect(service.getWorkflowMetadata().name).toEqual(DEFAULT_WORKFLOW_NAME);
+
+ service.setWorkflowName("My Workflow");
+ expect(service.getWorkflowMetadata().name).toEqual("My Workflow");
+ });
+
+ it("should manage workflow settings and publish state", () => {
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(100);
+
expect(service.getWorkflowSettings().executionMode).toEqual(ExecutionMode.PIPELINED);
+
+ // non-positive batch sizes are ignored, positive ones are applied
+ service.setWorkflowDataTransferBatchSize(0);
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(100);
+ service.setWorkflowDataTransferBatchSize(400);
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(400);
+
+ service.updateExecutionMode(ExecutionMode.MATERIALIZED);
+
expect(service.getWorkflowSettings().executionMode).toEqual(ExecutionMode.MATERIALIZED);
+
+ service.setWorkflowIsPublished(1);
+ expect(service.getWorkflowMetadata().isPublished).toEqual(1);
+
+ const settings: WorkflowSettings = { dataTransferBatchSize: 42,
executionMode: ExecutionMode.PIPELINED };
+ service.setWorkflowSettings(settings);
+ expect(service.getWorkflowSettings()).toEqual(settings);
+ });
+
+ it("should toggle the shared-editing connection through temp workflow", ()
=> {
+ const wsProvider = texeraGraph.sharedModel.wsProvider;
+ const disconnectSpy = vi.spyOn(wsProvider,
"disconnect").mockImplementation(() => {});
+ const connectSpy = vi.spyOn(wsProvider, "connect").mockImplementation(()
=> {});
+ const tempWorkflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ content: {
+ operators: [],
+ operatorPositions: {},
+ links: [],
+ commentBoxes: [],
+ settings: { dataTransferBatchSize: 100, executionMode:
ExecutionMode.PIPELINED },
+ },
+ };
+
+ service.setTempWorkflow(tempWorkflow);
+ expect(disconnectSpy).toHaveBeenCalled();
+ expect(service.getTempWorkflow()).toBe(tempWorkflow);
+
+ service.resetTempWorkflow();
+ expect(connectSpy).toHaveBeenCalled();
+ expect(service.getTempWorkflow()).toBeUndefined();
+ });
+
+ it("should add a dynamic input port to both the texera and joint graphs", ()
=> {
+ const dynamicOp: OperatorPredicate = {
+ ...mockMultiInputOutputPredicate,
+ dynamicInputPorts: true,
+ dynamicOutputPorts: true,
+ };
+ service.addOperator(dynamicOp, mockPoint);
+ const element = jointGraph.getCell(dynamicOp.operatorID) as
joint.dia.Element;
+ const beforeInputs =
texeraGraph.getOperator(dynamicOp.operatorID).inputPorts.length;
+ const beforePorts = element.getPorts().length;
+
+ service.addPort(dynamicOp.operatorID, true);
+
+
expect(texeraGraph.getOperator(dynamicOp.operatorID).inputPorts.length).toEqual(beforeInputs
+ 1);
+ expect(element.getPorts().length).toEqual(beforePorts + 1);
+ });
+
+ it("should honor disallowMultiInputs when adding an input port", () => {
+ const dynamicOp: OperatorPredicate = { ...mockMultiInputOutputPredicate,
dynamicInputPorts: true };
+ service.addOperator(dynamicOp, mockPoint);
+
+ service.addPort(dynamicOp.operatorID, true, true);
+
+ const inputPorts =
texeraGraph.getOperator(dynamicOp.operatorID).inputPorts;
+ expect(inputPorts[inputPorts.length - 1].disallowMultiInputs).toBe(true);
+ });
+
+ it("should remove the last dynamic port from both the texera and joint
graphs", () => {
+ const dynamicOp: OperatorPredicate = { ...mockMultiInputOutputPredicate,
dynamicInputPorts: true };
+ service.addOperator(dynamicOp, mockPoint);
+ const element = jointGraph.getCell(dynamicOp.operatorID) as
joint.dia.Element;
+ const beforeInputs =
texeraGraph.getOperator(dynamicOp.operatorID).inputPorts.length;
+ const beforePorts = element.getPorts().length;
+
+ service.removePort(dynamicOp.operatorID, true);
+
+
expect(texeraGraph.getOperator(dynamicOp.operatorID).inputPorts.length).toEqual(beforeInputs
- 1);
+ expect(element.getPorts().length).toEqual(beforePorts - 1);
+ });
Review Comment:
This test claims to remove the “last dynamic port”, but it never adds a
dynamic port first—`removePort` will just delete the last existing input port
(dynamic or not). Add a port before removal so the test actually exercises the
dynamic-port path it describes.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -277,4 +279,305 @@ describe("WorkflowActionService", () => {
// expect(sentimentOpPos).toEqual(mockPoint);
// expect(resultOpPos).toEqual(mockPoint);
});
+
+ it("should reload a workflow, repopulating the graph and clearing the
undo/redo stacks", () => {
+ const settings: WorkflowSettings = {
+ dataTransferBatchSize: 250,
+ executionMode: ExecutionMode.MATERIALIZED,
+ };
+ const workflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ name: "Reloaded WF",
+ content: {
+ operators: [mockScanPredicate, mockResultPredicate],
+ operatorPositions: {
+ [mockScanPredicate.operatorID]: mockPoint,
+ [mockResultPredicate.operatorID]: mockPoint,
+ },
+ links: [mockScanResultLink],
+ commentBoxes: [{ ...mockCommentBox, commentBoxID: "commentBox-1" }],
+ settings,
+ },
+ };
+ const clearUndoSpy = vi.spyOn(undoRedo, "clearUndoStack");
+ const clearRedoSpy = vi.spyOn(undoRedo, "clearRedoStack");
+ const restoreSpy = vi.spyOn(service.getJointGraphWrapper(),
"restoreDefaultZoomAndOffset");
+
+ service.reloadWorkflow(workflow);
+
+ expect(texeraGraph.hasOperator(mockScanPredicate.operatorID)).toBeTruthy();
+
expect(texeraGraph.hasOperator(mockResultPredicate.operatorID)).toBeTruthy();
+ expect(texeraGraph.hasLinkWithID(mockScanResultLink.linkID)).toBeTruthy();
+ expect(texeraGraph.hasCommentBox("commentBox-1")).toBeTruthy();
+ expect(service.getWorkflowMetadata().name).toEqual("Reloaded WF");
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(250);
+ expect(clearUndoSpy).toHaveBeenCalled();
+ expect(clearRedoSpy).toHaveBeenCalled();
+ expect(restoreSpy).toHaveBeenCalled();
+ });
+
+ it("should throw when a reloaded operator is missing its position", () => {
+ const workflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ content: {
+ operators: [mockScanPredicate],
+ operatorPositions: {},
+ links: [],
+ commentBoxes: [],
+ settings: { dataTransferBatchSize: 100, executionMode:
ExecutionMode.PIPELINED },
+ },
+ };
+ expect(() => service.reloadWorkflow(workflow)).toThrowError(
+ new RegExp(`position error: ${mockScanPredicate.operatorID}`)
+ );
+ });
+
+ it("should empty the graph and skip viewport restore when reloading
undefined", () => {
+ service.addOperator(mockScanPredicate, mockPoint);
+ const restoreSpy = vi.spyOn(service.getJointGraphWrapper(),
"restoreDefaultZoomAndOffset");
+
+ service.reloadWorkflow(undefined, false, false);
+
+ expect(texeraGraph.getAllOperators().length).toEqual(0);
+ expect(restoreSpy).not.toHaveBeenCalled();
+ });
+
+ it("should clear the workflow back to its defaults", () => {
+ service.addOperator(mockScanPredicate, mockPoint);
+ service.setWorkflowName("Something");
+ service.setWorkflowDataTransferBatchSize(999);
+ service.setHighlightingEnabled(true);
+
+ service.clearWorkflow();
+
+ expect(service.getWorkflowMetadata()).toBe(DEFAULT_WORKFLOW);
Review Comment:
Using `toBe(DEFAULT_WORKFLOW)` asserts reference identity, which makes this
test brittle (a future refactor could return an equivalent object rather than
the exact constant and this would fail without a behavior change). Prefer a
value comparison here.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -277,4 +279,305 @@ describe("WorkflowActionService", () => {
// expect(sentimentOpPos).toEqual(mockPoint);
// expect(resultOpPos).toEqual(mockPoint);
});
+
+ it("should reload a workflow, repopulating the graph and clearing the
undo/redo stacks", () => {
+ const settings: WorkflowSettings = {
+ dataTransferBatchSize: 250,
+ executionMode: ExecutionMode.MATERIALIZED,
+ };
+ const workflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ name: "Reloaded WF",
+ content: {
+ operators: [mockScanPredicate, mockResultPredicate],
+ operatorPositions: {
+ [mockScanPredicate.operatorID]: mockPoint,
+ [mockResultPredicate.operatorID]: mockPoint,
+ },
+ links: [mockScanResultLink],
+ commentBoxes: [{ ...mockCommentBox, commentBoxID: "commentBox-1" }],
+ settings,
+ },
+ };
+ const clearUndoSpy = vi.spyOn(undoRedo, "clearUndoStack");
+ const clearRedoSpy = vi.spyOn(undoRedo, "clearRedoStack");
+ const restoreSpy = vi.spyOn(service.getJointGraphWrapper(),
"restoreDefaultZoomAndOffset");
+
+ service.reloadWorkflow(workflow);
+
+ expect(texeraGraph.hasOperator(mockScanPredicate.operatorID)).toBeTruthy();
+
expect(texeraGraph.hasOperator(mockResultPredicate.operatorID)).toBeTruthy();
+ expect(texeraGraph.hasLinkWithID(mockScanResultLink.linkID)).toBeTruthy();
+ expect(texeraGraph.hasCommentBox("commentBox-1")).toBeTruthy();
+ expect(service.getWorkflowMetadata().name).toEqual("Reloaded WF");
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(250);
+ expect(clearUndoSpy).toHaveBeenCalled();
+ expect(clearRedoSpy).toHaveBeenCalled();
+ expect(restoreSpy).toHaveBeenCalled();
+ });
+
+ it("should throw when a reloaded operator is missing its position", () => {
+ const workflow: Workflow = {
+ ...DEFAULT_WORKFLOW,
+ content: {
+ operators: [mockScanPredicate],
+ operatorPositions: {},
+ links: [],
+ commentBoxes: [],
+ settings: { dataTransferBatchSize: 100, executionMode:
ExecutionMode.PIPELINED },
+ },
+ };
+ expect(() => service.reloadWorkflow(workflow)).toThrowError(
+ new RegExp(`position error: ${mockScanPredicate.operatorID}`)
+ );
+ });
+
+ it("should empty the graph and skip viewport restore when reloading
undefined", () => {
+ service.addOperator(mockScanPredicate, mockPoint);
+ const restoreSpy = vi.spyOn(service.getJointGraphWrapper(),
"restoreDefaultZoomAndOffset");
+
+ service.reloadWorkflow(undefined, false, false);
+
+ expect(texeraGraph.getAllOperators().length).toEqual(0);
+ expect(restoreSpy).not.toHaveBeenCalled();
+ });
+
+ it("should clear the workflow back to its defaults", () => {
+ service.addOperator(mockScanPredicate, mockPoint);
+ service.setWorkflowName("Something");
+ service.setWorkflowDataTransferBatchSize(999);
+ service.setHighlightingEnabled(true);
+
+ service.clearWorkflow();
+
+ expect(service.getWorkflowMetadata()).toBe(DEFAULT_WORKFLOW);
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(100);
+
expect(service.getWorkflowSettings().executionMode).toEqual(ExecutionMode.PIPELINED);
+ expect(texeraGraph.getAllOperators().length).toEqual(0);
+ expect(service.getHighlightingEnabled()).toBeFalsy();
+ });
+
+ it("should emit on workflowChanged when resetting as a new workflow", () => {
+ let changed = false;
+ service.workflowChanged().subscribe(() => (changed = true));
+
+ service.resetAsNewWorkflow();
+
+ expect(changed).toBeTruthy();
+ });
+
+ it("should only emit metadata changes when the reference changes", () => {
+ const current = service.getWorkflowMetadata();
+ let emitCount = 0;
+ service.workflowMetaDataChanged().subscribe(() => (emitCount += 1));
+
+ // identical reference -> no emission
+ service.setWorkflowMetadata(current);
+ expect(emitCount).toEqual(0);
+
+ // new metadata -> emission
+ service.setWorkflowMetadata({ ...DEFAULT_WORKFLOW, name: "renamed" });
+ expect(emitCount).toEqual(1);
+ expect(service.getWorkflowMetadata().name).toEqual("renamed");
+ });
+
+ it("should set the workflow name, defaulting blank names", () => {
+ service.setWorkflowName(" ");
+ expect(service.getWorkflowMetadata().name).toEqual(DEFAULT_WORKFLOW_NAME);
+
+ service.setWorkflowName("My Workflow");
+ expect(service.getWorkflowMetadata().name).toEqual("My Workflow");
+ });
+
+ it("should manage workflow settings and publish state", () => {
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(100);
+
expect(service.getWorkflowSettings().executionMode).toEqual(ExecutionMode.PIPELINED);
+
+ // non-positive batch sizes are ignored, positive ones are applied
+ service.setWorkflowDataTransferBatchSize(0);
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(100);
+ service.setWorkflowDataTransferBatchSize(400);
+ expect(service.getWorkflowSettings().dataTransferBatchSize).toEqual(400);
+
+ service.updateExecutionMode(ExecutionMode.MATERIALIZED);
+
expect(service.getWorkflowSettings().executionMode).toEqual(ExecutionMode.MATERIALIZED);
+
+ service.setWorkflowIsPublished(1);
+ expect(service.getWorkflowMetadata().isPublished).toEqual(1);
+
+ const settings: WorkflowSettings = { dataTransferBatchSize: 42,
executionMode: ExecutionMode.PIPELINED };
+ service.setWorkflowSettings(settings);
+ expect(service.getWorkflowSettings()).toEqual(settings);
+ });
+
+ it("should toggle the shared-editing connection through temp workflow", ()
=> {
+ const wsProvider = texeraGraph.sharedModel.wsProvider;
+ const disconnectSpy = vi.spyOn(wsProvider,
"disconnect").mockImplementation(() => {});
+ const connectSpy = vi.spyOn(wsProvider, "connect").mockImplementation(()
=> {});
Review Comment:
This test assumes `setTempWorkflow` always disconnects, but the
implementation only calls `disconnect()` when `wsProvider.shouldConnect` is
true. Make the test deterministic by forcing `shouldConnect` to true before
exercising the behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
