Skip to content

test: cover open editor launch paths #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benvinegar merged 2 commits into from
May 24, 2026
+243 −2
Merged

test: cover open editor launch paths #360

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32b957b
test: cover open editor launch paths
benvinegar May 24, 2026
54406b2
test: refine open editor coverage cases
benvinegar May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
245 changes: 243 additions & 2 deletions src/ui/lib/openInEditor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,60 @@
import { describe, expect, test } from "bun:test";
import { resolve } from "node:path";
import { afterEach, describe, expect, mock, test } from "bun:test";
import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join, resolve } from "node:path";
import { createTestDiffFile } from "../../../test/helpers/diff-helpers";
import {
buildEditorCommand,
openSelectedFileInEditor,
resolveEditableFilePath,
shouldSuspendForEditor,
} from "./openInEditor";

const originalEditor = process.env.EDITOR;
const originalSpawnSync = Bun.spawnSync;
const tempDirs: string[] = [];

function createTempDir() {
const dir = realpathSync(mkdtempSync(join(tmpdir(), "hunk-open-editor-")));
tempDirs.push(dir);
return dir;
}

function restoreEditorEnv() {
if (originalEditor === undefined) {
delete process.env.EDITOR;
} else {
process.env.EDITOR = originalEditor;
}
}

function mockSpawnSync(
implementation: (cmds: string[], options?: Parameters<typeof Bun.spawnSync>[1]) => unknown,
) {
const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync };
mutableBun.spawnSync = implementation as typeof Bun.spawnSync;
}

function createRenderer() {
return {
isDestroyed: false,
resume: mock(() => {}),
suspend: mock(() => {}),
};
}

afterEach(() => {
restoreEditorEnv();
mockSpawnSync(originalSpawnSync);

while (tempDirs.length > 0) {
const dir = tempDirs.pop();
if (dir) {
rmSync(dir, { recursive: true, force: true });
}
}
});

describe("open in editor helpers", () => {
test("builds vi-style editor args without shell quoting", () => {
expect(
Expand Down Expand Up @@ -70,4 +119,196 @@ describe("open in editor helpers", () => {
resolve("/tmp/project", "src/main.tsx"),
);
});

test("returns an error when no file is selected", () => {
const renderer = createRenderer();
const spawnCalls: string[][] = [];
mockSpawnSync((cmds) => {
spawnCalls.push(cmds);
return { exitCode: 0 };
});

expect(
openSelectedFileInEditor({
file: undefined,
renderer,
selectedHunk: undefined,
}),
).toBe("No file selected.");

expect(spawnCalls).toEqual([]);
expect(renderer.suspend).not.toHaveBeenCalled();
expect(renderer.resume).not.toHaveBeenCalled();
});

test("returns an error when $EDITOR is unset", () => {
const renderer = createRenderer();
const spawnCalls: string[][] = [];
mockSpawnSync((cmds) => {
spawnCalls.push(cmds);
return { exitCode: 0 };
});
delete process.env.EDITOR;

expect(
openSelectedFileInEditor({
file: createTestDiffFile({ path: "missing-editor.ts" }),
renderer,
selectedHunk: undefined,
}),
).toBe("$EDITOR is not set.");

expect(spawnCalls).toEqual([]);
expect(renderer.suspend).not.toHaveBeenCalled();
expect(renderer.resume).not.toHaveBeenCalled();
});

test("returns an error when the file does not exist on disk", () => {
const renderer = createRenderer();
const spawnCalls: string[][] = [];
mockSpawnSync((cmds) => {
spawnCalls.push(cmds);
return { exitCode: 0 };
});
process.env.EDITOR = "nvim";

expect(
openSelectedFileInEditor({
basePath: createTempDir(),
file: createTestDiffFile({ path: "missing-on-disk.ts" }),
renderer,
selectedHunk: undefined,
}),
).toBe("Cannot edit missing-on-disk.ts: file does not exist on disk.");

expect(spawnCalls).toEqual([]);
expect(renderer.suspend).not.toHaveBeenCalled();
expect(renderer.resume).not.toHaveBeenCalled();
});

test("spawns terminal editors with suspend and resume around a successful edit", () => {
const basePath = createTempDir();
writeFileSync(join(basePath, "example.ts"), "const value = 1;\n");
process.env.EDITOR = "nvim --clean";

const spawnCalls: Array<{
cmds: string[];
options: Parameters<typeof Bun.spawnSync>[1] | undefined;
}> = [];
mockSpawnSync((cmds, options) => {
spawnCalls.push({ cmds, options });
return { exitCode: 0 };
});
Comment on lines 120 to +201
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Multiple scenarios in a single test body

The "reports missing selection, editor, and on-disk file before spawning" test checks three completely independent error paths (no file, no EDITOR, missing disk file) in one function body. When any single expect fails, the test runner will stop there and the output won't clearly identify which of the three scenarios regressed — all it reports is a failure in the large combined test. Splitting into three focused tests (e.g. "returns error when no file is selected", "returns error when $EDITOR is unset", "returns error when file does not exist on disk") would make failures immediately actionable without changing the coverage.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/ui/lib/openInEditor.test.ts
Line: 121-178

Comment:
**Multiple scenarios in a single test body**

The "reports missing selection, editor, and on-disk file before spawning" test checks three completely independent error paths (no file, no EDITOR, missing disk file) in one function body. When any single `expect` fails, the test runner will stop there and the output won't clearly identify which of the three scenarios regressed — all it reports is a failure in the large combined test. Splitting into three focused tests (e.g. "returns error when no file is selected", "returns error when $EDITOR is unset", "returns error when file does not exist on disk") would make failures immediately actionable without changing the coverage.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Member Author

@benvinegar benvinegar May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the combined validation test into three focused tests for no selection, unset $EDITOR, and missing on-disk file.

This comment was generated by Pi using OpenAI GPT-5


const renderer = createRenderer();
const file = createTestDiffFile({ path: "example.ts" });

expect(
openSelectedFileInEditor({
basePath,
file,
renderer,
selectedHunk: undefined,
}),
).toBeNull();

expect(spawnCalls).toEqual([
{
cmds: ["nvim", "--clean", "+1", join(basePath, "example.ts")],
options: { stdin: "inherit", stdout: "inherit", stderr: "inherit" },
},
]);
expect(renderer.suspend).toHaveBeenCalledTimes(1);
expect(renderer.resume).toHaveBeenCalledTimes(1);
});

test("uses deletion line numbers for deleted files", () => {
const basePath = createTempDir();
writeFileSync(join(basePath, "deleted.ts"), "const old = true;\n");
process.env.EDITOR = "vim";

const spawnCalls: string[][] = [];
mockSpawnSync((cmds) => {
spawnCalls.push(cmds);
return { exitCode: 0 };
});

const baseFile = createTestDiffFile({ path: "deleted.ts" });
const file = {
...baseFile,
metadata: {
...baseFile.metadata,
type: "deleted" as const,
},
};
const selectedHunk = {
...file.metadata.hunks[0]!,
additionStart: 2,
deletionStart: 9,
};

expect(
openSelectedFileInEditor({
basePath,
file,
renderer: createRenderer(),
selectedHunk,
}),
).toBeNull();

expect(spawnCalls).toEqual([["vim", "+9", join(basePath, "deleted.ts")]]);
});

test("does not suspend GUI editors and reports non-zero exits", () => {
const basePath = createTempDir();
writeFileSync(join(basePath, "example.ts"), "const value = 1;\n");
process.env.EDITOR = "code --wait";

const spawnCalls: string[][] = [];
mockSpawnSync((cmds) => {
spawnCalls.push(cmds);
return { exitCode: 2 };
Comment on lines +256 to +270
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Minimal DiffFile cast silently drops required metadata fields

file is constructed as { metadata: { type: "deleted" }, path: "deleted.ts" } as DiffFile, omitting metadata.hunks and all other required fields. The production code currently only reads file.metadata.type and file.path along this path, so it works today — but if openSelectedFileInEditor or a helper it calls ever accesses file.metadata.hunks (or another required field), the test will throw a runtime TypeError rather than produce a meaningful assertion failure. Using createTestDiffFile({ path: "deleted.ts" }) and then setting metadata.type via a spread would give a structurally complete object and keep the same runtime behavior.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/ui/lib/openInEditor.test.ts
Line: 228-242

Comment:
**Minimal DiffFile cast silently drops required metadata fields**

`file` is constructed as `{ metadata: { type: "deleted" }, path: "deleted.ts" } as DiffFile`, omitting `metadata.hunks` and all other required fields. The production code currently only reads `file.metadata.type` and `file.path` along this path, so it works today — but if `openSelectedFileInEditor` or a helper it calls ever accesses `file.metadata.hunks` (or another required field), the test will throw a runtime `TypeError` rather than produce a meaningful assertion failure. Using `createTestDiffFile({ path: "deleted.ts" })` and then setting `metadata.type` via a spread would give a structurally complete object and keep the same runtime behavior.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Member Author

@benvinegar benvinegar May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the deleted-file test to start from createTestDiffFile and override metadata.type via spread, so the fixture remains structurally complete.

This comment was generated by Pi using OpenAI GPT-5

});

const renderer = createRenderer();
const file = createTestDiffFile({ path: "example.ts" });

expect(
openSelectedFileInEditor({
basePath,
file,
renderer,
selectedHunk: file.metadata.hunks[0],
}),
).toBe("Editor exited with status 2.");

expect(spawnCalls).toEqual([["code", "--wait", "--goto", `${join(basePath, "example.ts")}:1`]]);
expect(renderer.suspend).not.toHaveBeenCalled();
expect(renderer.resume).not.toHaveBeenCalled();
});

test("resumes after spawn failures and reports launch errors", () => {
const basePath = createTempDir();
writeFileSync(join(basePath, "example.ts"), "const value = 1;\n");
process.env.EDITOR = "vi";

mockSpawnSync(() => {
throw new Error("boom");
});

const renderer = createRenderer();
const file = createTestDiffFile({ path: "example.ts" });

expect(
openSelectedFileInEditor({
basePath,
file,
renderer,
selectedHunk: file.metadata.hunks[0],
}),
).toBe("Failed to launch editor: boom");

expect(renderer.suspend).toHaveBeenCalledTimes(1);
expect(renderer.resume).toHaveBeenCalledTimes(1);
});
});
Loading