Xiao-zhen-Liu commented on code in PR #5146:
URL: https://github.com/apache/texera/pull/5146#discussion_r3320350405
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -232,99 +233,107 @@ describe("WorkflowEditorComponent", () => {
fixture.detectChanges();
});
- it("should try to highlight the operator when user mouse clicks on an
operator", () => {
- const jointGraphWrapper = workflowActionService.getJointGraphWrapper();
- // install a spy on the highlight operator function and pass the call
through
- vi.spyOn(jointGraphWrapper, "highlightOperators");
- workflowActionService.addOperator(mockScanPredicate, mockPoint);
-
- // unhighlight the operator in case it's automatically highlighted
- jointGraphWrapper.unhighlightOperators(mockScanPredicate.operatorID);
-
- // find the joint Cell View object of the operator element
- const jointCellView =
component.paper.findViewByModel(mockScanPredicate.operatorID);
- jointCellView.$el.trigger("mousedown");
-
- fixture.detectChanges();
-
- // assert the function is called once
- // expect(highlightOperatorFunctionSpy.calls.count()).toEqual(1);
- // assert the highlighted operator is correct
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toEqual([mockScanPredicate.operatorID]);
- });
-
- it("should highlight the commentBox when user clicks on a commentBox", ()
=> {
- const jointGraphWrapper = workflowActionService.getJointGraphWrapper();
- vi.spyOn(jointGraphWrapper, "highlightCommentBoxes");
- workflowActionService.addCommentBox(mockCommentBox);
- jointGraphWrapper.unhighlightCommentBoxes(mockCommentBox.commentBoxID);
- const jointCellView =
component.paper.findViewByModel(mockCommentBox.commentBoxID);
- jointCellView.$el.trigger("mousedown");
- fixture.detectChanges();
-
expect(jointGraphWrapper.getCurrentHighlightedCommentBoxIDs()).toEqual([mockCommentBox.commentBoxID]);
- });
-
- it("should open commentBox as NzModal when user double clicks on a
commentBox", () => {
- const modalRef: NzModalRef = nzModalService.create({
- nzTitle: "CommentBox",
- nzContent: NzModalCommentBoxComponent,
- nzData: { commentBox: createYTypeFromObject(mockCommentBox) },
- nzAutofocus: null,
- nzFooter: [
- {
- label: "OK",
- onClick: () => {
- modalRef.destroy();
- },
- type: "primary",
- },
- ],
- });
- vi.spyOn(nzModalService, "create").mockReturnValue(modalRef);
- const jointGraphWrapper = workflowActionService.getJointGraphWrapper();
- workflowActionService.addCommentBox(mockCommentBox);
- jointGraphWrapper.highlightCommentBoxes(mockCommentBox.commentBoxID);
- const jointCellView =
component.paper.findViewByModel(mockCommentBox.commentBoxID);
- jointCellView.$el.trigger("dblclick");
- expect(nzModalService.create).toHaveBeenCalled();
- fixture.detectChanges();
- modalRef.destroy();
- });
-
- it("should unhighlight all highlighted operators when user mouse clicks on
the blank space", () => {
- const jointGraphWrapper = workflowActionService.getJointGraphWrapper();
-
- // add and highlight two operators
- workflowActionService.addOperatorsAndLinks(
- [
- { op: mockScanPredicate, pos: mockPoint },
- { op: mockResultPredicate, pos: mockPoint },
- ],
- []
- );
- jointGraphWrapper.highlightOperators(mockScanPredicate.operatorID,
mockResultPredicate.operatorID);
-
- // assert that both operators are highlighted
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockScanPredicate.operatorID);
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockResultPredicate.operatorID);
-
- // find a blank area on the JointJS paper
- const blankPoint = { x: mockPoint.x + 100, y: mockPoint.y + 100 };
- expect(component.paper.findViewsFromPoint(blankPoint)).toEqual([]);
-
- // trigger a click on the blank area using JointJS paper's jQuery element
- const point = component.paper.localToClientPoint(blankPoint);
- const event = createJQueryEvent("mousedown", {
- clientX: point.x,
- clientY: point.y,
- });
- component.paper.$el.trigger(event);
-
- fixture.detectChanges();
-
- // assert that all operators are unhighlighted
- expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toEqual([]);
- });
+ // TODO(#3614): the following four mouse/click-event tests rely on JointJS
Review Comment:
These 6 tests are the browser-only coverage for this file, and the
`test-browser` target (`angular.json:105`) runs *only* this spec. Since they're
commented out *here*, they no longer run under `ng run gui:test-browser` either
— the PR description's claim that they "continue to pass" there isn't accurate;
commenting them out drops them from every target.
Prefer `it.skip(...)`/`xit(...)` so they still appear as explicitly skipped
(discoverable, not silently lost), or better, a runtime guard that skips under
jsdom but executes under the real-Chrome `test-browser` runner — that keeps the
coverage the browser target exists to provide. ~150 lines of commented-out test
code will rot quickly and a follow-up PR to "revive" them rarely happens.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -844,52 +853,56 @@ describe("WorkflowEditorComponent", () => {
// }
// });
- it("should highlight multiple operators when user clicks on them with
shift key pressed", () => {
- const jointGraphWrapper = workflowActionService.getJointGraphWrapper();
-
- workflowActionService.addOperator(mockScanPredicate, mockPoint);
- workflowActionService.addOperator(mockResultPredicate, mockPoint);
- jointGraphWrapper.highlightOperators(mockResultPredicate.operatorID);
-
- // assert that only the last operator is highlighted
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockResultPredicate.operatorID);
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).not.toContain(mockScanPredicate.operatorID);
-
- // find the joint Cell View object of the first operator element
- const jointCellView =
component.paper.findViewByModel(mockScanPredicate.operatorID);
-
- // trigger a shift click on the cell view using its jQuery element
- const event = createJQueryEvent("mousedown", { shiftKey: true });
- jointCellView.$el.trigger(event);
-
- fixture.detectChanges();
-
- // assert that both operators are highlighted
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockScanPredicate.operatorID);
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockResultPredicate.operatorID);
- });
-
- it("should unhighlight the highlighted operator when user clicks on it
with shift key pressed", () => {
- const jointGraphWrapper = workflowActionService.getJointGraphWrapper();
-
- workflowActionService.addOperator(mockScanPredicate, mockPoint);
- jointGraphWrapper.highlightOperators(mockScanPredicate.operatorID);
-
- // assert that the operator is highlighted
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockScanPredicate.operatorID);
-
- // find the joint Cell View object of the operator element
- const jointCellView =
component.paper.findViewByModel(mockScanPredicate.operatorID);
-
- // trigger a shift click on the cell view using its jQuery element
- const event = createJQueryEvent("mousedown", { shiftKey: true });
- jointCellView.$el.trigger(event);
-
- fixture.detectChanges();
-
- // assert that the operator is unhighlighted
-
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).not.toContain(mockScanPredicate.operatorID);
- });
+ // TODO(#3614): the next two shift-click multi-select tests also depend on
Review Comment:
Same concern as the earlier commented-out block — these shift-click tests
are also removed from every target, including `test-browser`. `it.skip`/`xit`
or an env-guard would keep them visible and reviveable.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -358,6 +358,28 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
});
+
+ // When operators are (re)added to the graph — e.g. after navigating back
to
+ // the workflow page, where WorkspaceComponent calls reloadWorkflow and
+ // operators are recreated from the workflow JSON — restore their visual
+ // state from the cached status so completed runs don't appear to reset.
+ this.workflowActionService
Review Comment:
This new subscription and `handleOperatorValidation` below now both read
`workflowStatusService.getCurrentStatus()` and paint the execution-state
border. That's duplicated "read cache → repaint" logic across two handlers, and
the root problem (two streams racing to write `rect.body/stroke`) isn't removed
— it's just made consistent for the valid+cached case. A single helper that
resolves the final border from (validation result, cached state) and is invoked
from one place would be cleaner and order-independent.
Minor: a `// eslint`-style nit aside, this restores port counts/worker count
(which validation doesn't), so the subscription is genuinely needed — it's the
border-stroke overlap with validation that's the smell.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -949,15 +971,31 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
/**
- * if the operator is valid , the border of the box will be default
+ * Applies the validation result to the operator's border.
+ * - Invalid operators are drawn with a red border (validation takes
priority).
+ * - Valid operators with a cached execution status keep their
execution-state
+ * border so e.g. a Completed (green) run isn't repainted gray when the
user
+ * navigates back to the workflow and reloadWorkflow re-adds the operators,
+ * which triggers a validation pass that would otherwise overwrite the
+ * execution-state stroke set by handleOperatorStatisticsUpdate.
+ * - Valid operators with no cached status get the default valid border.
*/
private handleOperatorValidation(): void {
this.validationWorkflowService
.getOperatorValidationStream()
.pipe(untilDestroyed(this))
- .subscribe(value =>
- this.jointUIService.changeOperatorColor(this.paper, value.operatorID,
value.validation.isValid)
- );
+ .subscribe(value => {
+ if (!value.validation.isValid) {
+ this.jointUIService.changeOperatorColor(this.paper,
value.operatorID, false);
+ return;
+ }
+ const statistics =
this.workflowStatusService.getCurrentStatus()[value.operatorID];
+ if (statistics) {
+ this.jointUIService.changeOperatorState(this.paper,
value.operatorID, statistics.operatorState);
Review Comment:
Correctness/fragility: the invalid + cached path is order-dependent. The
operator-add subscription above paints the cached *state* color (e.g. green)
via `changeOperatorStatistics`, and this handler paints red. The red only
"wins" if the validation stream emits *after* the add-stream handler — but they
reach here via different event chains (add-stream → component subscription vs.
operator-add → `dynamicSchemaService` → validation). Nothing guarantees that
ordering. It's rare in practice (an invalid operator rarely has cached stats),
but if validation ever fires first, the green paint would overwrite the red
border. Computing the final stroke in one resolver (validation takes priority,
else cached state, else default) would remove the dependency on emission order.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -962,5 +975,75 @@ describe("WorkflowEditorComponent", () => {
fixture.detectChanges();
expect(redoSpy).toHaveBeenCalledTimes(4);
});
+
+ /**
+ * Regression coverage for the bug where the operator border resets to the
+ * default (gray) when the user navigates away from and back to a workflow
+ * that has already finished executing. The visual state is driven by two
+ * separate streams that both touch rect.body/stroke: the execution status
+ * stream (sets state-derived color) and the validation stream (sets red on
+ * invalid, gray on valid). When operators are re-added by reloadWorkflow,
+ * the validation pass fires after the status repaint and used to overwrite
+ * it. handleOperatorValidation now consults the cached status before
+ * deciding which color to apply.
+ */
+ describe("operator border restoration after navigation", () => {
+ let workflowStatusService: WorkflowStatusService;
+ const cachedCompleted = {
+ [mockScanPredicate.operatorID]: {
+ operatorState: OperatorState.Completed,
+ aggregatedInputRowCount: 0,
+ inputPortMetrics: {},
+ aggregatedOutputRowCount: 0,
+ outputPortMetrics: {},
+ },
+ };
+
+ beforeEach(() => {
+ workflowStatusService = TestBed.inject(WorkflowStatusService);
+ });
+
+ it("repaints execution-state stroke for valid operators with a cached
status", () => {
Review Comment:
This test doesn't isolate the change it's meant to cover.
`changeOperatorState(..., Completed)` is *also* called by the operator-add
subscription via `changeOperatorStatistics`, so this assertion passes even if
`handleOperatorValidation` were reverted to the old behavior. To actually
exercise the validation branch, either stub/disable the add-stream path or
assert the final `rect.body/stroke` attribute on the model (the visible
outcome) rather than that a method was called.
##########
frontend/angular.json:
##########
@@ -92,10 +92,7 @@
"runnerConfig": "vitest.config.ts",
"tsConfig": "src/tsconfig.spec.json",
"include": ["**/*.spec.ts"],
- "setupFiles": ["src/jsdom-svg-polyfill.ts"],
- "exclude": [
-
"**/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts"
- ]
+ "setupFiles": ["src/jsdom-svg-polyfill.ts"]
Review Comment:
Decomposability: un-excluding this spec from the default jsdom target is a
CI/test-infra change that is logically independent of the operator-border bug
fix. Bundling them couples a behavior fix to a test-config change, making the
PR harder to review and revert as a unit. Consider splitting the
test-enablement into its own PR.
Also note `test-browser` (line 105 below) includes *only* this spec — see
the comment on the commented-out tests in the spec file for why that matters.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -962,5 +975,75 @@ describe("WorkflowEditorComponent", () => {
fixture.detectChanges();
expect(redoSpy).toHaveBeenCalledTimes(4);
});
+
+ /**
+ * Regression coverage for the bug where the operator border resets to the
+ * default (gray) when the user navigates away from and back to a workflow
+ * that has already finished executing. The visual state is driven by two
+ * separate streams that both touch rect.body/stroke: the execution status
+ * stream (sets state-derived color) and the validation stream (sets red on
+ * invalid, gray on valid). When operators are re-added by reloadWorkflow,
+ * the validation pass fires after the status repaint and used to overwrite
+ * it. handleOperatorValidation now consults the cached status before
+ * deciding which color to apply.
+ */
+ describe("operator border restoration after navigation", () => {
+ let workflowStatusService: WorkflowStatusService;
+ const cachedCompleted = {
+ [mockScanPredicate.operatorID]: {
+ operatorState: OperatorState.Completed,
+ aggregatedInputRowCount: 0,
+ inputPortMetrics: {},
+ aggregatedOutputRowCount: 0,
+ outputPortMetrics: {},
+ },
+ };
+
+ beforeEach(() => {
+ workflowStatusService = TestBed.inject(WorkflowStatusService);
+ });
+
+ it("repaints execution-state stroke for valid operators with a cached
status", () => {
+ vi.spyOn(workflowStatusService,
"getCurrentStatus").mockReturnValue(cachedCompleted);
+ vi.spyOn(validationWorkflowService,
"validateOperator").mockReturnValue({ isValid: true });
+ const changeOperatorStateSpy = vi.spyOn(jointUIService,
"changeOperatorState");
+
+ workflowActionService.addOperator(mockScanPredicate, mockPoint);
+ fixture.detectChanges();
+
+ expect(changeOperatorStateSpy).toHaveBeenCalledWith(
+ component.paper,
+ mockScanPredicate.operatorID,
+ OperatorState.Completed
+ );
+ });
+
+ it("falls back to the default valid stroke when no cached status
exists", () => {
+ vi.spyOn(workflowStatusService,
"getCurrentStatus").mockReturnValue({});
+ vi.spyOn(validationWorkflowService,
"validateOperator").mockReturnValue({ isValid: true });
+ const changeOperatorColorSpy = vi.spyOn(jointUIService,
"changeOperatorColor");
+
+ workflowActionService.addOperator(mockScanPredicate, mockPoint);
+ fixture.detectChanges();
+
+ expect(changeOperatorColorSpy).toHaveBeenCalledWith(component.paper,
mockScanPredicate.operatorID, true);
+ });
+
+ it("paints the invalid stroke (red) for invalid operators regardless of
cached status", () => {
Review Comment:
This asserts `changeOperatorColor(..., false)` was *called*, but not that
red is the *final/winning* paint — and per the note on
`handleOperatorValidation`, that outcome is order-dependent. The test even
acknowledges in its comment that the add-stream still paints green first.
Asserting the resulting `rect.body/stroke` === `red` on the model would catch a
regression where the green statistics paint lands last; the current assertion
would not.
--
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]
