From 8af6d1a186c8406c8461e892ff9299212b922ab3 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Feb 2026 20:04:02 +0000 Subject: [PATCH] refactor(test): dedupe repeated fixture setup helpers --- .../server-context.remote-tab-ops.test.ts | 26 +++-- src/plugins/install.test.ts | 65 ++++++------ src/plugins/uninstall.test.ts | 98 +++++++------------ src/security/temp-path-guard.test.ts | 72 ++++++++------ src/slack/monitor/monitor.test.ts | 26 +++-- 5 files changed, 131 insertions(+), 156 deletions(-) diff --git a/src/browser/server-context.remote-tab-ops.test.ts b/src/browser/server-context.remote-tab-ops.test.ts index 9847b20cfd3..f7fdf31ba6b 100644 --- a/src/browser/server-context.remote-tab-ops.test.ts +++ b/src/browser/server-context.remote-tab-ops.test.ts @@ -64,6 +64,16 @@ function createRemoteRouteHarness(fetchMock?: ReturnType) { return { state, remote: ctx.forProfile("remote"), fetchMock: activeFetchMock }; } +function createSequentialPageLister(responses: T[]) { + return vi.fn(async () => { + const next = responses.shift(); + if (!next) { + throw new Error("no more responses"); + } + return next; + }); +} + describe("browser server-context remote profile tab operations", () => { it("uses Playwright tab operations when available", async () => { const listPagesViaPlaywright = vi.fn(async () => [ @@ -158,13 +168,7 @@ describe("browser server-context remote profile tab operations", () => { [{ targetId: "T1", title: "Tab 1", url: "https://example.com", type: "page" }], [{ targetId: "T1", title: "Tab 1", url: "https://example.com", type: "page" }], ]; - const listPagesViaPlaywright = vi.fn(async () => { - const next = responses.shift(); - if (!next) { - throw new Error("no more responses"); - } - return next; - }); + const listPagesViaPlaywright = createSequentialPageLister(responses); vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ listPagesViaPlaywright, @@ -186,13 +190,7 @@ describe("browser server-context remote profile tab operations", () => { { targetId: "B", title: "B", url: "https://b.example", type: "page" }, ], ]; - const listPagesViaPlaywright = vi.fn(async () => { - const next = responses.shift(); - if (!next) { - throw new Error("no more responses"); - } - return next; - }); + const listPagesViaPlaywright = createSequentialPageLister(responses); vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ listPagesViaPlaywright, diff --git a/src/plugins/install.test.ts b/src/plugins/install.test.ts index 02360ebccc6..87409e7eee0 100644 --- a/src/plugins/install.test.ts +++ b/src/plugins/install.test.ts @@ -167,6 +167,26 @@ function setupPluginInstallDirs() { return { tmpDir, pluginDir, extensionsDir }; } +function setupInstallPluginFromDirFixture(params?: { devDependencies?: Record }) { + const workDir = makeTempDir(); + const stateDir = makeTempDir(); + const pluginDir = path.join(workDir, "plugin"); + fs.mkdirSync(path.join(pluginDir, "dist"), { recursive: true }); + fs.writeFileSync( + path.join(pluginDir, "package.json"), + JSON.stringify({ + name: "@openclaw/test-plugin", + version: "0.0.1", + openclaw: { extensions: ["./dist/index.js"] }, + dependencies: { "left-pad": "1.3.0" }, + ...(params?.devDependencies ? { devDependencies: params.devDependencies } : {}), + }), + "utf-8", + ); + fs.writeFileSync(path.join(pluginDir, "dist", "index.js"), "export {};", "utf-8"); + return { pluginDir, extensionsDir: path.join(stateDir, "extensions") }; +} + async function installFromDirWithWarnings(params: { pluginDir: string; extensionsDir: string }) { const warnings: string[] = []; const result = await installPluginFromDir({ @@ -445,21 +465,7 @@ describe("installPluginFromArchive", () => { describe("installPluginFromDir", () => { it("uses --ignore-scripts for dependency install", async () => { - const workDir = makeTempDir(); - const stateDir = makeTempDir(); - const pluginDir = path.join(workDir, "plugin"); - fs.mkdirSync(path.join(pluginDir, "dist"), { recursive: true }); - fs.writeFileSync( - path.join(pluginDir, "package.json"), - JSON.stringify({ - name: "@openclaw/test-plugin", - version: "0.0.1", - openclaw: { extensions: ["./dist/index.js"] }, - dependencies: { "left-pad": "1.3.0" }, - }), - "utf-8", - ); - fs.writeFileSync(path.join(pluginDir, "dist", "index.js"), "export {};", "utf-8"); + const { pluginDir, extensionsDir } = setupInstallPluginFromDirFixture(); const run = vi.mocked(runCommandWithTimeout); await expectInstallUsesIgnoreScripts({ @@ -467,31 +473,18 @@ describe("installPluginFromDir", () => { install: async () => await installPluginFromDir({ dirPath: pluginDir, - extensionsDir: path.join(stateDir, "extensions"), + extensionsDir, }), }); }); it("strips workspace devDependencies before npm install", async () => { - const workDir = makeTempDir(); - const stateDir = makeTempDir(); - const pluginDir = path.join(workDir, "plugin"); - fs.mkdirSync(path.join(pluginDir, "dist"), { recursive: true }); - fs.writeFileSync( - path.join(pluginDir, "package.json"), - JSON.stringify({ - name: "@openclaw/test-plugin", - version: "0.0.1", - openclaw: { extensions: ["./dist/index.js"] }, - dependencies: { "left-pad": "1.3.0" }, - devDependencies: { - openclaw: "workspace:*", - vitest: "^3.0.0", - }, - }), - "utf-8", - ); - fs.writeFileSync(path.join(pluginDir, "dist", "index.js"), "export {};", "utf-8"); + const { pluginDir, extensionsDir } = setupInstallPluginFromDirFixture({ + devDependencies: { + openclaw: "workspace:*", + vitest: "^3.0.0", + }, + }); const run = vi.mocked(runCommandWithTimeout); run.mockResolvedValue({ @@ -505,7 +498,7 @@ describe("installPluginFromDir", () => { const res = await installPluginFromDir({ dirPath: pluginDir, - extensionsDir: path.join(stateDir, "extensions"), + extensionsDir, }); expect(res.ok).toBe(true); if (!res.ok) { diff --git a/src/plugins/uninstall.test.ts b/src/plugins/uninstall.test.ts index 112bc375e31..9286726afc2 100644 --- a/src/plugins/uninstall.test.ts +++ b/src/plugins/uninstall.test.ts @@ -46,6 +46,24 @@ async function createInstalledNpmPluginFixture(params: { }; } +type UninstallResult = Awaited>; + +async function runDeleteInstalledNpmPluginFixture(baseDir: string): Promise<{ + pluginDir: string; + result: UninstallResult; +}> { + const { pluginId, extensionsDir, pluginDir, config } = await createInstalledNpmPluginFixture({ + baseDir, + }); + const result = await uninstallPlugin({ + config, + pluginId, + deleteFiles: true, + extensionsDir, + }); + return { pluginDir, result }; +} + function createSinglePluginEntries(pluginId = "my-plugin") { return { [pluginId]: { enabled: true }, @@ -61,6 +79,21 @@ function createSinglePluginWithEmptySlotsConfig(): OpenClawConfig { }; } +function createSingleNpmInstallConfig(installPath: string): OpenClawConfig { + return { + plugins: { + entries: createSinglePluginEntries(), + installs: { + "my-plugin": { + source: "npm", + spec: "my-plugin@1.0.0", + installPath, + }, + }, + }, + }; +} + async function createPluginDirFixture(baseDir: string, pluginId = "my-plugin") { const pluginDir = path.join(baseDir, pluginId); await fs.mkdir(pluginDir, { recursive: true }); @@ -330,18 +363,9 @@ describe("uninstallPlugin", () => { }); it("deletes directory when deleteFiles is true", async () => { - const { pluginId, extensionsDir, pluginDir, config } = await createInstalledNpmPluginFixture({ - baseDir: tempDir, - }); + const { pluginDir, result } = await runDeleteInstalledNpmPluginFixture(tempDir); try { - const result = await uninstallPlugin({ - config, - pluginId, - deleteFiles: true, - extensionsDir, - }); - expect(result.ok).toBe(true); if (result.ok) { expect(result.actions.directory).toBe(true); @@ -389,18 +413,7 @@ describe("uninstallPlugin", () => { it("does not delete directory when deleteFiles is false", async () => { const pluginDir = await createPluginDirFixture(tempDir); - const config: OpenClawConfig = { - plugins: { - entries: createSinglePluginEntries(), - installs: { - "my-plugin": { - source: "npm", - spec: "my-plugin@1.0.0", - installPath: pluginDir, - }, - }, - }, - }; + const config = createSingleNpmInstallConfig(pluginDir); const result = await uninstallPlugin({ config, @@ -417,20 +430,7 @@ describe("uninstallPlugin", () => { }); it("succeeds even if directory does not exist", async () => { - const config: OpenClawConfig = { - plugins: { - entries: { - "my-plugin": { enabled: true }, - }, - installs: { - "my-plugin": { - source: "npm", - spec: "my-plugin@1.0.0", - installPath: "/nonexistent/path", - }, - }, - }, - }; + const config = createSingleNpmInstallConfig("/nonexistent/path"); const result = await uninstallPlugin({ config, @@ -447,18 +447,9 @@ describe("uninstallPlugin", () => { }); it("returns a warning when directory deletion fails unexpectedly", async () => { - const { pluginId, extensionsDir, config } = await createInstalledNpmPluginFixture({ - baseDir: tempDir, - }); - const rmSpy = vi.spyOn(fs, "rm").mockRejectedValueOnce(new Error("permission denied")); try { - const result = await uninstallPlugin({ - config, - pluginId, - deleteFiles: true, - extensionsDir, - }); + const { result } = await runDeleteInstalledNpmPluginFixture(tempDir); expect(result.ok).toBe(true); if (result.ok) { @@ -477,20 +468,7 @@ describe("uninstallPlugin", () => { await fs.mkdir(outsideDir, { recursive: true }); await fs.writeFile(path.join(outsideDir, "index.js"), "// keep me"); - const config: OpenClawConfig = { - plugins: { - entries: { - "my-plugin": { enabled: true }, - }, - installs: { - "my-plugin": { - source: "npm", - spec: "my-plugin@1.0.0", - installPath: outsideDir, - }, - }, - }, - }; + const config = createSingleNpmInstallConfig(outsideDir); const result = await uninstallPlugin({ config, diff --git a/src/security/temp-path-guard.test.ts b/src/security/temp-path-guard.test.ts index ccf9b58c952..cbc6a1e7c8b 100644 --- a/src/security/temp-path-guard.test.ts +++ b/src/security/temp-path-guard.test.ts @@ -17,6 +17,13 @@ const SKIP_PATTERNS = [ /[\\/][^\\/]*test-harness(?:\.[^\\/]+)?\.ts$/, ]; +type QuoteChar = "'" | '"' | "`"; + +type QuoteScanState = { + quote: QuoteChar | null; + escaped: boolean; +}; + function shouldSkip(relativePath: string): boolean { return SKIP_PATTERNS.some((pattern) => pattern.test(relativePath)); } @@ -27,26 +34,13 @@ function stripCommentsForScan(input: string): string { function findMatchingParen(source: string, openIndex: number): number { let depth = 1; - let quote: "'" | '"' | "`" | null = null; - let escaped = false; + const quoteState: QuoteScanState = { quote: null, escaped: false }; for (let i = openIndex + 1; i < source.length; i += 1) { const ch = source[i]; - if (quote) { - if (escaped) { - escaped = false; - continue; - } - if (ch === "\\") { - escaped = true; - continue; - } - if (ch === quote) { - quote = null; - } + if (consumeQuotedChar(quoteState, ch)) { continue; } - if (ch === "'" || ch === '"' || ch === "`") { - quote = ch; + if (beginQuotedSection(quoteState, ch)) { continue; } if (ch === "(") { @@ -69,27 +63,15 @@ function splitTopLevelArguments(source: string): string[] { let parenDepth = 0; let bracketDepth = 0; let braceDepth = 0; - let quote: "'" | '"' | "`" | null = null; - let escaped = false; + const quoteState: QuoteScanState = { quote: null, escaped: false }; for (let i = 0; i < source.length; i += 1) { const ch = source[i]; - if (quote) { + if (quoteState.quote) { current += ch; - if (escaped) { - escaped = false; - continue; - } - if (ch === "\\") { - escaped = true; - continue; - } - if (ch === quote) { - quote = null; - } + consumeQuotedChar(quoteState, ch); continue; } - if (ch === "'" || ch === '"' || ch === "`") { - quote = ch; + if (beginQuotedSection(quoteState, ch)) { current += ch; continue; } @@ -142,6 +124,32 @@ function splitTopLevelArguments(source: string): string[] { return out; } +function beginQuotedSection(state: QuoteScanState, ch: string): boolean { + if (ch !== "'" && ch !== '"' && ch !== "`") { + return false; + } + state.quote = ch; + return true; +} + +function consumeQuotedChar(state: QuoteScanState, ch: string): boolean { + if (!state.quote) { + return false; + } + if (state.escaped) { + state.escaped = false; + return true; + } + if (ch === "\\") { + state.escaped = true; + return true; + } + if (ch === state.quote) { + state.quote = null; + } + return true; +} + function isOsTmpdirExpression(argument: string): boolean { return /^os\s*\.\s*tmpdir\s*\(\s*\)$/u.test(argument.trim()); } diff --git a/src/slack/monitor/monitor.test.ts b/src/slack/monitor/monitor.test.ts index 9da7fdf0f01..399b9cc563f 100644 --- a/src/slack/monitor/monitor.test.ts +++ b/src/slack/monitor/monitor.test.ts @@ -113,6 +113,16 @@ function createThreadStarterRepliesClient( return { replies, client }; } +function createListedChannelsContext(groupPolicy: "open" | "allowlist") { + return createSlackMonitorContext({ + ...baseParams(), + groupPolicy, + channelsConfig: { + C_LISTED: { requireMention: true }, + }, + }); +} + describe("normalizeSlackChannelType", () => { it("infers channel types from ids when missing", () => { expect(normalizeSlackChannelType(undefined, "C123")).toBe("channel"); @@ -138,13 +148,7 @@ describe("isChannelAllowed with groupPolicy and channelsConfig", () => { it("allows unlisted channels when groupPolicy is open even with channelsConfig entries", () => { // Bug fix: when groupPolicy="open" and channels has some entries, // unlisted channels should still be allowed (not blocked) - const ctx = createSlackMonitorContext({ - ...baseParams(), - groupPolicy: "open", - channelsConfig: { - C_LISTED: { requireMention: true }, - }, - }); + const ctx = createListedChannelsContext("open"); // Listed channel should be allowed expect(ctx.isChannelAllowed({ channelId: "C_LISTED", channelType: "channel" })).toBe(true); // Unlisted channel should ALSO be allowed when policy is "open" @@ -152,13 +156,7 @@ describe("isChannelAllowed with groupPolicy and channelsConfig", () => { }); it("blocks unlisted channels when groupPolicy is allowlist", () => { - const ctx = createSlackMonitorContext({ - ...baseParams(), - groupPolicy: "allowlist", - channelsConfig: { - C_LISTED: { requireMention: true }, - }, - }); + const ctx = createListedChannelsContext("allowlist"); // Listed channel should be allowed expect(ctx.isChannelAllowed({ channelId: "C_LISTED", channelType: "channel" })).toBe(true); // Unlisted channel should be blocked when policy is "allowlist"