From 9b39be3d1f642ada9c1db95d969ee8edabfb727e Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 22 Jun 2026 12:25:43 -0700 Subject: [PATCH] refactor+test: complete migration to ensurePatchloomReadyOrNotify for configureMcp/setup/verifyMcp (#147); improve testability and add unit test for helper (#149) - Updated remaining commands using the early-ready-notify pattern. - Moved vscode import inside notify branches (only when UI needed). - Added ensure test for success path (error paths require VSCode env). - Re-ran compile + unit tests: all green. Signed-off-by: Sebastien Tardif --- src/binary/patchloom.ts | 12 +++++++++--- src/commands/configureMcp.ts | 26 ++++---------------------- src/commands/setupWorkspace.ts | 23 +++-------------------- src/commands/verifyMcp.ts | 16 ++++------------ test/unit/binary.test.ts | 15 +++++++++++++++ 5 files changed, 35 insertions(+), 57 deletions(-) diff --git a/src/binary/patchloom.ts b/src/binary/patchloom.ts index 836a540..ea9d25f 100644 --- a/src/binary/patchloom.ts +++ b/src/binary/patchloom.ts @@ -160,11 +160,16 @@ export function patchloomNeedsUpgrade(status: PatchloomStatus): boolean { * * This removes duplicated ready-check + notify logic across commands. */ -export async function ensurePatchloomReadyOrNotify(contextSuffix = ""): Promise { - const status = await resolvePatchloomStatus(); - const vscode = await import("vscode"); +export async function ensurePatchloomReadyOrNotify( + contextSuffix = "", + testInputs?: PatchloomStatusInputs +): Promise { + const status = testInputs + ? await resolvePatchloomStatusWithInputs(testInputs) + : await resolvePatchloomStatus(); if (!status.ready || !status.binaryPath) { + const vscode = await import("vscode"); const choice = await vscode.window.showWarningMessage( `${status.message}${contextSuffix ? `\n\n${contextSuffix}` : ""}`, "Open Settings" @@ -176,6 +181,7 @@ export async function ensurePatchloomReadyOrNotify(contextSuffix = ""): Promise< } if (patchloomNeedsUpgrade(status)) { + const vscode = await import("vscode"); const choice = await vscode.window.showWarningMessage( `${status.compatibilityMessage}${contextSuffix ? `\n\n${contextSuffix}` : ""}`, "Open Releases" diff --git a/src/commands/configureMcp.ts b/src/commands/configureMcp.ts index 4a2723e..6c8bfb3 100644 --- a/src/commands/configureMcp.ts +++ b/src/commands/configureMcp.ts @@ -1,32 +1,14 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import * as vscode from "vscode"; -import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js"; +import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js"; import { configureMcpTargets, inspectMcpTargets } from "../mcp/config.js"; import { activeWorkspaceFolder, describeWorkspaceEnvironment } from "../workspace/readiness.js"; import { refreshStatusBar } from "../status/statusBar.js"; export async function configureMcp(): Promise { - const status = await resolvePatchloomStatus(); - if (!status.ready) { - const choice = await vscode.window.showWarningMessage( - `${status.message}\n\nPatchloom needs a working binary before MCP setup can continue.`, - "Open Settings" - ); - if (choice === "Open Settings") { - await vscode.commands.executeCommand("patchloom.openPatchloomSettings"); - } - return; - } - - if (patchloomNeedsUpgrade(status)) { - const choice = await vscode.window.showWarningMessage( - `${status.compatibilityMessage}\n\nUpgrade Patchloom before MCP setup can continue.`, - "Open Releases" - ); - if (choice === "Open Releases") { - await vscode.commands.executeCommand("patchloom.openPatchloomReleases"); - } + const binaryPath = await ensurePatchloomReadyOrNotify("Patchloom needs a working binary before MCP setup can continue."); + if (!binaryPath) { return; } @@ -66,7 +48,7 @@ export async function configureMcp(): Promise { workspaceFolderPath, includeKinds: selectedKinds, includeUserTarget: environment.supportsUserMcpConfig, - patchloomPathSetting: status.binaryPath, + patchloomPathSetting: binaryPath, readFile: async (filePath) => { try { return await fs.readFile(filePath, "utf8"); diff --git a/src/commands/setupWorkspace.ts b/src/commands/setupWorkspace.ts index 6f159a4..e002282 100644 --- a/src/commands/setupWorkspace.ts +++ b/src/commands/setupWorkspace.ts @@ -1,17 +1,10 @@ import * as vscode from "vscode"; -import { PATCHLOOM_DOCS_URL, PATCHLOOM_RELEASES_URL, patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js"; +import { ensurePatchloomReadyOrNotify, PATCHLOOM_DOCS_URL, PATCHLOOM_RELEASES_URL } from "../binary/patchloom.js"; import { describeWorkspaceEnvironment, inspectWorkspaceReadiness } from "../workspace/readiness.js"; export async function setupWorkspace(): Promise { - const status = await resolvePatchloomStatus(); - if (!status.ready) { - const choice = await vscode.window.showWarningMessage( - `${status.message}\n\nPatchloom needs a working binary before workspace setup can continue.`, - "Open Settings" - ); - if (choice === "Open Settings") { - await vscode.commands.executeCommand("patchloom.openPatchloomSettings"); - } + const binaryPath = await ensurePatchloomReadyOrNotify("Patchloom needs a working binary before workspace setup can continue."); + if (!binaryPath) { return; } @@ -19,16 +12,6 @@ export async function setupWorkspace(): Promise { promptIfMany: true, placeHolder: "Select the workspace folder to inspect for Patchloom setup" }); - if (patchloomNeedsUpgrade(status)) { - const choice = await vscode.window.showWarningMessage( - `${status.compatibilityMessage}\n\nUpgrade Patchloom before workspace setup can continue.`, - "Open Releases" - ); - if (choice === "Open Releases") { - await vscode.commands.executeCommand("patchloom.openPatchloomReleases"); - } - return; - } if (!readiness.hasWorkspace) { await vscode.window.showWarningMessage("Open a workspace folder before running Patchloom: Setup Workspace."); diff --git a/src/commands/verifyMcp.ts b/src/commands/verifyMcp.ts index d734ded..2c956fd 100644 --- a/src/commands/verifyMcp.ts +++ b/src/commands/verifyMcp.ts @@ -1,5 +1,5 @@ import { spawn } from "node:child_process"; -import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js"; +import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js"; import { getPatchloomLog } from "../logging/outputChannel.js"; export interface VerifyMcpInputs { @@ -16,16 +16,8 @@ export interface VerifyMcpResult { export async function verifyMcp(): Promise { const vscode = await import("vscode"); - const status = await resolvePatchloomStatus(); - if (!status.ready || !status.binaryPath) { - await vscode.window.showWarningMessage(status.message); - return; - } - - if (patchloomNeedsUpgrade(status)) { - await vscode.window.showWarningMessage( - `${status.compatibilityMessage}\n\nUpgrade Patchloom before verifying the MCP server.` - ); + const binaryPath = await ensurePatchloomReadyOrNotify("Upgrade Patchloom before verifying the MCP server."); + if (!binaryPath) { return; } @@ -37,7 +29,7 @@ export async function verifyMcp(): Promise { }, async (progress) => { progress.report({ message: "Verifying MCP server..." }); - return verifyMcpServer({ binaryPath: status.binaryPath! }); + return verifyMcpServer({ binaryPath }); } ); diff --git a/test/unit/binary.test.ts b/test/unit/binary.test.ts index b60f600..84daeb5 100644 --- a/test/unit/binary.test.ts +++ b/test/unit/binary.test.ts @@ -13,6 +13,7 @@ import { MINIMUM_SUPPORTED_PATCHLOOM_VERSION, patchloomNeedsUpgrade, parsePatchloomVersion, + ensurePatchloomReadyOrNotify, resolvePatchloomStatusWithInputs } from "../../src/binary/patchloom.js"; import { @@ -127,6 +128,20 @@ test("resolvePatchloomStatusWithInputs exposes compatibility diagnostics", async assert.match(status.compatibilityMessage ?? "", /older than the minimum supported version/i); }); +test("ensurePatchloomReadyOrNotify returns path for ready supported status (test inputs)", async () => { + const path = await ensurePatchloomReadyOrNotify("", { + configuredPath: "/good/patchloom", + canExecute: async () => true, + getVersion: async () => "patchloom 0.2.0" + }); + assert.equal(path, "/good/patchloom"); +}); + +// Note: error paths (not-ready, upgrade) execute vscode.window.show* which is +// not unit-testable in pure node (would require full VS Code test env or mocks). +// They are exercised via command integration and manual. The core logic delegates +// to resolve+needsUpgrade which are unit tested. + test("defaultWorkspaceFolderIndex prefers active folders and only auto-selects single roots", () => { assert.equal(defaultWorkspaceFolderIndex(3, 2), 2); assert.equal(defaultWorkspaceFolderIndex(1, undefined), 0);