test: add unit tests for src/git/diff.ts

[lb1192176991-lab]

What

Added comprehensive unit tests for all exported functions in src/git/diff.ts using Node's built-in node:test and node:assert/strict.

Why

Git operations are central to the tool but had no test coverage. These tests validate the core git interaction layer in isolated temporary repositories.

Test coverage

• checkGitRepo() — returns successfully inside a git repo, throws outside
• getStagedDiff() — returns diff when staged, empty when nothing staged
• getUnstagedDiff() — detects unstaged changes on tracked files
• commit() — returns hash and summary, works with body, throws on empty commit
• getRepoRoot() — returns the absolute path of the repository root

Testing

• npm run build && node --test tests/git-diff.test.mjs — all 9 tests pass

[404-Page-Found]

Review: PR #76 — test: add unit tests for src/git/diff.ts

Author	lb1192176991-lab
Base → Head	main → feat/git-diff-tests
Files changed	2 (+165 / -2)
State	Open

---

Overview ✅

The PR title and description clearly explain what (add unit tests for src/git/diff.ts) and why (git operations are central but had no coverage). Scope is focused — tests plus a minor refactor to the commit() return type. Size is manageable at 165 additions.

---

Correctness & Logic — ⚠️ Blocking

1. commit() return type change breaks all callers

The function signature changed from return string to return CommitResult (an object), but the two call sites in src/commands/suggest.ts were not updated:

• Line 149 (auto path): result.trim() — .trim() on an object will throw at runtime.
• Line 210 (interactive path): result.trim() — same issue.

Both need to become result.raw.trim() (or result.raw).

2. Existing test will break

tests/commit-echo.test.mjs does console.log(out) where out is the return value of commit(). With the new object return type, this prints [object Object] instead of the raw git output string, causing assertions like res.stdout.includes(title) to fail. The test runner needs console.log(out.raw).

---

Code Quality & Maintainability — Comment

3. Regex parsing of git output is fragile

The regex /[\[\S+(?:\s+\([^)]+\))?\s+([a-f0-9]+)\]\s+(.+)/ targets the standard [branch hash] summary format, but git output varies by detached HEAD state, rebase/merge states, non-English locale, and custom format.pretty overrides. A mismatch silently produces hash: undefined, summary: undefined. Consider adding a fallback when the regex doesn't match.

---

Testing ✅

The test suite itself is well-structured:

• Isolated environments: Each test creates a temp dir, initializes a real git repo, and cleans up via try/finally — clean and reliable.
• Coverage: All 5 exported functions are tested (9 tests) covering happy paths and error paths.
• Error cases: checkGitRepo() throws outside a repo, commit() throws when nothing is staged, getStagedDiff() returns empty when nothing is staged.
• Conventions: Uses node:test / node:assert/strict matching existing project pattern. Imports compiled .js from dist/. ✅

---

Performance, Security, Documentation ✅

No concerns. No new dependencies, no security-sensitive changes.

---

Verdict: Request changes

Must fix before merge:

1. Update src/commands/suggest.ts lines 149 and 210 to use result.raw instead of bare result (since .trim() is called on the value).
2. Update tests/commit-echo.test.mjs to handle the new CommitResult return type (change console.log(out) to console.log(out.raw)).

Recommended (non-blocking):
3. Add a fallback in commit() when the regex doesn't match the git output format, so hash and summary are explicitly undefined rather than silently failing.