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-7993-2b8138118d8e56eee6779f8b06bcc416a77e9689 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 57a4230b6f744ba009aa0099893da825999f3077 Author: Xinyuan Lin <[email protected]> AuthorDate: Thu Aug 27 01:31:34 2026 +0000 test(frontend): cover the result, export and runtime-statistics services (#7993) ### What changes were proposed in this PR? Four frontend services and one component, bundled because each is only ~3 lines alone. Measured with the CI command itself (`nx test --coverage --coverage-reporters=lcovonly`), before state restored per-file via `git show HEAD:<path>`. **+11 fully-covered lines and +10 branch arms.** Three of the four files reach 100%; `workflow-result.service.ts` reaches 99.15%. **Of those +11, nine are reachable from the real UI and two are defensive defaults on public methods.** I would rather split them than present all eleven as equivalent. The weaker two: `workflow-runtime-statistics.component.ts:137` is an out-of-range tab index that the template cannot produce — it renders exactly 8 tabs for 8 metric keys — but it *is* a public method reachable with a legal argument, so it counts rather than being refused. **`user-dataset-file-renderer.component.ts` was in scope and is absent**, contributing zero: two dead `??`/`||` fallbacks already gated by `isPreviewSupported`, plus an unreachable partial. `workflow-action.service.ts` was excluded from the outset for the same reason — its three partials compare a `Y.Map` value against a freshly-constructed `{x, y}` by reference, so the guard cannot fire in production. ### A dead-code defect found while assessing, reported not pinned The empty-row filter in `user-dataset-file-renderer`'s `loadTabularFile` is **entirely dead**. `for (const cell in row)` enumerates array *index strings* — `"0"`, `"1"`, … — never `""`, so `cell != ""` is always true, `areCellAllEmpty` is always false, and no row is ever filtered. Verified directly in node: a table containing an all-empty row and an empty row keeps both, with the empty one padded out — contradicting the code's own "filter out all empty row" comment. Fixing it means iterating values with `for...of`, which is a production change. The current behaviour is deliberately **not** pinned, so a fix will not have to fight a test. ### Verification 22 mutations, **21 killed, 1 equivalent survivor** (`workflow-result.service.ts:222`, an exhaustive-union arm whose branch body is unreachable). The first draft claimed `survivors: []` on `ui-udf-parameters-sync.service.ts` at "100.00% Codecov". The percentage was metric-true — independently reproduced at 54/54 — but the file carried **two real holes** behind it. That is the pattern worth naming: a file can be fully executed and still barely constrained. Two further corrections: the bundle presented all +11 lines as equal in quality (split above), and one uniqueness claim was accurate as far as it went but incomplete about which sibling tests also failed. ### Deliberately not included Three arms in the dropped renderer are unreachable, and any mutation confined to them is equivalent by construction — so none was attempted rather than being reported as a survivor. Two constraints shaped what was possible here, both worth recording. `@angular/build`'s unit-test runner hard-codes `isolate: false`, so every spec shares one module registry: that rules out mutating an exported const at test time (it would leak into every other importer), and it is why no `vi.mock` for Plotly was added to the runtime-statistics spec — per #6580 that pattern is green solo and red on CI. The existing mock-free real-Plotly pattern is used instead. No production file is touched. ### Any related issues, documentation, discussions? Closes #7991 ### How was this PR tested? ``` npx ng test --watch=false --include="**/ui-udf-parameters-sync.service.spec.ts" --include="**/workflow-result.service.spec.ts" --include="**/workflow-result-export.service.spec.ts" --include="**/workflow-runtime-statistics.component.spec.ts" ``` ``` Test Files 4 passed (4) ``` `yarn format:ci` passes. `frontend/junit.xml` and `frontend/coverage/` are regenerated by every run and are not committed; the jsdom `getContext not implemented` noise from Monaco is pre-existing. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> --- .../workflow-runtime-statistics.component.spec.ts | 51 +++++++++++ .../ui-udf-parameters-sync.service.spec.ts | 63 +++++++++++++ .../workflow-result-export.service.spec.ts | 101 +++++++++++++++++++++ .../workflow-result.service.spec.ts | 74 ++++++++++++++- 4 files changed, 288 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-runtime-statistics/workflow-runtime-statistics.component.spec.ts b/frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-runtime-statistics/workflow-runtime-statistics.component.spec.ts index b4a9b6dd71..12e3647d41 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-runtime-statistics/workflow-runtime-statistics.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-runtime-statistics/workflow-runtime-statistics.component.spec.ts @@ -68,12 +68,17 @@ describe("WorkflowRuntimeStatisticsComponent", () => { // Two operators, the first appearing twice, so grouping/relative-time behavior is observable. // initialTimestamp = 1000 (first stat) => relative timestamps 0, 2000, 1000. + // Every metric the scan operator carries gets a DISTINCT value, so no two metric keys produce the + // same y-series and the metric-index -> metric-key mapping cannot be silently permuted. function validStats(): WorkflowRuntimeStatistics[] { return [ makeStat({ operatorId: "scan-op-111111", timestamp: 1000, inputTupleCount: 10, + inputTupleSize: 11, + outputTupleCount: 12, + outputTupleSize: 13, totalDataProcessingTime: 2 * NANOS, totalControlProcessingTime: 3 * NANOS, totalIdleTime: 4 * NANOS, @@ -83,7 +88,12 @@ describe("WorkflowRuntimeStatisticsComponent", () => { operatorId: "scan-op-111111", timestamp: 3000, inputTupleCount: 20, + inputTupleSize: 21, + outputTupleCount: 22, + outputTupleSize: 23, totalDataProcessingTime: 5 * NANOS, + totalControlProcessingTime: 6 * NANOS, + totalIdleTime: 7 * NANOS, numberOfWorkers: 2, }), makeStat({ @@ -236,6 +246,15 @@ describe("WorkflowRuntimeStatisticsComponent", () => { // Metric 4 = Total Data Processing Time (ns->s converted during grouping). const scanProc = seriesNamed(dataset(4, grouped), "scan-111111"); expect(scanProc.y).toEqual([2, 5]); + + // The remaining indices, so no two entries of the metricKeys array are interchangeable: + // 1 = Input Tuple Size, 2 = Output Tuple Count, 3 = Output Tuple Size, + // 5 = Total Control Processing Time, 6 = Total Idle Time (5 and 6 ns->s converted). + expect(seriesNamed(dataset(1, grouped), "scan-111111").y).toEqual([11, 21]); + expect(seriesNamed(dataset(2, grouped), "scan-111111").y).toEqual([12, 22]); + expect(seriesNamed(dataset(3, grouped), "scan-111111").y).toEqual([13, 23]); + expect(seriesNamed(dataset(5, grouped), "scan-111111").y).toEqual([3, 6]); + expect(seriesNamed(dataset(6, grouped), "scan-111111").y).toEqual([4, 7]); }); it("onTabChanged re-plots the newly selected metric onto the #chart div", async () => { @@ -272,4 +291,36 @@ describe("WorkflowRuntimeStatisticsComponent", () => { expect(warnSpy).toHaveBeenCalledWith("No data available for the chart."); expect(gd.data).toBeUndefined(); }); + + it("onTabChanged after a statistics-less init yields an empty dataset and warns instead of plotting", async () => { + // ngOnInit bails out before groupedStatistics is ever assigned, but the tabs still render, + // so a tab click reaches createDataset with no grouping in place. + await createFixture({ workflowRuntimeStatistics: undefined }); + const gd = chartDiv(); + fixture.detectChanges(); + + expect((component as unknown as { createDataset(i: number): Series[] }).createDataset(3)).toEqual([]); + + component.onTabChanged(3); + + expect(warnSpy).toHaveBeenCalledWith("No data available for the chart."); + expect(gd.data).toBeUndefined(); + }); + + it("createDataset falls back to the numberOfWorkers metric for an out-of-range metric index", async () => { + await createFixture({ workflowRuntimeStatistics: validStats() }); + const grouped = group(); + + // NOTE: the template renders exactly one nz-tab per metric key, so the real UI never produces + // an out-of-range index. This pins a defensive default in createDataset for programmatic callers, + // not a UI path. + // There are 8 metric keys (indices 0..7), so index 8 selects none of them. + const outOfRange = seriesNamed(dataset(8, grouped), "scan-111111"); + + // It falls back to numberOfWorkers, i.e. the same series metric index 7 selects... + expect(outOfRange.y).toEqual([1, 2]); + expect(outOfRange.y).toEqual(seriesNamed(dataset(7, grouped), "scan-111111").y); + // ...and not the metric-0 series, so the fallback really did pick a different key. + expect(outOfRange.y).not.toEqual(seriesNamed(dataset(0, grouped), "scan-111111").y); + }); }); diff --git a/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-sync.service.spec.ts b/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-sync.service.spec.ts index e07f565256..b6777b5a75 100644 --- a/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-sync.service.spec.ts +++ b/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-sync.service.spec.ts @@ -122,6 +122,69 @@ describe("UiUdfParametersSyncService", () => { }); }); + it("should rethrow a non-parse error without emitting a parse-error event", () => { + const unexpectedError = new TypeError("cannot read properties of undefined"); + operator.operatorProperties.uiParameters = [parameter("count", "integer", "42")]; + parserServiceMock.parse.mockImplementation(() => { + throw unexpectedError; + }); + + const parametersChangedObserver = observeParameterChanges(); + const parseErrorObserver = vitest.fn(); + service.uiParametersParseError$.subscribe(parseErrorObserver); + + // Only UiUdfParametersParseError is a user-facing parse problem; anything else is a defect + // and must surface instead of being reported to the user as an editor parse error. + expect(() => service.syncStructureFromCode(operatorId, code)).toThrow(unexpectedError); + + expect(parseErrorObserver).not.toHaveBeenCalled(); + expect(parametersChangedObserver).not.toHaveBeenCalled(); + }); + + // Both nullish guards on the existing-parameter read are covered: the missing-key row exercises + // the `?? []` right operand, the missing-object row the `?.` short-circuit in front of it. + // operatorProperties is typed non-optional on OperatorPredicate, so the second row can only arise + // from a malformed or legacy persisted operator record, never from a statically-typed caller. + [ + { shape: "no uiParameters key", properties: {} as { uiParameters: UiUdfParameter[] } }, + { shape: "no operatorProperties at all", properties: undefined as unknown as { uiParameters: UiUdfParameter[] } }, + ].forEach(({ shape, properties }) => { + it(`should treat a Python UDF with ${shape} as having no existing parameters`, () => { + // Workflows saved before uiParameters existed (and freshly added UDFs) carry no such key. + operator.operatorProperties = properties; + parserServiceMock.parse.mockReturnValue([parameter("count", "integer"), parameter("name", "string")]); + + const parametersChangedObserver = observeParameterChanges(); + + service.syncStructureFromCode(operatorId, code); + + // No existing values to preserve, so every parsed parameter starts blank. + expect(parametersChangedObserver).toHaveBeenCalledWith({ + operatorId, + parameters: [parameter("count", "integer", ""), parameter("name", "string", "")], + }); + expect(parametersChangedObserver).toHaveBeenCalledOnce(); + }); + }); + + it("should clear a previous parse error once the code parses again", () => { + const parseErrorObserver = vitest.fn(); + service.uiParametersParseError$.subscribe(parseErrorObserver); + + parserServiceMock.parse.mockImplementationOnce(() => { + throw new UiUdfParametersParseError("invalid parameters"); + }); + service.syncStructureFromCode(operatorId, code); + + parserServiceMock.parse.mockReturnValue([parameter("count", "integer")]); + service.syncStructureFromCode(operatorId, code); + + // uiParametersParseError$ documents that an event WITHOUT a message clears the operator's + // current parse error, so a successful re-parse must emit exactly {operatorId} and must never + // report a message of its own. + expect(parseErrorObserver.mock.calls).toEqual([[{ operatorId, message: "invalid parameters" }], [{ operatorId }]]); + }); + it("should not replay a previous parser error to a late subscriber", () => { parserServiceMock.parse.mockImplementation(() => { throw new UiUdfParametersParseError("invalid parameters"); diff --git a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts index 0188d52216..c07deff0be 100644 --- a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts @@ -320,6 +320,48 @@ describe("WorkflowResultExportService", () => { expect(notificationServiceSpy.success).toHaveBeenCalledWith("Result exported successfully"); }); + it("errors without a dataset list when every operator is blocked but no dataset was named", () => { + enableExport(); + // The backend can mark an operator restricted while naming no dataset labels for it, + // which leaves the message with nothing to append. + const download = stubDownloadService({ downloadability: { op1: [] } }); + texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); + + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", true, "dataset", makeUnit()); + + expect(notificationServiceSpy.error).toHaveBeenCalledWith( + "Cannot export result: selection depends on dataset(s) that are not downloadable" + ); + expect(notificationServiceSpy.loading).not.toHaveBeenCalled(); + expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled(); + }); + + it("warns without a dataset list when only some operators are blocked and no dataset was named", () => { + enableExport(); + notificationServiceSpy.warning = vi.fn(); + const download = stubDownloadService({ downloadability: { op2: [] } }); + texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }, { operatorID: "op2" }] as any); + + service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", true, "dataset", makeUnit()); + + expect(notificationServiceSpy.warning).toHaveBeenCalledWith( + "Some operators were skipped because their results depend on dataset(s) that are not downloadable" + ); + // the unblocked operator is still exported despite the label-less restriction + expect(notificationServiceSpy.loading).toHaveBeenCalledWith("Exporting..."); + expect(download.exportWorkflowResultToDataset).toHaveBeenCalledWith( + "csv", + "workflow1", + "wf", + [{ id: "op1", outputType: "csv" }], + [7], + 1, + 2, + "file", + expect.anything() + ); + }); + it("exports to dataset and reports success on a success response", () => { enableExport(); (notificationServiceSpy as any).warning = vi.fn(); @@ -369,6 +411,32 @@ describe("WorkflowResultExportService", () => { ); }); + it("reports the error payload itself when the failure carries no nested message", () => { + enableExport(); + // .error is present but is a bare string, so there is no .error.message to extract. + stubDownloadService({ datasetError: { error: "dataset quota exceeded" } }); + texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); + + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + + expect(notificationServiceSpy.error).toHaveBeenCalledWith( + "An error happened in exporting operator results: dataset quota exceeded" + ); + }); + + it("reports the raw failure when it carries no error field at all", () => { + enableExport(); + // e.g. a transport-level failure surfacing as a bare value rather than an HttpErrorResponse. + stubDownloadService({ datasetError: "connection reset by peer" }); + texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); + + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + + expect(notificationServiceSpy.error).toHaveBeenCalledWith( + "An error happened in exporting operator results: connection reset by peer" + ); + }); + it("routes to the local-filesystem export when destination is 'local'", () => { enableExport(); const download = stubDownloadService(); @@ -408,6 +476,39 @@ describe("WorkflowResultExportService", () => { expect(service.hasResultToExportOnAllOperators.value).toBe(true); }); + // The two flags read two DIFFERENT operator scopes -- the highlighted selection for the + // context-menu button, every operator on the canvas for the top-menu button. Giving only one + // scope a result proves each flag reads its own scope and not the other's. + [ + { + scope: "the highlighted selection", + operatorWithSnapshot: "opH", + expectedHighlightedFlag: true, + expectedAllOperatorsFlag: false, + }, + { + scope: "an unselected operator elsewhere on the canvas", + operatorWithSnapshot: "opA", + expectedHighlightedFlag: false, + expectedAllOperatorsFlag: true, + }, + ].forEach(({ scope, operatorWithSnapshot, expectedHighlightedFlag, expectedAllOperatorsFlag }) => { + it(`marks only the matching flag exportable when just ${scope} has a result`, () => { + executeWorkflowServiceSpy.getExecutionState.mockReturnValue({ state: ExecutionState.Completed } as any); + jointGraphWrapperSpy.getCurrentHighlightedOperatorIDs.mockReturnValue(["opH"]); + texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "opA" }] as any); + workflowResultServiceSpy.hasAnyResult.mockReturnValue(false); + workflowResultServiceSpy.getResultService.mockImplementation((operatorId: string) => + operatorId === operatorWithSnapshot ? ({ getCurrentResultSnapshot: () => ({}) } as any) : undefined + ); + + (service as any).updateExportAvailabilityFlags(); + + expect(service.hasResultToExportOnHighlightedOperators).toBe(expectedHighlightedFlag); + expect(service.hasResultToExportOnAllOperators.value).toBe(expectedAllOperatorsFlag); + }); + }); + it("keeps results non-exportable while a workflow is still executing", () => { executeWorkflowServiceSpy.getExecutionState.mockReturnValue({ state: ExecutionState.Running } as any); jointGraphWrapperSpy.getCurrentHighlightedOperatorIDs.mockReturnValue(["opH"]); diff --git a/frontend/src/app/workspace/service/workflow-result/workflow-result.service.spec.ts b/frontend/src/app/workspace/service/workflow-result/workflow-result.service.spec.ts index 7842661761..31b83b5b10 100644 --- a/frontend/src/app/workspace/service/workflow-result/workflow-result.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-result/workflow-result.service.spec.ts @@ -91,11 +91,19 @@ describe("WorkflowResultService", () => { it("routes pagination updates to a paginated service and data updates to a result service", () => { const ws = TestBed.inject(WorkflowWebsocketService); const updateEvents: Record<string, unknown>[] = []; - service.getResultUpdateStream().subscribe(u => updateEvents.push(u)); + // Sampled from INSIDE the subscriber so the emit point is pinned, not just the payload: + // consumers (visualization-frame-content) read the operator's result service back out of this + // service while handling the event, so routing must be finished before the stream fires. + const routedAtEmit: boolean[] = []; + service.getResultUpdateStream().subscribe(u => { + updateEvents.push(u); + routedAtEmit.push(service.hasResult("dataOp"), service.hasPaginatedResult("pagOp")); + }); const updates = { pagOp: paginationUpdate(42, [2]), dataOp: snapshotUpdate([{ a: 1 }]) }; pushWsEvent(ws, { type: "WebResultUpdateEvent", updates, tableStats: {} }); + expect(routedAtEmit).toEqual([true, true]); expect(service.hasPaginatedResult("pagOp")).toBe(true); expect(service.hasResult("pagOp")).toBe(false); expect(service.hasResult("dataOp")).toBe(true); @@ -121,6 +129,70 @@ describe("WorkflowResultService", () => { expect(initiated).toEqual(["pagOp", "dataOp"]); }); + it("creates no result service for a cleared operator entry but still forwards the update record", () => { + const ws = TestBed.inject(WorkflowWebsocketService); + const updateEvents: Record<string, unknown>[] = []; + service.getResultUpdateStream().subscribe(u => updateEvents.push(u)); + const initiated: string[] = []; + service.getResultInitiateStream().subscribe(op => initiated.push(op)); + + // An undefined entry means the operator's result was cleared: it is neither a pagination + // nor a data update, so no service of either kind may be created for it. + const updates = { clearedOp: undefined }; + pushWsEvent(ws, { type: "WebResultUpdateEvent", updates, tableStats: {} }); + + expect(service.hasResult("clearedOp")).toBe(false); + expect(service.hasPaginatedResult("clearedOp")).toBe(false); + expect(service.hasAnyResult("clearedOp")).toBe(false); + expect(initiated).toEqual([]); + // the record itself is still republished verbatim so consumers can drop the stale frame + expect(updateEvents.length).toBe(1); + expect(updateEvents[0]).toBe(updates); + }); + + it("reuses the existing result service for a second data update and announces the operator only once", () => { + const ws = TestBed.inject(WorkflowWebsocketService); + const initiated: string[] = []; + service.getResultInitiateStream().subscribe(op => initiated.push(op)); + + pushWsEvent(ws, { type: "WebResultUpdateEvent", updates: { op: snapshotUpdate([{ a: 1 }]) }, tableStats: {} }); + const firstService = service.getResultService("op"); + expect(firstService).toBeDefined(); + + pushWsEvent(ws, { type: "WebResultUpdateEvent", updates: { op: snapshotUpdate([{ b: 2 }]) }, tableStats: {} }); + + // the SAME instance is reused for the second update -- this identity check carries the whole + // reuse claim on its own + expect(service.getResultService("op")).toBe(firstService); + // a plain shape check on the routed payload; SetSnapshotMode REPLACES the snapshot rather than + // accumulating, so this value alone would also hold for a freshly-constructed service + expect(service.getResultService("op")!.getCurrentResultSnapshot()).toEqual([{ b: 2 }]); + // and the operator is announced only on first creation, not on every update + expect(initiated).toEqual(["op"]); + }); + + it("reuses the existing paginated service for a second pagination update and announces it only once", () => { + const ws = TestBed.inject(WorkflowWebsocketService); + const initiated: string[] = []; + service.getResultInitiateStream().subscribe(op => initiated.push(op)); + + // tableStats also names pagOp, so handleTableStatsUpdate reaches the same get-or-init path in + // the same frame; neither that nor the second frame may re-announce the operator. + pushWsEvent(ws, { + type: "WebResultUpdateEvent", + updates: { pagOp: paginationUpdate(3) }, + tableStats: { pagOp: { colA: { count: 1 } } }, + }); + const firstService = service.getPaginatedResultService("pagOp"); + expect(firstService).toBeDefined(); + + pushWsEvent(ws, { type: "WebResultUpdateEvent", updates: { pagOp: paginationUpdate(9) }, tableStats: {} }); + + expect(service.getPaginatedResultService("pagOp")).toBe(firstService); + expect(service.getPaginatedResultService("pagOp")!.getCurrentTotalNumTuples()).toBe(9); + expect(initiated).toEqual(["pagOp"]); + }); + it("feeds table stats to the matching paginated service and republishes the snapshot", () => { const ws = TestBed.inject(WorkflowWebsocketService); const statsPairs: [unknown, unknown][] = [];
