test: cover open editor launch paths#360
Conversation
Greptile SummaryThis PR adds focused test coverage for
Confidence Score: 4/5Safe to merge — changes are test-only and do not touch production code. The new tests are test-only additions that cover real execution paths in openSelectedFileInEditor. Two minor structural concerns exist: one test bundles three independent scenarios making targeted failure diagnosis harder, and a manual DiffFile cast in the deleted-file test omits required metadata fields, leaving the test brittle against future production code changes. Neither issue affects correctness today, but both reduce the long-term value of the test suite. src/ui/lib/openInEditor.test.ts — the bundled-scenario test and the incomplete DiffFile cast warrant a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[openSelectedFileInEditor called] --> B{file defined?}
B -- No --> C[return 'No file selected.']
B -- Yes --> D{EDITOR set?}
D -- No --> E[return '$EDITOR is not set.']
D -- Yes --> F{file exists on disk?}
F -- No --> G[return 'Cannot edit ...: file does not exist on disk.']
F -- Yes --> H[compute line number]
H --> I{shouldSuspendForEditor?}
I -- Yes --> J[renderer.suspend]
J --> K[Bun.spawnSync]
I -- No --> K
K -- throws --> L[catch: failureMessage = error.message]
K -- exitCode != 0 --> M[exitCode captured]
K -- exitCode = 0 --> N[success]
L --> O{shouldSuspend and not isDestroyed?}
M --> O
N --> O
O -- Yes --> P[renderer.resume]
O -- No --> Q{failureMessage?}
P --> Q
Q -- Yes --> R[return 'Failed to launch editor: ...']
Q -- No --> S{exitCode != 0?}
S -- Yes --> T[return 'Editor exited with status N.']
S -- No --> U[return null]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/ui/lib/openInEditor.test.ts:121-178
**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.
### Issue 2 of 2
src/ui/lib/openInEditor.test.ts:228-242
**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.
Reviews (1): Last reviewed commit: "test: cover open editor launch paths" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ); | ||
| }); | ||
|
|
||
| test("reports missing selection, editor, and on-disk file before spawning", () => { | ||
| 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."); | ||
|
|
||
| delete process.env.EDITOR; | ||
| const missingEditorFile = createTestDiffFile({ path: "missing-editor.ts" }); | ||
| expect( | ||
| openSelectedFileInEditor({ | ||
| file: missingEditorFile, | ||
| renderer, | ||
| selectedHunk: undefined, | ||
| }), | ||
| ).toBe("$EDITOR is not set."); | ||
|
|
||
| process.env.EDITOR = "nvim"; | ||
| const missingDiskFile = createTestDiffFile({ path: "missing-on-disk.ts" }); | ||
| expect( | ||
| openSelectedFileInEditor({ | ||
| basePath: createTempDir(), | ||
| file: missingDiskFile, | ||
| 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 }; | ||
| }); |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
| }), | ||
| ).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 }; |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
1 participant
Summary
openSelectedFileInEditorvalidation and spawn paths.Tests
bun test src/ui/lib/openInEditor.test.tsbun run typecheckbun test --coverage src/ui/lib/openInEditor.test.tsThis PR description was generated by Pi using <model name + version>