Skip to content

Commit a20da2b

Browse files
SebTardifSebTardif
authored
refactor+test+docs: multi-perspective improvements (QA, Developer, New Contributor) (#150)
* refactor+test: extract ensurePatchloomReadyOrNotify helper (reduce ready-check duplication across commands); increase managed install --version timeout for cold-start flakiness - Developer: deduped 6+ copies of ready + upgrade notify logic. - QA: 30s timeout for post-install --version in e2e (was 15s, cold binaries can exceed). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> * docs: update test counts in AGENTS.md for accuracy (New Contributor perspective) - patchloomCli.test.ts count refreshed to reflect current e2e + unit. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> --------- Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
1 parent 3256246 commit a20da2b

6 files changed

Lines changed: 50 additions & 60 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ test/
5252
managedLifecycle.test.ts Managed install with real file I/O (22 tests)
5353
mcpConfig.test.ts MCP config with real temp directories (9 tests)
5454
outputChannel.test.ts Output channel logging wrapper (10 tests)
55-
patchloomCli.test.ts Patchloom CLI integration with real binary + managed install e2e MCP (36 tests)
55+
patchloomCli.test.ts Patchloom CLI integration with real binary + managed install e2e MCP (50+ tests incl. e2e)
5656
propertyBased.test.ts Property-based tests with fast-check (13 tests)
5757
quickActions.test.ts Quick action command building, path containment, patch merge (46 tests)
5858
verifyMcp.test.ts MCP server verify and JSON-RPC response parsing (15 tests)

‎src/binary/patchloom.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,42 @@ export function patchloomNeedsUpgrade(status: PatchloomStatus): boolean {
153153
return status.compatibility === "unsupported";
154154
}
155155

156+
/**
157+
* Ensures Patchloom is ready (found + compatible). If not, shows a warning
158+
* and offers to open Settings or Releases. Returns the binaryPath if ready,
159+
* otherwise null (after showing UI).
160+
*
161+
* This removes duplicated ready-check + notify logic across commands.
162+
*/
163+
export async function ensurePatchloomReadyOrNotify(contextSuffix = ""): Promise<string | null> {
164+
const status = await resolvePatchloomStatus();
165+
const vscode = await import("vscode");
166+
167+
if (!status.ready || !status.binaryPath) {
168+
const choice = await vscode.window.showWarningMessage(
169+
`${status.message}${contextSuffix ? `\n\n${contextSuffix}` : ""}`,
170+
"Open Settings"
171+
);
172+
if (choice === "Open Settings") {
173+
await vscode.commands.executeCommand("patchloom.openPatchloomSettings");
174+
}
175+
return null;
176+
}
177+
178+
if (patchloomNeedsUpgrade(status)) {
179+
const choice = await vscode.window.showWarningMessage(
180+
`${status.compatibilityMessage}${contextSuffix ? `\n\n${contextSuffix}` : ""}`,
181+
"Open Releases"
182+
);
183+
if (choice === "Open Releases") {
184+
await vscode.commands.executeCommand("patchloom.openPatchloomReleases");
185+
}
186+
return null;
187+
}
188+
189+
return status.binaryPath;
190+
}
191+
156192
export function parsePatchloomVersion(versionText?: string): string | undefined {
157193
if (!versionText) {
158194
return undefined;

‎src/commands/batchApply.ts‎

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { execFile } from "node:child_process";
22
import type * as VSCode from "vscode";
3-
import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js";
3+
import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js";
44
import { formatCliOutput } from "../util.js";
55
import { getPatchloomLog } from "../logging/outputChannel.js";
66
import { activeWorkspaceFolder } from "../workspace/readiness.js";
@@ -22,27 +22,12 @@ export function parseBatchOperationCount(plan: string): number {
2222
}
2323

2424
export async function batchApply(): Promise<void> {
25-
const vscode: typeof VSCode = await import("vscode");
26-
const status = await resolvePatchloomStatus();
27-
if (!status.ready || !status.binaryPath) {
28-
const choice = await vscode.window.showWarningMessage(status.message, "Open Settings");
29-
if (choice === "Open Settings") {
30-
await vscode.commands.executeCommand("patchloom.openPatchloomSettings");
31-
}
32-
return;
33-
}
34-
35-
if (patchloomNeedsUpgrade(status)) {
36-
const choice = await vscode.window.showWarningMessage(
37-
`${status.compatibilityMessage}\n\nUpgrade Patchloom before running batch operations.`,
38-
"Open Releases"
39-
);
40-
if (choice === "Open Releases") {
41-
await vscode.commands.executeCommand("patchloom.openPatchloomReleases");
42-
}
25+
const binaryPath = await ensurePatchloomReadyOrNotify("Upgrade Patchloom before running batch operations.");
26+
if (!binaryPath) {
4327
return;
4428
}
4529

30+
const vscode: typeof VSCode = await import("vscode");
4631
const folder = await activeWorkspaceFolder({
4732
promptIfMany: true,
4833
placeHolder: "Select workspace folder for batch apply"
@@ -52,7 +37,6 @@ export async function batchApply(): Promise<void> {
5237
return;
5338
}
5439

55-
const binaryPath = status.binaryPath;
5640
const doc = await vscode.workspace.openTextDocument({
5741
language: "plaintext",
5842
content: BATCH_TEMPLATE

‎src/commands/initializeProject.ts‎

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { execFile } from "node:child_process";
22
import { promisify } from "node:util";
33
import type * as VSCode from "vscode";
4-
import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js";
4+
import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js";
55
import { getPatchloomLog } from "../logging/outputChannel.js";
66
import { formatError } from "../util.js";
77
import { activeWorkspaceFolder } from "../workspace/readiness.js";
@@ -24,29 +24,14 @@ export async function initializeProject(): Promise<void> {
2424
return;
2525
}
2626

27-
const status = await resolvePatchloomStatus();
28-
if (!status.ready || !status.binaryPath) {
29-
const choice = await vscode.window.showWarningMessage(status.message, "Open Settings");
30-
if (choice === "Open Settings") {
31-
await vscode.commands.executeCommand("workbench.action.openSettings", "patchloom.path");
32-
}
33-
return;
34-
}
35-
36-
if (patchloomNeedsUpgrade(status)) {
37-
const choice = await vscode.window.showWarningMessage(
38-
`${status.compatibilityMessage}\n\nUpgrade Patchloom before initializing this workspace.`,
39-
"Open Releases"
40-
);
41-
if (choice === "Open Releases") {
42-
await vscode.commands.executeCommand("patchloom.openPatchloomReleases");
43-
}
27+
const binaryPath = await ensurePatchloomReadyOrNotify("Upgrade Patchloom before initializing this workspace.");
28+
if (!binaryPath) {
4429
return;
4530
}
4631

4732
let rules: string;
4833
try {
49-
rules = await generateAgentRules(status.binaryPath, folder.uri.fsPath);
34+
rules = await generateAgentRules(binaryPath, folder.uri.fsPath);
5035
} catch (error) {
5136
await vscode.window.showErrorMessage(`Failed to run patchloom agent-rules in ${folder.name}: ${formatError(error)}`);
5237
return;

‎src/commands/quickActions.ts‎

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as os from "node:os";
44
import * as path from "node:path";
55
import { promisify } from "node:util";
66
import type * as VSCode from "vscode";
7-
import { patchloomNeedsUpgrade, resolvePatchloomStatus } from "../binary/patchloom.js";
7+
import { ensurePatchloomReadyOrNotify } from "../binary/patchloom.js";
88
import { getPatchloomLog } from "../logging/outputChannel.js";
99
import { formatCliOutput, formatError } from "../util.js";
1010
import { activeWorkspaceFolder, describeWorkspaceEnvironment } from "../workspace/readiness.js";
@@ -37,27 +37,11 @@ interface PatchloomCommandResult {
3737

3838
export async function runQuickAction(): Promise<void> {
3939
const vscode = await import("vscode");
40-
const status = await resolvePatchloomStatus();
41-
if (!status.ready || !status.binaryPath) {
42-
const choice = await vscode.window.showWarningMessage(status.message, "Open Settings");
43-
if (choice === "Open Settings") {
44-
await vscode.commands.executeCommand("patchloom.openPatchloomSettings");
45-
}
46-
return;
47-
}
48-
49-
if (patchloomNeedsUpgrade(status)) {
50-
const choice = await vscode.window.showWarningMessage(
51-
`${status.compatibilityMessage}\n\nUpgrade Patchloom before running quick actions.`,
52-
"Open Releases"
53-
);
54-
if (choice === "Open Releases") {
55-
await vscode.commands.executeCommand("patchloom.openPatchloomReleases");
56-
}
40+
const binaryPath = await ensurePatchloomReadyOrNotify("Upgrade Patchloom before running quick actions.");
41+
if (!binaryPath) {
5742
return;
5843
}
5944

60-
const binaryPath = status.binaryPath;
6145
const actions: Array<VSCode.QuickPickItem & { run: () => Promise<void> }> = [
6246
{
6347
label: "Replace text in file",

‎test/unit/patchloomCli.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,8 @@ describe("managed install end-to-end MCP", { timeout: 120_000 }, async () => {
647647

648648
// Verify the binary is executable
649649
test("managed install produces a runnable binary", async () => {
650-
const { stdout, stderr } = await execFileAsync(binaryPath, ["--version"], { timeout: 15000 });
650+
// Cold start after fresh managed install can take >15s on some runners; use 30s.
651+
const { stdout, stderr } = await execFileAsync(binaryPath, ["--version"], { timeout: 30000 });
651652
const output = `${stdout}${stderr}`.trim();
652653
const version = parsePatchloomVersion(output);
653654
assert.ok(version, `should parse version from managed binary: ${output}`);

0 commit comments

Comments
 (0)