Copilot commented on code in PR #4971:
URL: https://github.com/apache/texera/pull/4971#discussion_r3193990661
##########
frontend/src/app/workspace/service/drag-drop/drag-drop.service.spec.ts:
##########
@@ -155,81 +159,89 @@ describe("DragDropService", () => {
expect(inputOps).toEqual([]);
});
- // Same root cause as the skipped test above — link inference depends on
- // findClosestOperators returning real geometry. Tracked in #4866.
- it.skip(
- "should update highlighting, add operator, and add links when an operator
is dropped",
- marbles(async () => {
- const workflowActionService: WorkflowActionService =
TestBed.inject(WorkflowActionService);
- const workflowUtilService: WorkflowUtilService =
TestBed.inject(WorkflowUtilService);
- workflowActionService.getJointGraphWrapper();
- const operatorType = "MultiInputOutput";
- const operator = mockMultiInputOutputPredicate;
- const input1 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const input2 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const input3 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const output1 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const output2 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const output3 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const heightSortedInputs: OperatorPredicate[] = [input1, input2, input3];
- const heightSortedOutputs: OperatorPredicate[] = [output1, output2,
output3];
-
- // lists to be populated by observables/streams
- const highlights: string[] = [];
- const unhighlights: string[] = [];
- const links: OperatorLink[] = [];
- // expected end results of above lists
- const expectedHighlights: OperatorPredicate[] = []; // expected empty
- const expectedUnhighlights = [
+ it("should add the dropped operator with links to suggested neighbors and
unhighlight prior suggestions", async () => {
+ const workflowActionService: WorkflowActionService =
TestBed.inject(WorkflowActionService);
+ const workflowUtilService: WorkflowUtilService =
TestBed.inject(WorkflowUtilService);
+ const input1 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const input2 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const input3 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const output1 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+ const output2 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+ const output3 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+
+ // Real main jointjs paper attached to a hidden DOM host so coordinate
+ // transforms in `dragStarted` / mousemove / `dragDropped` resolve without
+ // stubs. jsdom doesn't compute real layout, but the SVG polyfill returns
+ // identity transforms so `pageToLocalPoint(x, y)` ≈ (x, y) — fine for
+ // this test which doesn't assert on the dropped operator's position.
Review Comment:
The comment about jsdom/SVG polyfills is internally inconsistent: it says
`pageToLocalPoint(x, y)` is approximately `(x, y)`, but later the test relies
on `pageToLocalPoint` always returning `(0, 0)`. Given the repo’s
`jsdom-svg-polyfill` stubs `createSVGPoint().matrixTransform()` to return
`{x:0,y:0}`, `pageToLocalPoint` will effectively collapse to `(0,0)`. Please
update the earlier comment to match the actual behavior so future readers don’t
misinterpret why the x=-100/+100 placement is necessary.
##########
frontend/src/app/workspace/service/drag-drop/drag-drop.service.spec.ts:
##########
@@ -155,81 +159,89 @@ describe("DragDropService", () => {
expect(inputOps).toEqual([]);
});
- // Same root cause as the skipped test above — link inference depends on
- // findClosestOperators returning real geometry. Tracked in #4866.
- it.skip(
- "should update highlighting, add operator, and add links when an operator
is dropped",
- marbles(async () => {
- const workflowActionService: WorkflowActionService =
TestBed.inject(WorkflowActionService);
- const workflowUtilService: WorkflowUtilService =
TestBed.inject(WorkflowUtilService);
- workflowActionService.getJointGraphWrapper();
- const operatorType = "MultiInputOutput";
- const operator = mockMultiInputOutputPredicate;
- const input1 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const input2 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const input3 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const output1 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const output2 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const output3 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const heightSortedInputs: OperatorPredicate[] = [input1, input2, input3];
- const heightSortedOutputs: OperatorPredicate[] = [output1, output2,
output3];
-
- // lists to be populated by observables/streams
- const highlights: string[] = [];
- const unhighlights: string[] = [];
- const links: OperatorLink[] = [];
- // expected end results of above lists
- const expectedHighlights: OperatorPredicate[] = []; // expected empty
- const expectedUnhighlights = [
+ it("should add the dropped operator with links to suggested neighbors and
unhighlight prior suggestions", async () => {
+ const workflowActionService: WorkflowActionService =
TestBed.inject(WorkflowActionService);
+ const workflowUtilService: WorkflowUtilService =
TestBed.inject(WorkflowUtilService);
+ const input1 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const input2 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const input3 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const output1 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+ const output2 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+ const output3 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+
+ // Real main jointjs paper attached to a hidden DOM host so coordinate
+ // transforms in `dragStarted` / mousemove / `dragDropped` resolve without
+ // stubs. jsdom doesn't compute real layout, but the SVG polyfill returns
+ // identity transforms so `pageToLocalPoint(x, y)` ≈ (x, y) — fine for
+ // this test which doesn't assert on the dropped operator's position.
+ const paperHost = document.createElement("div");
+ document.body.appendChild(paperHost);
+ workflowActionService.getJointGraphWrapper().attachMainJointPaper({ el:
paperHost });
+
+ // `dragStarted` mounts a "ghost" jointjs paper in this DOM node.
+ const flyingOpHost = document.createElement("div");
+ flyingOpHost.id = "flyingOP";
+ document.body.appendChild(flyingOpHost);
+
+ // jsdom doesn't compute layout, so the paper's `pageToLocalPoint` always
+ // returns (0, 0) regardless of the dispatched mouse position. Place the
+ // input operators at negative x and outputs at positive x so the
+ // origin-at-(0,0) drop point classifies them correctly via
+ // `findClosestOperators` (which compares operator x against mouse x).
+ workflowActionService.addOperator(input1, { x: -100, y: 10 });
+ workflowActionService.addOperator(input2, { x: -100, y: 20 });
+ workflowActionService.addOperator(input3, { x: -100, y: 30 });
+ workflowActionService.addOperator(output1, { x: 100, y: 10 });
+ workflowActionService.addOperator(output2, { x: 100, y: 20 });
+ workflowActionService.addOperator(output3, { x: 100, y: 30 });
+
+ const unhighlights: string[] = [];
+ dragDropService.getOperatorSuggestionUnhighlightStream().subscribe(id =>
unhighlights.push(id));
+ const links: OperatorLink[] = [];
+ workflowActionService
+ .getTexeraGraph()
+ .getLinkAddStream()
+ .subscribe(link => links.push(link));
+
+ // dragStarted creates a fresh `op` of the given type and subscribes to
+ // window mousemove to populate suggestionInputs / suggestionOutputs.
+ dragDropService.dragStarted("MultiInputOutput");
+ const droppedOp = (dragDropService as any).op as OperatorPredicate;
+
+ // Drive the suggestion pipeline. Any mousemove will do — jsdom's
+ // `pageToLocalPoint` always resolves to (0, 0) since it has no layout,
+ // and we placed inputs/outputs around the origin to match.
+ window.dispatchEvent(new MouseEvent("mousemove", { clientX: 0, clientY: 0
}));
+ await new Promise(resolve => setTimeout(resolve, 0));
+
+ dragDropService.dragDropped({ x: 0, y: 0 });
Review Comment:
`dragStarted()` installs window `mousemove` subscriptions that only stop
processing after a `mouseup` sets `isOperatorDropped = true`. This test never
dispatches a `mouseup`, so the subscriptions remain active after
`dragDropped()` and can react to mouse events from other specs, potentially
causing cross-test interference. After `dragDropped`, dispatch a synthetic
`mouseup` (or otherwise terminate the drag recommendation pipeline) before
cleaning up the DOM.
##########
frontend/src/app/workspace/service/drag-drop/drag-drop.service.spec.ts:
##########
@@ -155,81 +159,89 @@ describe("DragDropService", () => {
expect(inputOps).toEqual([]);
});
- // Same root cause as the skipped test above — link inference depends on
- // findClosestOperators returning real geometry. Tracked in #4866.
- it.skip(
- "should update highlighting, add operator, and add links when an operator
is dropped",
- marbles(async () => {
- const workflowActionService: WorkflowActionService =
TestBed.inject(WorkflowActionService);
- const workflowUtilService: WorkflowUtilService =
TestBed.inject(WorkflowUtilService);
- workflowActionService.getJointGraphWrapper();
- const operatorType = "MultiInputOutput";
- const operator = mockMultiInputOutputPredicate;
- const input1 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const input2 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const input3 = workflowUtilService.getNewOperatorPredicate("ScanSource");
- const output1 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const output2 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const output3 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
- const heightSortedInputs: OperatorPredicate[] = [input1, input2, input3];
- const heightSortedOutputs: OperatorPredicate[] = [output1, output2,
output3];
-
- // lists to be populated by observables/streams
- const highlights: string[] = [];
- const unhighlights: string[] = [];
- const links: OperatorLink[] = [];
- // expected end results of above lists
- const expectedHighlights: OperatorPredicate[] = []; // expected empty
- const expectedUnhighlights = [
+ it("should add the dropped operator with links to suggested neighbors and
unhighlight prior suggestions", async () => {
+ const workflowActionService: WorkflowActionService =
TestBed.inject(WorkflowActionService);
+ const workflowUtilService: WorkflowUtilService =
TestBed.inject(WorkflowUtilService);
+ const input1 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const input2 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const input3 = workflowUtilService.getNewOperatorPredicate("ScanSource");
+ const output1 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+ const output2 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+ const output3 =
workflowUtilService.getNewOperatorPredicate(VIEW_RESULT_OP_TYPE);
+
+ // Real main jointjs paper attached to a hidden DOM host so coordinate
+ // transforms in `dragStarted` / mousemove / `dragDropped` resolve without
+ // stubs. jsdom doesn't compute real layout, but the SVG polyfill returns
+ // identity transforms so `pageToLocalPoint(x, y)` ≈ (x, y) — fine for
+ // this test which doesn't assert on the dropped operator's position.
+ const paperHost = document.createElement("div");
+ document.body.appendChild(paperHost);
+ workflowActionService.getJointGraphWrapper().attachMainJointPaper({ el:
paperHost });
+
+ // `dragStarted` mounts a "ghost" jointjs paper in this DOM node.
+ const flyingOpHost = document.createElement("div");
+ flyingOpHost.id = "flyingOP";
+ document.body.appendChild(flyingOpHost);
+
Review Comment:
This test appends `paperHost` and `flyingOpHost` to `document.body`, but
cleanup happens only at the very end. If any assertion throws before lines
244-245, these DOM nodes (and the JointJS paper attached to them) will leak
into later tests and can cause flakes. Consider wrapping the body in
`try/finally` (or using `afterEach`) to guarantee DOM cleanup even on failure.
--
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]
