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-7026-1f0d2b26875eed3ec38ed2a4a274aed19adb8f68 in repository https://gitbox.apache.org/repos/asf/texera.git
commit f6cb8f2ad8f853b50bdac504336ba95de33f0d61 Author: Xinyuan Lin <[email protected]> AuthorDate: Wed Jul 29 17:24:51 2026 -0700 test(agent-service): extend workflow-crud-tools and add a logger spec (#7026) ### What changes were proposed in this PR? Adds 18 tests (17 -> 35). No source changes. - **`workflow-crud-tools.ts`** (~94% covered): the `modifyOperator` path where properties **pass** metadata validation (only the failing path was covered before), plus the input-port index guards — non-numeric key, negative index, and out-of-range index — each asserting the specific error message. - **`logger.ts`** (~57% covered, **no test before**): a new spec covering its level and formatting branches deterministically, with any stubbed global state restored. Note: `src/agent/prompts.ts` was deliberately left alone — another open PR is already adding `prompts.test.ts`. ### Any related issues, documentation, discussions? Closes #7024. ### How was this PR tested? `bun test src/agent/tools/workflow-crud-tools.spec.ts src/logger.spec.ts` -> 35 pass, 0 fail (104 expect() calls). `bunx prettier --check "src/**/*.{ts,js,json}"` clean. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8 [1M context]) --- .../src/agent/tools/workflow-crud-tools.spec.ts | 141 ++++++++++++++++ agent-service/src/logger.spec.ts | 177 +++++++++++++++++++++ 2 files changed, 318 insertions(+) diff --git a/agent-service/src/agent/tools/workflow-crud-tools.spec.ts b/agent-service/src/agent/tools/workflow-crud-tools.spec.ts index e047d21f09..c1e2b6b93d 100644 --- a/agent-service/src/agent/tools/workflow-crud-tools.spec.ts +++ b/agent-service/src/agent/tools/workflow-crud-tools.spec.ts @@ -186,6 +186,128 @@ describe("createModifyOperatorTool", () => { expect(links[0].source.operatorID).toBe("op1"); expect(links[0].target.operatorID).toBe("op2"); }); + + test("applies properties that pass metadata validation", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op1")); + + // Same validating context as the failure case above, but with values the + // schema accepts, so the tool falls through the validation guard. + const result = await runTool(createModifyOperatorTool(state, context), { + operatorId: "op1", + properties: { title: "kept", count: 7 }, + summary: "valid", + }); + + expect(result).toBe("Operator op1 modified"); + const op = state.getOperator("op1")!; + expect(op.operatorProperties.title).toBe("kept"); + expect(op.operatorProperties.count).toBe(7); + expect(op.customDisplayName).toBe("valid"); + }); + + test("returns an error when an input port index is not a number", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op0", 0, 1)); + state.addOperator(makeOperator("op1")); + state.addLink({ + linkID: "l0", + source: { operatorID: "op0", portID: "output-0" }, + target: { operatorID: "op1", portID: "input-0" }, + }); + + const result = await runTool(createModifyOperatorTool(state), { + operatorId: "op1", + inputOperatorIds: { foo: ["op0"] }, + summary: "bad port", + }); + + expect(result).toContain("[ERROR]"); + expect(result).toContain("non-negative integer"); + // Existing incoming links are dropped before the port indices are validated, + // so the failed modify leaves the operator disconnected. + expect(state.getAllLinks()).toHaveLength(0); + }); + + test("returns an error when an input port index is negative", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op1")); + + const result = await runTool(createModifyOperatorTool(state), { + operatorId: "op1", + inputOperatorIds: { "-1": ["op1"] }, + summary: "bad port", + }); + + expect(result).toContain("[ERROR]"); + expect(result).toContain("non-negative integer"); + expect(state.getAllLinks()).toHaveLength(0); + }); + + test("returns an error when an input port index is out of range", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op0", 0, 1)); + state.addOperator(makeOperator("op1", 1, 1)); + + const result = await runTool(createModifyOperatorTool(state), { + operatorId: "op1", + inputOperatorIds: { "3": ["op0"] }, + summary: "bad port", + }); + + expect(result).toContain("[ERROR]"); + expect(result).toContain("Input port index 3 out of range"); + expect(result).toContain("has 1 input port(s)"); + expect(state.getAllLinks()).toHaveLength(0); + }); + + test("returns an error when a referenced source operator does not exist", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op1")); + + const result = await runTool(createModifyOperatorTool(state), { + operatorId: "op1", + inputOperatorIds: { "0": ["ghost"] }, + summary: "bad source", + }); + + expect(result).toContain("[ERROR]"); + expect(result).toContain('Source operator "ghost" not found'); + expect(state.getAllLinks()).toHaveLength(0); + }); + + test("wraps an unexpected state failure with the operator id", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op1")); + // Force the state layer to blow up so the tool's catch block is exercised. + state.updateOperatorProperties = () => { + throw new Error("state exploded"); + }; + + const result = await runTool(createModifyOperatorTool(state), { + operatorId: "op1", + properties: { title: "x" }, + summary: "boom", + }); + + expect(result).toBe("[ERROR] Error on operator op1: state exploded"); + }); + + test("falls back to the stringified value when a thrown error has no message", async () => { + const state = new WorkflowState(); + state.addOperator(makeOperator("op1")); + state.updateOperatorProperties = () => { + throw "plain string failure"; + }; + + const result = await runTool(createModifyOperatorTool(state), { + operatorId: "op1", + properties: { title: "x" }, + summary: "boom", + }); + + expect(result).toBe("[ERROR] Error on operator op1: plain string failure"); + }); }); describe("createAddOperatorTool", () => { @@ -363,4 +485,23 @@ describe("createAddOperatorTool", () => { expect(links[0].source.operatorID).toBe("op1"); expect(links[0].target.operatorID).toBe("op2"); }); + + test("reports an unexpected state failure without adding the operator", async () => { + const state = new WorkflowState(); + // Force the state layer to blow up so the tool's catch block is exercised. + state.addOperator = () => { + throw new Error("insert failed"); + }; + + const result = await runTool(createAddOperatorTool(state, operatorSchemas, context), { + operatorId: "op1", + operatorType: "TestOp", + properties: {}, + summary: "boom", + }); + + // Unlike modifyOperator, addOperator reports the raw message with no id prefix. + expect(result).toBe("[ERROR] insert failed"); + expect(state.getAllOperators()).toHaveLength(0); + }); }); diff --git a/agent-service/src/logger.spec.ts b/agent-service/src/logger.spec.ts new file mode 100644 index 0000000000..d0063a20df --- /dev/null +++ b/agent-service/src/logger.spec.ts @@ -0,0 +1,177 @@ +/** + * 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 { describe, expect, mock, test } from "bun:test"; +import * as realPinoModule from "pino"; +import * as realEnvModule from "./config/env"; +import { createLogger, logger } from "./logger"; + +// Snapshot the real modules eagerly: `mock.module` swaps the live module +// namespaces process-wide, so these references are what the helper below +// restores once it is done. +const REAL_ENV = realEnvModule.env; +const REAL_LOG_LEVEL = REAL_ENV.TEXERA_SERVICE_LOG_LEVEL; +const REAL_PINO = { ...realPinoModule }; + +type CapturedPinoOptions = { + level?: string; + base?: unknown; + transport?: { target?: string; options?: Record<string, unknown> }; +}; + +type LoadedLogger = { + /** The options object logger.ts handed to the pino factory. */ + options: CapturedPinoOptions; + /** Bindings passed to `rootLogger.child(...)`, in call order. */ + childBindings: Record<string, unknown>[]; + module: typeof import("./logger"); + fakeRoot: any; +}; + +/** + * Re-evaluate logger.ts against a stubbed `env` and a stubbed `pino` factory. + * + * logger.ts builds its root logger at module-evaluation time, so the LOG_PRETTY + * branch is only observable by forcing a fresh evaluation. The `?tag` suffix + * makes Bun build a new module instance instead of returning the cached one. + */ +async function loadLogger(envOverrides: Record<string, unknown>, tag: string): Promise<LoadedLogger> { + const captured: CapturedPinoOptions[] = []; + const childBindings: Record<string, unknown>[] = []; + const fakeRoot: any = { + child(bindings: Record<string, unknown>) { + childBindings.push(bindings); + return { ...fakeRoot, bindings: () => bindings }; + }, + }; + const fakePino: any = (options: CapturedPinoOptions) => { + captured.push(options); + fakeRoot.level = options.level; + return fakeRoot; + }; + + mock.module("./config/env", () => ({ env: { ...REAL_ENV, ...envOverrides } })); + mock.module("pino", () => ({ ...REAL_PINO, default: fakePino, pino: fakePino })); + let loaded: typeof import("./logger"); + try { + loaded = await import(`./logger.ts?${tag}`); + } finally { + mock.module("./config/env", () => ({ env: REAL_ENV })); + mock.module("pino", () => REAL_PINO); + } + + expect(captured).toHaveLength(1); + return { options: captured[0]!, childBindings, module: loaded, fakeRoot }; +} + +describe("root logger construction", () => { + test("uses plain JSON output with no base fields when LOG_PRETTY is off", async () => { + const { options, module } = await loadLogger({ TEXERA_SERVICE_LOG_LEVEL: "warn", LOG_PRETTY: false }, "plain"); + + expect(options.level).toBe("warn"); + // `base: undefined` is explicit so pino omits pid/hostname from every record. + expect("base" in options).toBe(true); + expect(options.base).toBeUndefined(); + // The non-pretty branch spreads `{}`, so the key must be absent entirely - + // a `transport: undefined` would make pino throw. + expect("transport" in options).toBe(false); + // The module exports exactly the instance it built. + expect(module.logger.level).toBe("warn"); + }); + + test("attaches the pino-pretty transport when LOG_PRETTY is on", async () => { + const { options, module, childBindings, fakeRoot } = await loadLogger( + { TEXERA_SERVICE_LOG_LEVEL: "debug", LOG_PRETTY: true }, + "pretty" + ); + + expect(options.level).toBe("debug"); + expect(options.base).toBeUndefined(); + expect(options.transport?.target).toBe("pino-pretty"); + expect(options.transport?.options).toEqual({ + colorize: true, + translateTime: "HH:MM:ss.l", + ignore: "pid,hostname", + }); + expect(module.logger).toBe(fakeRoot); + + // createLogger delegates to the root logger's child(), regardless of transport. + module.createLogger("Ingest", { agentId: "agent-1" }); + expect(childBindings).toEqual([{ module: "Ingest", agentId: "agent-1" }]); + }); +}); + +describe("exported root logger", () => { + test("is wired to the configured level rather than a hardcoded one", () => { + expect(logger.level).toBe(REAL_LOG_LEVEL); + }); + + test("carries no bindings of its own", () => { + expect(logger.bindings()).toEqual({}); + }); +}); + +describe("createLogger", () => { + test("returns a child logger tagged with the module name", () => { + const child = createLogger("Server"); + + expect(child).not.toBe(logger); + expect(child.bindings()).toEqual({ module: "Server" }); + expect(child.level).toBe(logger.level); + }); + + test("merges extra bindings alongside the module name", () => { + const child = createLogger("WS", { agentId: "agent-1", userId: 7 }); + + expect(child.bindings()).toEqual({ module: "WS", agentId: "agent-1", userId: 7 }); + }); + + test("lets explicit bindings override the module name", () => { + // `{ module, ...bindings }` means a `module` key inside bindings wins. + const child = createLogger("Original", { module: "Override" }); + + expect(child.bindings()).toEqual({ module: "Override" }); + }); + + test("does not leak child bindings back onto the root logger", () => { + createLogger("Transient", { agentId: "agent-2" }); + + expect(logger.bindings()).toEqual({}); + }); + + test("gives sibling children independent bindings", () => { + const first = createLogger("A", { agentId: "a" }); + const second = createLogger("B"); + + expect(first.bindings()).toEqual({ module: "A", agentId: "a" }); + expect(second.bindings()).toEqual({ module: "B" }); + }); + + test("honours a level threshold set on the child alone", () => { + const child = createLogger("Levels"); + child.level = "warn"; + + expect(child.isLevelEnabled("error")).toBe(true); + expect(child.isLevelEnabled("warn")).toBe(true); + expect(child.isLevelEnabled("info")).toBe(false); + expect(child.isLevelEnabled("debug")).toBe(false); + // Changing the child must not disturb the shared root logger. + expect(logger.level).toBe(REAL_LOG_LEVEL); + }); +});
