From df76e0f44be2de7ede3e5ad975f6f788ab1db898 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 17 Mar 2026 08:01:38 +0000 Subject: [PATCH] test: harden CI-sensitive test suites --- .github/workflows/ci.yml | 4 +- extensions/feishu/src/bot.test.ts | 21 ++-- extensions/feishu/src/client.test.ts | 103 ++++++++++++------ ...compaction-retry-aggregate-timeout.test.ts | 44 ++++---- .../pi-embedded-runner/system-prompt.test.ts | 21 +++- src/plugins/contracts/wizard.contract.test.ts | 19 ++-- 6 files changed, 131 insertions(+), 81 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ce1299a5d2a..6dc68d2275a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -274,8 +274,8 @@ jobs: - name: Run changed extension tests env: - EXTENSION_ID: ${{ matrix.extension }} - run: pnpm test:extension "$EXTENSION_ID" + OPENCLAW_CHANGED_EXTENSION: ${{ matrix.extension }} + run: pnpm test:extension "$OPENCLAW_CHANGED_EXTENSION" # Types, lint, and format check. check: diff --git a/extensions/feishu/src/bot.test.ts b/extensions/feishu/src/bot.test.ts index ea7dbcb51ec..910fa03f28c 100644 --- a/extensions/feishu/src/bot.test.ts +++ b/extensions/feishu/src/bot.test.ts @@ -77,14 +77,19 @@ vi.mock("./client.js", () => ({ createFeishuClient: mockCreateFeishuClient, })); -vi.mock("openclaw/plugin-sdk/conversation-runtime", () => ({ - resolveConfiguredAcpRoute: (params: unknown) => mockResolveConfiguredAcpRoute(params), - ensureConfiguredAcpRouteReady: (params: unknown) => mockEnsureConfiguredAcpRouteReady(params), - getSessionBindingService: () => ({ - resolveByConversation: mockResolveBoundConversation, - touch: mockTouchBinding, - }), -})); +vi.mock("openclaw/plugin-sdk/conversation-runtime", async (importOriginal) => { + const original = + await importOriginal(); + return { + ...original, + resolveConfiguredAcpRoute: (params: unknown) => mockResolveConfiguredAcpRoute(params), + ensureConfiguredAcpRouteReady: (params: unknown) => mockEnsureConfiguredAcpRouteReady(params), + getSessionBindingService: () => ({ + resolveByConversation: mockResolveBoundConversation, + touch: mockTouchBinding, + }), + }; +}); function createRuntimeEnv(): RuntimeEnv { return { diff --git a/extensions/feishu/src/client.test.ts b/extensions/feishu/src/client.test.ts index 6efda0cbb4e..fe4e04dc310 100644 --- a/extensions/feishu/src/client.test.ts +++ b/extensions/feishu/src/client.test.ts @@ -1,7 +1,16 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { FeishuConfig, ResolvedFeishuAccount } from "./types.js"; -const clientCtorMock = vi.hoisted(() => vi.fn()); +type CreateFeishuClient = typeof import("./client.js").createFeishuClient; +type CreateFeishuWSClient = typeof import("./client.js").createFeishuWSClient; +type ClearClientCache = typeof import("./client.js").clearClientCache; +type SetFeishuClientRuntimeForTest = typeof import("./client.js").setFeishuClientRuntimeForTest; + +const clientCtorMock = vi.hoisted(() => + vi.fn(function clientCtor() { + return { connected: true }; + }), +); const wsClientCtorMock = vi.hoisted(() => vi.fn(function wsClientCtor() { return { connected: true }; @@ -12,7 +21,6 @@ const httpsProxyAgentCtorMock = vi.hoisted(() => return { proxyUrl }; }), ); - const mockBaseHttpInstance = vi.hoisted(() => ({ request: vi.fn().mockResolvedValue({}), get: vi.fn().mockResolvedValue({}), @@ -23,19 +31,17 @@ const mockBaseHttpInstance = vi.hoisted(() => ({ head: vi.fn().mockResolvedValue({}), options: vi.fn().mockResolvedValue({}), })); -import { - createFeishuClient, - createFeishuWSClient, - clearClientCache, - FEISHU_HTTP_TIMEOUT_MS, - FEISHU_HTTP_TIMEOUT_MAX_MS, - FEISHU_HTTP_TIMEOUT_ENV_VAR, - setFeishuClientRuntimeForTest, -} from "./client.js"; - const proxyEnvKeys = ["https_proxy", "HTTPS_PROXY", "http_proxy", "HTTP_PROXY"] as const; type ProxyEnvKey = (typeof proxyEnvKeys)[number]; +let createFeishuClient: CreateFeishuClient; +let createFeishuWSClient: CreateFeishuWSClient; +let clearClientCache: ClearClientCache; +let setFeishuClientRuntimeForTest: SetFeishuClientRuntimeForTest; +let FEISHU_HTTP_TIMEOUT_MS: number; +let FEISHU_HTTP_TIMEOUT_MAX_MS: number; +let FEISHU_HTTP_TIMEOUT_ENV_VAR: string; + let priorProxyEnv: Partial> = {}; let priorFeishuTimeoutEnv: string | undefined; @@ -55,7 +61,31 @@ function firstWsClientOptions(): { agent?: unknown } { return calls[0]?.[0] ?? {}; } -beforeEach(() => { +beforeEach(async () => { + vi.resetModules(); + vi.doMock("@larksuiteoapi/node-sdk", () => ({ + AppType: { SelfBuild: "self" }, + Domain: { Feishu: "https://open.feishu.cn", Lark: "https://open.larksuite.com" }, + LoggerLevel: { info: "info" }, + Client: clientCtorMock, + WSClient: wsClientCtorMock, + EventDispatcher: vi.fn(), + defaultHttpInstance: mockBaseHttpInstance, + })); + vi.doMock("https-proxy-agent", () => ({ + HttpsProxyAgent: httpsProxyAgentCtorMock, + })); + + ({ + createFeishuClient, + createFeishuWSClient, + clearClientCache, + setFeishuClientRuntimeForTest, + FEISHU_HTTP_TIMEOUT_MS, + FEISHU_HTTP_TIMEOUT_MAX_MS, + FEISHU_HTTP_TIMEOUT_ENV_VAR, + } = await import("./client.js")); + priorProxyEnv = {}; priorFeishuTimeoutEnv = process.env[FEISHU_HTTP_TIMEOUT_ENV_VAR]; delete process.env[FEISHU_HTTP_TIMEOUT_ENV_VAR]; @@ -104,7 +134,7 @@ describe("createFeishuClient HTTP timeout", () => { }); const getLastClientHttpInstance = () => { - const calls = clientCtorMock.mock.calls; + const calls = clientCtorMock.mock.calls as unknown as Array<[options: unknown]>; const lastCall = calls[calls.length - 1]?.[0] as | { httpInstance?: { get: (...args: unknown[]) => Promise } } | undefined; @@ -124,21 +154,22 @@ describe("createFeishuClient HTTP timeout", () => { it("passes a custom httpInstance with default timeout to Lark.Client", () => { createFeishuClient({ appId: "app_1", appSecret: "secret_1", accountId: "timeout-test" }); // pragma: allowlist secret - const calls = clientCtorMock.mock.calls; - const lastCall = calls[calls.length - 1][0] as { httpInstance?: unknown }; - expect(lastCall.httpInstance).toBeDefined(); + const calls = clientCtorMock.mock.calls as unknown as Array<[options: unknown]>; + const lastCall = calls[calls.length - 1]?.[0] as { httpInstance?: unknown } | undefined; + expect(lastCall?.httpInstance).toBeDefined(); }); it("injects default timeout into HTTP request options", async () => { createFeishuClient({ appId: "app_2", appSecret: "secret_2", accountId: "timeout-inject" }); // pragma: allowlist secret - const calls = clientCtorMock.mock.calls; - const lastCall = calls[calls.length - 1][0] as { - httpInstance: { post: (...args: unknown[]) => Promise }; - }; - const httpInstance = lastCall.httpInstance; + const calls = clientCtorMock.mock.calls as unknown as Array<[options: unknown]>; + const lastCall = calls[calls.length - 1]?.[0] as + | { httpInstance: { post: (...args: unknown[]) => Promise } } + | undefined; + const httpInstance = lastCall?.httpInstance; - await httpInstance.post( + expect(httpInstance).toBeDefined(); + await httpInstance?.post( "https://example.com/api", { data: 1 }, { headers: { "X-Custom": "yes" } }, @@ -154,13 +185,14 @@ describe("createFeishuClient HTTP timeout", () => { it("allows explicit timeout override per-request", async () => { createFeishuClient({ appId: "app_3", appSecret: "secret_3", accountId: "timeout-override" }); // pragma: allowlist secret - const calls = clientCtorMock.mock.calls; - const lastCall = calls[calls.length - 1][0] as { - httpInstance: { get: (...args: unknown[]) => Promise }; - }; - const httpInstance = lastCall.httpInstance; + const calls = clientCtorMock.mock.calls as unknown as Array<[options: unknown]>; + const lastCall = calls[calls.length - 1]?.[0] as + | { httpInstance: { get: (...args: unknown[]) => Promise } } + | undefined; + const httpInstance = lastCall?.httpInstance; - await httpInstance.get("https://example.com/api", { timeout: 5_000 }); + expect(httpInstance).toBeDefined(); + await httpInstance?.get("https://example.com/api", { timeout: 5_000 }); expect(mockBaseHttpInstance.get).toHaveBeenCalledWith( "https://example.com/api", @@ -243,13 +275,14 @@ describe("createFeishuClient HTTP timeout", () => { config: { httpTimeoutMs: 45_000 }, }); - const calls = clientCtorMock.mock.calls; + const calls = clientCtorMock.mock.calls as unknown as Array<[options: unknown]>; expect(calls.length).toBe(2); - const lastCall = calls[calls.length - 1][0] as { - httpInstance: { get: (...args: unknown[]) => Promise }; - }; - await lastCall.httpInstance.get("https://example.com/api"); + const lastCall = calls[calls.length - 1]?.[0] as + | { httpInstance: { get: (...args: unknown[]) => Promise } } + | undefined; + expect(lastCall?.httpInstance).toBeDefined(); + await lastCall?.httpInstance.get("https://example.com/api"); expect(mockBaseHttpInstance.get).toHaveBeenCalledWith( "https://example.com/api", @@ -264,7 +297,7 @@ describe("createFeishuWSClient proxy handling", () => { expect(httpsProxyAgentCtorMock).not.toHaveBeenCalled(); const options = firstWsClientOptions(); - expect(options?.agent).toBeUndefined(); + expect(options.agent).toBeUndefined(); }); it("uses proxy env precedence: https_proxy first, then HTTPS_PROXY, then http_proxy/HTTP_PROXY", () => { diff --git a/src/agents/pi-embedded-runner/run/compaction-retry-aggregate-timeout.test.ts b/src/agents/pi-embedded-runner/run/compaction-retry-aggregate-timeout.test.ts index 7b7ce460826..e5f02cecf0c 100644 --- a/src/agents/pi-embedded-runner/run/compaction-retry-aggregate-timeout.test.ts +++ b/src/agents/pi-embedded-runner/run/compaction-retry-aggregate-timeout.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it, vi } from "vitest"; import { waitForCompactionRetryWithAggregateTimeout } from "./compaction-retry-aggregate-timeout.js"; type AggregateTimeoutParams = Parameters[0]; +type TimeoutCallback = NonNullable; +type TimeoutCallbackMock = ReturnType>; async function withFakeTimers(run: () => Promise) { vi.useFakeTimers(); @@ -13,7 +15,7 @@ async function withFakeTimers(run: () => Promise) { } } -function expectClearedTimeoutState(onTimeout: ReturnType, timedOut: boolean) { +function expectClearedTimeoutState(onTimeout: TimeoutCallbackMock, timedOut: boolean) { if (timedOut) { expect(onTimeout).toHaveBeenCalledTimes(1); } else { @@ -25,18 +27,15 @@ function expectClearedTimeoutState(onTimeout: ReturnType, timedOut function buildAggregateTimeoutParams( overrides: Partial & Pick, -): { params: AggregateTimeoutParams; onTimeoutSpy: ReturnType } { - const onTimeoutSpy = vi.fn(); - const onTimeout = overrides.onTimeout ?? (() => onTimeoutSpy()); +): AggregateTimeoutParams & { onTimeout: TimeoutCallbackMock } { + const onTimeout = + (overrides.onTimeout as TimeoutCallbackMock | undefined) ?? vi.fn(); return { - params: { - waitForCompactionRetry: overrides.waitForCompactionRetry, - abortable: overrides.abortable ?? (async (promise) => await promise), - aggregateTimeoutMs: overrides.aggregateTimeoutMs ?? 60_000, - isCompactionStillInFlight: overrides.isCompactionStillInFlight, - onTimeout, - }, - onTimeoutSpy, + waitForCompactionRetry: overrides.waitForCompactionRetry, + abortable: overrides.abortable ?? (async (promise) => await promise), + aggregateTimeoutMs: overrides.aggregateTimeoutMs ?? 60_000, + isCompactionStillInFlight: overrides.isCompactionStillInFlight, + onTimeout, }; } @@ -44,7 +43,7 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { it("times out and fires callback when compaction retry never resolves", async () => { await withFakeTimers(async () => { const waitForCompactionRetry = vi.fn(async () => await new Promise(() => {})); - const { params, onTimeoutSpy } = buildAggregateTimeoutParams({ waitForCompactionRetry }); + const params = buildAggregateTimeoutParams({ waitForCompactionRetry }); const resultPromise = waitForCompactionRetryWithAggregateTimeout(params); @@ -52,7 +51,7 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { const result = await resultPromise; expect(result.timedOut).toBe(true); - expectClearedTimeoutState(onTimeoutSpy, true); + expectClearedTimeoutState(params.onTimeout, true); }); }); @@ -72,15 +71,14 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { waitForCompactionRetry, isCompactionStillInFlight: () => compactionInFlight, }); - const { params: aggregateTimeoutParams, onTimeoutSpy } = params; - const resultPromise = waitForCompactionRetryWithAggregateTimeout(aggregateTimeoutParams); + const resultPromise = waitForCompactionRetryWithAggregateTimeout(params); await vi.advanceTimersByTimeAsync(170_000); const result = await resultPromise; expect(result.timedOut).toBe(false); - expectClearedTimeoutState(onTimeoutSpy, false); + expectClearedTimeoutState(params.onTimeout, false); }); }); @@ -91,7 +89,7 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { setTimeout(() => { compactionInFlight = false; }, 90_000); - const { params, onTimeoutSpy } = buildAggregateTimeoutParams({ + const params = buildAggregateTimeoutParams({ waitForCompactionRetry, isCompactionStillInFlight: () => compactionInFlight, }); @@ -102,19 +100,19 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { const result = await resultPromise; expect(result.timedOut).toBe(true); - expectClearedTimeoutState(onTimeoutSpy, true); + expectClearedTimeoutState(params.onTimeout, true); }); }); it("does not time out when compaction retry resolves", async () => { await withFakeTimers(async () => { const waitForCompactionRetry = vi.fn(async () => {}); - const { params, onTimeoutSpy } = buildAggregateTimeoutParams({ waitForCompactionRetry }); + const params = buildAggregateTimeoutParams({ waitForCompactionRetry }); const result = await waitForCompactionRetryWithAggregateTimeout(params); expect(result.timedOut).toBe(false); - expectClearedTimeoutState(onTimeoutSpy, false); + expectClearedTimeoutState(params.onTimeout, false); }); }); @@ -123,7 +121,7 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { const abortError = new Error("aborted"); abortError.name = "AbortError"; const waitForCompactionRetry = vi.fn(async () => await new Promise(() => {})); - const { params, onTimeoutSpy } = buildAggregateTimeoutParams({ + const params = buildAggregateTimeoutParams({ waitForCompactionRetry, abortable: async () => { throw abortError; @@ -132,7 +130,7 @@ describe("waitForCompactionRetryWithAggregateTimeout", () => { await expect(waitForCompactionRetryWithAggregateTimeout(params)).rejects.toThrow("aborted"); - expectClearedTimeoutState(onTimeoutSpy, false); + expectClearedTimeoutState(params.onTimeout, false); }); }); }); diff --git a/src/agents/pi-embedded-runner/system-prompt.test.ts b/src/agents/pi-embedded-runner/system-prompt.test.ts index 0ba4ee66d0f..b50565eb738 100644 --- a/src/agents/pi-embedded-runner/system-prompt.test.ts +++ b/src/agents/pi-embedded-runner/system-prompt.test.ts @@ -2,16 +2,25 @@ import type { AgentSession } from "@mariozechner/pi-coding-agent"; import { describe, expect, it, vi } from "vitest"; import { applySystemPromptOverrideToSession, createSystemPromptOverride } from "./system-prompt.js"; -type MutableSystemPromptFields = { +type MutableSession = { _baseSystemPrompt?: string; _rebuildSystemPrompt?: (toolNames: string[]) => string; }; -function createMockSession() { - const setSystemPrompt = vi.fn(); +type MockSession = MutableSession & { + agent: { + setSystemPrompt: ReturnType; + }; +}; + +function createMockSession(): { + session: MockSession; + setSystemPrompt: ReturnType; +} { + const setSystemPrompt = vi.fn<(prompt: string) => void>(); const session = { agent: { setSystemPrompt }, - } as unknown as AgentSession; + } as MockSession; return { session, setSystemPrompt }; } @@ -19,9 +28,9 @@ function applyAndGetMutableSession( prompt: Parameters[1], ) { const { session, setSystemPrompt } = createMockSession(); - applySystemPromptOverrideToSession(session, prompt); + applySystemPromptOverrideToSession(session as unknown as AgentSession, prompt); return { - mutable: session as unknown as MutableSystemPromptFields, + mutable: session, setSystemPrompt, }; } diff --git a/src/plugins/contracts/wizard.contract.test.ts b/src/plugins/contracts/wizard.contract.test.ts index 9af9d21d411..1e0ca6e49be 100644 --- a/src/plugins/contracts/wizard.contract.test.ts +++ b/src/plugins/contracts/wizard.contract.test.ts @@ -8,12 +8,10 @@ vi.mock("../providers.js", () => ({ resolvePluginProviders: (...args: unknown[]) => resolvePluginProvidersMock(...args), })); -const { - buildProviderPluginMethodChoice, - resolveProviderModelPickerEntries, - resolveProviderPluginChoice, - resolveProviderWizardOptions, -} = await import("../provider-wizard.js"); +let buildProviderPluginMethodChoice: typeof import("../provider-wizard.js").buildProviderPluginMethodChoice; +let resolveProviderModelPickerEntries: typeof import("../provider-wizard.js").resolveProviderModelPickerEntries; +let resolveProviderPluginChoice: typeof import("../provider-wizard.js").resolveProviderPluginChoice; +let resolveProviderWizardOptions: typeof import("../provider-wizard.js").resolveProviderWizardOptions; function resolveExpectedWizardChoiceValues(providers: ProviderPlugin[]) { const values: string[] = []; @@ -72,7 +70,14 @@ function resolveExpectedModelPickerValues(providers: ProviderPlugin[]) { } describe("provider wizard contract", () => { - beforeEach(() => { + beforeEach(async () => { + vi.resetModules(); + ({ + buildProviderPluginMethodChoice, + resolveProviderModelPickerEntries, + resolveProviderPluginChoice, + resolveProviderWizardOptions, + } = await import("../provider-wizard.js")); resolvePluginProvidersMock.mockReset(); resolvePluginProvidersMock.mockReturnValue(uniqueProviderContractProviders); });