Copilot commented on code in PR #6379: URL: https://github.com/apache/texera/pull/6379#discussion_r3567400128
########## agent-service/src/agent/tools/workflow-execution-tools.spec.ts: ########## @@ -0,0 +1,262 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { executeOperatorAndFormat, type ExecutionConfig } from "./workflow-execution-tools"; +import { WorkflowState } from "../workflow-state"; +import { WorkflowSystemMetadata } from "../util/workflow-system-metadata"; +import type { OperatorPredicate, PortDescription } from "../../types/workflow"; +import type { OperatorInfo, SyncExecutionResult } from "../../types/execution"; + +function makeOperator(id: string, inputPorts: PortDescription[] = []): OperatorPredicate { + return { + operatorID: id, + operatorType: "TestOp", + operatorVersion: "1.0", + operatorProperties: {}, + inputPorts, + outputPorts: [], + showAdvanced: false, + }; +} + +function stateWith(...operators: OperatorPredicate[]): WorkflowState { + const state = new WorkflowState(); + for (const op of operators) state.addOperator(op); + return state; +} + +function cfg(overrides: Partial<ExecutionConfig> = {}): ExecutionConfig { + return { userToken: "tok", workflowId: 1, ...overrides }; +} + +// A fetch double resolving to an ok response whose body is the given result. +function resolveFetch(spy: ReturnType<typeof spyOn>, result: SyncExecutionResult): void { + spy.mockResolvedValue({ ok: true, json: async () => result } as unknown as Response); +} + +let fetchSpy: ReturnType<typeof spyOn>; +let validateSpy: ReturnType<typeof spyOn>; + +beforeEach(() => { + // Default: any unexpected network call fails loudly instead of hitting localhost. + fetchSpy = spyOn(globalThis, "fetch").mockRejectedValue(new Error("unexpected fetch")); + // Isolate connection validation from schema validation (TestOp is an unknown type). + validateSpy = spyOn(WorkflowSystemMetadata.getInstance(), "validateOperatorProperties").mockReturnValue({ + isValid: true, + }); +}); + +afterEach(() => { + fetchSpy.mockRestore(); + validateSpy.mockRestore(); +}); + +describe("executeOperatorAndFormat — guards & validation", () => { + test("reports an empty workflow when the target operator is absent", async () => { + const result = await executeOperatorAndFormat(new WorkflowState(), cfg(), "op1"); + expect(result).toBe("[ERROR] Cannot execute: workflow has no operators."); Review Comment: This test name says the target operator is absent, but the setup uses an entirely empty `WorkflowState()` (it’s really testing the “no operators” guard). Renaming the test to match the actual scenario will make the suite clearer/less misleading. ########## agent-service/src/agent/tools/workflow-execution-tools.spec.ts: ########## @@ -0,0 +1,262 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { executeOperatorAndFormat, type ExecutionConfig } from "./workflow-execution-tools"; +import { WorkflowState } from "../workflow-state"; +import { WorkflowSystemMetadata } from "../util/workflow-system-metadata"; +import type { OperatorPredicate, PortDescription } from "../../types/workflow"; +import type { OperatorInfo, SyncExecutionResult } from "../../types/execution"; + +function makeOperator(id: string, inputPorts: PortDescription[] = []): OperatorPredicate { + return { + operatorID: id, + operatorType: "TestOp", + operatorVersion: "1.0", + operatorProperties: {}, + inputPorts, + outputPorts: [], + showAdvanced: false, + }; +} + +function stateWith(...operators: OperatorPredicate[]): WorkflowState { + const state = new WorkflowState(); + for (const op of operators) state.addOperator(op); + return state; +} + +function cfg(overrides: Partial<ExecutionConfig> = {}): ExecutionConfig { + return { userToken: "tok", workflowId: 1, ...overrides }; +} + +// A fetch double resolving to an ok response whose body is the given result. +function resolveFetch(spy: ReturnType<typeof spyOn>, result: SyncExecutionResult): void { + spy.mockResolvedValue({ ok: true, json: async () => result } as unknown as Response); +} + +let fetchSpy: ReturnType<typeof spyOn>; +let validateSpy: ReturnType<typeof spyOn>; + +beforeEach(() => { + // Default: any unexpected network call fails loudly instead of hitting localhost. + fetchSpy = spyOn(globalThis, "fetch").mockRejectedValue(new Error("unexpected fetch")); + // Isolate connection validation from schema validation (TestOp is an unknown type). + validateSpy = spyOn(WorkflowSystemMetadata.getInstance(), "validateOperatorProperties").mockReturnValue({ + isValid: true, + }); +}); + +afterEach(() => { + fetchSpy.mockRestore(); + validateSpy.mockRestore(); +}); + +describe("executeOperatorAndFormat — guards & validation", () => { + test("reports an empty workflow when the target operator is absent", async () => { + const result = await executeOperatorAndFormat(new WorkflowState(), cfg(), "op1"); + expect(result).toBe("[ERROR] Cannot execute: workflow has no operators."); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + test("blocks on the target operator's own validation error without executing", async () => { + // One unlinked input port -> connection validation fails for op1. + const state = stateWith(makeOperator("op1", [{ portID: "input-0" }])); + const result = await executeOperatorAndFormat(state, cfg(), "op1"); + expect(result).toBe("[ERROR] Operator op1:\n - inputs: input-0 requires at least 1 input, has 0."); + expect(fetchSpy).not.toHaveBeenCalled(); + }); +}); + +describe("executeOperatorAndFormat — execution-level failures", () => { + test("formats per-operator errors when the run state is Failed and notifies onResult", async () => { + const state = stateWith(makeOperator("op1")); + resolveFetch(fetchSpy, { + success: false, + state: "Failed", + operators: { + op1: { state: "Failed", inputTuples: 0, outputTuples: 0, resultMode: "table", error: "runtime boom" }, + }, + errors: [], + }); + const onResult = mock((_id: string, _info: OperatorInfo) => {}); + + const result = await executeOperatorAndFormat(state, cfg(), "op1", { onResult }); + + expect(result).toContain("[ERROR]"); + expect(result).toContain("Execution error:"); + expect(result).toContain("op1: runtime boom"); + expect(onResult).toHaveBeenCalledTimes(1); + expect(onResult.mock.calls[0][0]).toBe("op1"); + expect(onResult.mock.calls[0][1].state).toBe("Failed"); + expect(onResult.mock.calls[0][1].error).toContain("runtime boom"); + }); + + test("formats compilation errors when the run state is CompilationFailed", async () => { + const state = stateWith(makeOperator("op1")); + resolveFetch(fetchSpy, { + success: false, + state: "CompilationFailed", + operators: {}, + compilationErrors: { op1: "type mismatch" }, + }); + + const result = await executeOperatorAndFormat(state, cfg(), "op1"); + + expect(result).toContain("Compilation error:"); + expect(result).toContain("op1: type mismatch"); + }); + + test("reports a timeout message when the run state is Killed", async () => { + const state = stateWith(makeOperator("op1")); + resolveFetch(fetchSpy, { success: false, state: "Killed", operators: {}, errors: ["ignored"] }); + + const result = await executeOperatorAndFormat(state, cfg(), "op1"); + + expect(result).toContain("Workflow execution was killed (timeout)."); + expect(result).not.toContain("ignored"); + }); + + test("surfaces a network error as a general error", async () => { + const state = stateWith(makeOperator("op1")); + fetchSpy.mockRejectedValue(new Error("network down")); + + const result = await executeOperatorAndFormat(state, cfg(), "op1"); + + expect(result).toContain("[ERROR]"); + expect(result).toContain("network down"); + }); + + test("surfaces a non-ok HTTP response as a general error", async () => { + const state = stateWith(makeOperator("op1")); + fetchSpy.mockResolvedValue({ + ok: false, + status: 500, + statusText: "Internal Server Error", + text: async () => "upstream boom", + } as unknown as Response); + + const result = await executeOperatorAndFormat(state, cfg(), "op1"); + + expect(result).toContain("Execution request failed: 500 Internal Server Error - upstream boom"); + }); +}); + +describe("executeOperatorAndFormat — operator result handling", () => { + test("errors when the run succeeds but the target operator has no result entry", async () => { + const state = stateWith(makeOperator("op1")); + resolveFetch(fetchSpy, { success: true, state: "Completed", operators: {} }); + + const result = await executeOperatorAndFormat(state, cfg(), "op1"); + + expect(result).toContain("[ERROR]"); + expect(result).toContain("No result found for operator: op1"); + }); + + test("errors and notifies onResult when the target operator carries an error", async () => { + const state = stateWith(makeOperator("op1")); + const opInfo: OperatorInfo = { + state: "Completed", + inputTuples: 0, + outputTuples: 0, + resultMode: "table", + error: "kaboom", + }; + resolveFetch(fetchSpy, { success: true, state: "Completed", operators: { op1: opInfo } }); + const onResult = mock((_id: string, _info: OperatorInfo) => {}); + + const result = await executeOperatorAndFormat(state, cfg(), "op1", { onResult }); + + expect(result).toContain("Execution error:"); + expect(result).toContain("op1: kaboom"); + expect(onResult).toHaveBeenCalledTimes(1); + expect(onResult.mock.calls[0][1].error).toBe("kaboom"); + }); + + test("returns a placeholder when the operator result is not tabular", async () => { Review Comment: The behavior under test here is the `(no result data)` placeholder when `opInfo.result` is missing or not an array; it’s not about a “non-tabular” result (the result mode is still `table`). Renaming the test will better reflect the branch being exercised. ########## agent-service/src/agent/tools/workflow-execution-tools.spec.ts: ########## @@ -0,0 +1,262 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { executeOperatorAndFormat, type ExecutionConfig } from "./workflow-execution-tools"; +import { WorkflowState } from "../workflow-state"; +import { WorkflowSystemMetadata } from "../util/workflow-system-metadata"; +import type { OperatorPredicate, PortDescription } from "../../types/workflow"; +import type { OperatorInfo, SyncExecutionResult } from "../../types/execution"; + +function makeOperator(id: string, inputPorts: PortDescription[] = []): OperatorPredicate { + return { + operatorID: id, + operatorType: "TestOp", + operatorVersion: "1.0", + operatorProperties: {}, + inputPorts, + outputPorts: [], + showAdvanced: false, + }; +} + +function stateWith(...operators: OperatorPredicate[]): WorkflowState { + const state = new WorkflowState(); + for (const op of operators) state.addOperator(op); + return state; +} + +function cfg(overrides: Partial<ExecutionConfig> = {}): ExecutionConfig { + return { userToken: "tok", workflowId: 1, ...overrides }; +} + +// A fetch double resolving to an ok response whose body is the given result. +function resolveFetch(spy: ReturnType<typeof spyOn>, result: SyncExecutionResult): void { + spy.mockResolvedValue({ ok: true, json: async () => result } as unknown as Response); +} + +let fetchSpy: ReturnType<typeof spyOn>; +let validateSpy: ReturnType<typeof spyOn>; + +beforeEach(() => { + // Default: any unexpected network call fails loudly instead of hitting localhost. + fetchSpy = spyOn(globalThis, "fetch").mockRejectedValue(new Error("unexpected fetch")); + // Isolate connection validation from schema validation (TestOp is an unknown type). + validateSpy = spyOn(WorkflowSystemMetadata.getInstance(), "validateOperatorProperties").mockReturnValue({ + isValid: true, + }); +}); + +afterEach(() => { + fetchSpy.mockRestore(); + validateSpy.mockRestore(); +}); Review Comment: `bun test` runs tests concurrently by default, but this suite mutates shared globals (`globalThis.fetch`) and a singleton method spy in `beforeEach`/`afterEach`. If tests in this file execute in parallel, they can race/overwrite each other’s spies and produce flaky failures (or hit the "unexpected fetch" rejection from another test’s setup). Consider marking these tests as serial (e.g., change each `test(...)` to `test.serial(...)`) or otherwise isolating the mocked globals per test run. -- 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]
