Copilot commented on code in PR #6598:
URL: https://github.com/apache/texera/pull/6598#discussion_r3613267026
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts:
##########
@@ -136,6 +140,287 @@ describe("WorkflowResultExportService", () => {
expect(service.hasResultToExportOnHighlightedOperators).toBe(false);
expect(service.hasResultToExportOnAllOperators.value).toBe(false);
});
+
+ // ---- Test helpers
----------------------------------------------------------
+
+ function enableExport(): void {
+ const config = TestBed.inject(GuiConfigService) as unknown as
MockGuiConfigService;
+ config.setConfig({ exportExecutionResultEnabled: true });
+ }
Review Comment:
`enableExport()` mutates a shared config service state but there’s no
corresponding reset shown here. If the `GuiConfigService` instance is shared
across tests in this suite, this introduces order-dependence (tests that expect
export disabled could pass/fail depending on prior tests). Prefer setting the
config explicitly in each test (enabled/disabled), or resetting the config in
`beforeEach`/`afterEach` so each test starts from a known baseline.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts:
##########
@@ -136,6 +140,287 @@ describe("WorkflowResultExportService", () => {
expect(service.hasResultToExportOnHighlightedOperators).toBe(false);
expect(service.hasResultToExportOnAllOperators.value).toBe(false);
});
+
+ // ---- Test helpers
----------------------------------------------------------
+
+ function enableExport(): void {
+ const config = TestBed.inject(GuiConfigService) as unknown as
MockGuiConfigService;
+ config.setConfig({ exportExecutionResultEnabled: true });
+ }
+
+ function makeUnit(): DashboardWorkflowComputingUnit {
+ return { computingUnit: { cuid: 1, type: "local" } } as unknown as
DashboardWorkflowComputingUnit;
+ }
+
+ /** Stubs the export-flow methods the real DownloadService exposes but the
base spy omits. */
+ function stubDownloadService(overrides?: {
+ downloadability?: Record<string, string[]>;
+ datasetResponse?: HttpResponse<unknown>;
+ datasetError?: unknown;
+ }): {
+ getWorkflowResultDownloadability: ReturnType<typeof vi.fn>;
+ exportWorkflowResultToDataset: ReturnType<typeof vi.fn>;
+ exportWorkflowResultToLocal: ReturnType<typeof vi.fn>;
+ } {
+ const getWorkflowResultDownloadability =
vi.fn().mockReturnValue(of(overrides?.downloadability ?? {}));
+ const exportWorkflowResultToDataset = vi
+ .fn()
+ .mockReturnValue(
+ overrides?.datasetError !== undefined
+ ? throwError(() => overrides.datasetError)
+ : of(overrides?.datasetResponse ?? new HttpResponse({ body: {
status: "success", message: "ok" } }))
+ );
+ const exportWorkflowResultToLocal = vi.fn();
+ (downloadServiceSpy as any).getWorkflowResultDownloadability =
getWorkflowResultDownloadability;
+ (downloadServiceSpy as any).exportWorkflowResultToDataset =
exportWorkflowResultToDataset;
+ (downloadServiceSpy as any).exportWorkflowResultToLocal =
exportWorkflowResultToLocal;
+ (workflowResultServiceSpy as any).determineOutputExtension =
vi.fn().mockReturnValue("csv");
+ return { getWorkflowResultDownloadability, exportWorkflowResultToDataset,
exportWorkflowResultToLocal };
+ }
Review Comment:
Assigning methods via `(spy as any).method = ...` bypasses type checking and
can hide mismatches between test doubles and the real service API (especially
as DownloadService evolves). Prefer defining these methods on the mock/spies
when the provider is created (so they’re typed and reset consistently), or
extending the spy type to include the additional methods instead of patching
via `any`.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts:
##########
@@ -136,6 +140,287 @@ describe("WorkflowResultExportService", () => {
expect(service.hasResultToExportOnHighlightedOperators).toBe(false);
expect(service.hasResultToExportOnAllOperators.value).toBe(false);
});
+
+ // ---- Test helpers
----------------------------------------------------------
+
+ function enableExport(): void {
+ const config = TestBed.inject(GuiConfigService) as unknown as
MockGuiConfigService;
+ config.setConfig({ exportExecutionResultEnabled: true });
+ }
+
+ function makeUnit(): DashboardWorkflowComputingUnit {
+ return { computingUnit: { cuid: 1, type: "local" } } as unknown as
DashboardWorkflowComputingUnit;
+ }
+
+ /** Stubs the export-flow methods the real DownloadService exposes but the
base spy omits. */
+ function stubDownloadService(overrides?: {
+ downloadability?: Record<string, string[]>;
+ datasetResponse?: HttpResponse<unknown>;
+ datasetError?: unknown;
+ }): {
+ getWorkflowResultDownloadability: ReturnType<typeof vi.fn>;
+ exportWorkflowResultToDataset: ReturnType<typeof vi.fn>;
+ exportWorkflowResultToLocal: ReturnType<typeof vi.fn>;
+ } {
+ const getWorkflowResultDownloadability =
vi.fn().mockReturnValue(of(overrides?.downloadability ?? {}));
+ const exportWorkflowResultToDataset = vi
+ .fn()
+ .mockReturnValue(
+ overrides?.datasetError !== undefined
+ ? throwError(() => overrides.datasetError)
+ : of(overrides?.datasetResponse ?? new HttpResponse({ body: {
status: "success", message: "ok" } }))
+ );
+ const exportWorkflowResultToLocal = vi.fn();
+ (downloadServiceSpy as any).getWorkflowResultDownloadability =
getWorkflowResultDownloadability;
+ (downloadServiceSpy as any).exportWorkflowResultToDataset =
exportWorkflowResultToDataset;
+ (downloadServiceSpy as any).exportWorkflowResultToLocal =
exportWorkflowResultToLocal;
+ (workflowResultServiceSpy as any).determineOutputExtension =
vi.fn().mockReturnValue("csv");
+ return { getWorkflowResultDownloadability, exportWorkflowResultToDataset,
exportWorkflowResultToLocal };
+ }
+
+ describe("computeRestrictionAnalysis", () => {
+ it("returns an empty restriction map without hitting the backend when the
workflow has no id", () => {
+ workflowActionServiceSpy.getWorkflow.mockReturnValue({ wid: undefined }
as any);
+ const download = stubDownloadService({ downloadability: { op1: ["ds1"] }
});
+
+ let result: WorkflowResultDownloadability | undefined;
+ service.computeRestrictionAnalysis().subscribe(r => (result = r));
+
+ expect(result?.restrictedOperatorMap.size).toBe(0);
+ expect(download.getWorkflowResultDownloadability).not.toHaveBeenCalled();
+ });
+
+ it("maps the backend response into a Map of operatorId ->
Set<datasetLabel>", () => {
+ const download = stubDownloadService({
+ downloadability: { op1: ["ds1 ([email protected])"], op2: ["ds2
([email protected])", "ds3 ([email protected])"] },
+ });
+
+ let result: WorkflowResultDownloadability | undefined;
+ service.computeRestrictionAnalysis().subscribe(r => (result = r));
+
+
expect(download.getWorkflowResultDownloadability).toHaveBeenCalledWith("workflow1");
+ expect(result?.restrictedOperatorMap.get("op1")).toEqual(new Set(["ds1
([email protected])"]));
+ expect(result?.restrictedOperatorMap.get("op2")).toEqual(new Set(["ds2
([email protected])", "ds3 ([email protected])"]));
+ });
+
+ it("falls back to an empty restriction map when the backend call errors",
() => {
+ const getWorkflowResultDownloadability =
vi.fn().mockReturnValue(throwError(() => new Error("boom")));
+ (downloadServiceSpy as any).getWorkflowResultDownloadability =
getWorkflowResultDownloadability;
+
+ let result: WorkflowResultDownloadability | undefined;
+ service.computeRestrictionAnalysis().subscribe(r => (result = r));
+
+ expect(result).toBeInstanceOf(WorkflowResultDownloadability);
+ expect(result?.restrictedOperatorMap.size).toBe(0);
+ });
+ });
+
+ describe("export flow (exportWorkflowExecutionResult / performExport)", ()
=> {
+ it("does nothing when export is disabled in the gui config", () => {
+ const download = stubDownloadService();
+ texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
+
+ service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file",
true, "dataset", makeUnit());
+
+ expect(notificationServiceSpy.loading).not.toHaveBeenCalled();
+ expect(notificationServiceSpy.error).not.toHaveBeenCalled();
+ expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled();
+ });
+
+ it("errors when the computing unit is null", () => {
+ enableExport();
+ const download = stubDownloadService();
+ texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
+
+ service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file",
true, "dataset", null);
+
+ expect(notificationServiceSpy.error).toHaveBeenCalledWith(
+ "Cannot export result: computing unit is not available"
+ );
+ expect(notificationServiceSpy.loading).not.toHaveBeenCalled();
+ expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled();
+ });
+
+ it("errors when the workflow id is not available", () => {
+ enableExport();
+ workflowActionServiceSpy.getWorkflow.mockReturnValue({ wid: undefined }
as any);
+ const download = stubDownloadService();
+ 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: workflow ID is not available");
+ expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled();
+ });
+
+ it("does nothing when no operators are selected (highlighted export with
empty selection)", () => {
+ enableExport();
+ const download = stubDownloadService();
+
jointGraphWrapperSpy.getCurrentHighlightedOperatorIDs.mockReturnValue([]);
+
+ service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file",
false, "dataset", makeUnit());
+
+ expect(notificationServiceSpy.loading).not.toHaveBeenCalled();
+ expect(notificationServiceSpy.error).not.toHaveBeenCalled();
+ expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled();
+ });
+
+ it("errors (no export) when every selected operator is blocked by a
non-downloadable dataset", () => {
+ enableExport();
+ const download = stubDownloadService({ downloadability: { op1: ["ds1
([email protected])"] } });
+ 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: ds1 ([email protected])"
+ );
+ expect(notificationServiceSpy.loading).not.toHaveBeenCalled();
+ expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled();
+ });
+
+ it("warns and still exports the unblocked operators when only some are
blocked", () => {
+ enableExport();
+ (notificationServiceSpy as any).warning = vi.fn();
+ const download = stubDownloadService({ downloadability: { op2: ["ds2
([email protected])"] } });
+ 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" +
+ " (ds2 ([email protected]))"
+ );
+
expect(notificationServiceSpy.loading).toHaveBeenCalledWith("Exporting...");
+ expect(download.exportWorkflowResultToDataset).toHaveBeenCalledWith(
+ "csv",
+ "workflow1",
+ "wf",
+ [{ id: "op1", outputType: "csv" }],
+ [7],
+ 1,
+ 2,
+ "file",
+ expect.anything()
+ );
+ expect(notificationServiceSpy.success).toHaveBeenCalledWith("Result
exported successfully");
+ });
+
+ it("exports to dataset and reports success on a success response", () => {
+ enableExport();
+ (notificationServiceSpy as any).warning = vi.fn();
+ const download = stubDownloadService();
+ texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
+
+ service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file",
true, "dataset", makeUnit());
+
+ expect(notificationServiceSpy.warning).not.toHaveBeenCalled();
+
expect(notificationServiceSpy.loading).toHaveBeenCalledWith("Exporting...");
+ expect(download.exportWorkflowResultToDataset).toHaveBeenCalledTimes(1);
+ expect(notificationServiceSpy.success).toHaveBeenCalledWith("Result
exported successfully");
+ });
+
+ it("reports the server message when the dataset export response is not
successful", () => {
+ enableExport();
+ stubDownloadService({
+ datasetResponse: new HttpResponse({ body: { status: "failure",
message: "quota exceeded" } }),
+ });
+ texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
+
+ service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file",
true, "dataset", makeUnit());
+
+ expect(notificationServiceSpy.error).toHaveBeenCalledWith("quota
exceeded");
+ expect(notificationServiceSpy.success).not.toHaveBeenCalled();
+ });
+
+ it("reports a default error message when the dataset export response has
no body", () => {
+ enableExport();
+ stubDownloadService({ datasetResponse: new HttpResponse({}) });
+ texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
+
+ service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file",
true, "dataset", makeUnit());
+
+ expect(notificationServiceSpy.error).toHaveBeenCalledWith("An error
occurred during export");
+ });
+
+ it("reports the extracted error message when the dataset export call
fails", () => {
+ enableExport();
+ stubDownloadService({ datasetError: { error: { message: "server
exploded" } } });
+ 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: server exploded"
+ );
+ });
+
+ it("routes to the local-filesystem export when destination is 'local'", ()
=> {
+ enableExport();
+ const download = stubDownloadService();
+ texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
+
+ service.exportWorkflowExecutionResult("json", "wf", [], 3, 4,
"local-file", true, "local", makeUnit());
+
+
expect(notificationServiceSpy.loading).toHaveBeenCalledWith("Exporting...");
+ expect(download.exportWorkflowResultToLocal).toHaveBeenCalledWith(
+ "json",
+ "workflow1",
+ "wf",
+ [{ id: "op1", outputType: "csv" }],
+ 3,
+ 4,
+ "local-file",
+ expect.anything()
+ );
Review Comment:
The test passes `"json"` as the export format but asserts `outputType:
"csv"`. This makes the test (and the stubbed `determineOutputExtension`)
inconsistent with the intent of verifying local-export routing, and can mask
regressions around output type selection. Align the expectation with the
intended output type (likely `"json"` here), or adjust the stub so
`determineOutputExtension` returns the expected type for this scenario.
--
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]
