Skip to content

fix(tests): canonicalize the temp repo path in the integration helper - #200

Merged
helizaga merged 1 commit into
mainfrom
tommy/fix-macos-clean-path-test
Sep 15, 2026
Merged

helizaga merged 1 commit into
mainfrom
tommy/fix-macos-clean-path-test

Conversation

@helizaga

@helizaga helizaga commented Sep 15, 2026

Copy link
Copy Markdown
Member

Description

cmd_clean --merged uses nested registered worktree path fails on macOS and passes in CI. setup_integration_repo took its path straight from mktemp -d, which on macOS returns /var/folders/... while /var is a symlink to /private/var. git canonicalizes when it registers a worktree, so the preRemove hook received the /private/var spelling and the assertion compared it against the /var one:

EXPECTED: preRemove:/var/folders/.../tmp.vC6q33uqBL-worktrees/jsmith/my-feature
ACTUAL  : preRemove:/private/var/folders/.../tmp.vC6q33uqBL-worktrees/jsmith/my-feature

Resolving the path once in the helper fixes it for every integration test rather than patching the single assertion. Linux is unaffected, because mktemp -d returns /tmp with no symlink to resolve, which is why CI was green throughout.

Motivation

The suite was permanently red on macOS, which trains contributors to ignore failures and hides real regressions behind an expected one.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature
  • Breaking change
  • Documentation update
  • Code refactoring (no functional changes)

This is the test being wrong, not the product

Worth stating explicitly, since "make the failing assertion pass" can hide a defect. Passing a /var path to git worktree add makes git itself register the resolved one:

passed to git : /var/folders/.../tmp.MDxNeFBZZ2/wt
git registered: /private/var/folders/.../tmp.MDxNeFBZZ2/wt

So the canonical path a hook receives is git's own behavior and the correct thing for cmd_clean to pass along. The test's expectation was the part that was wrong.

Testing

Tested on:

  • macOS (26.6.2, git 2.54.0, bash 5.3.15)
  • Linux
  • Windows (Git Bash)

Linux coverage comes from the CI Tests job on this PR.

Automated gates

  • bats tests/561 of 561 pass, up from 560 of 561. This is the first fully green local run.
  • shellcheck bin/gtr bin/git-gtr lib/*.sh lib/commands/*.sh adapters/editor/*.sh adapters/ai/*.sh — clean.
  • ./scripts/generate-completions.sh --check — up to date.

Teardown still removes the temporary directories; no strays remain after a run.

Breaking Changes

  • This PR introduces breaking changes

None. Test-only; no shipped code paths change.

Checklist

  • I have read CONTRIBUTING.md
  • My code follows the project's style guidelines (cd -P … && pwd matches the existing idiom in bin/gtr and bin/git-gtr)
  • I have performed manual testing on at least one platform
  • I have updated documentation if needed (not applicable)
  • My changes work on multiple platforms (or I've noted platform-specific behavior)
  • I have added/updated shell completions (not applicable; --check passes)
  • I have tested with both git gtr and ./bin/gtr
  • No new external dependencies are introduced
  • All existing functionality still works

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed path handling in macOS integration tests to consistently resolve temporary repository locations.
    • Resolved cleanup failures in nested worktree scenarios caused by differences between symbolic and canonical path formats.
  • Documentation

    • Added an unreleased changelog entry describing the path canonicalization fix.

cmd_clean --merged uses nested registered worktree path failed on macOS
and passed in CI. setup_integration_repo took its path straight from
mktemp -d, which on macOS returns /var/folders/... while /var is a
symlink to /private/var. git canonicalizes when it registers a worktree,
so the hook received /private/var/... and the assertion compared it
against the /var/... spelling.

Verified this is the test's expectation being wrong rather than a
product bug: passing /var/... to `git worktree add` makes git itself
register /private/var/..., so the canonical path a hook receives is
git's own behavior.

Resolving the path in the helper fixes it for every integration test
rather than patching the one assertion. Linux is unaffected, since
mktemp -d returns /tmp with no symlink to resolve.

Full suite now passes locally: 561/561, previously 560/561.
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 14dae5ab-870f-42df-8028-036890b0f450

📥 Commits

Reviewing files that changed from the base of the PR and between f4df634 and 804aa4f.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • tests/test_helper.bash

Included review availability: Your plan provides up to 100 included reviews per hour; 98 remain after this review.


Walkthrough

The integration helper now canonicalizes temporary repository paths before storing them. This aligns macOS paths with Git’s registered worktree paths. The changelog documents the fix. Product code remains unchanged.

Changes

Integration test path canonicalization

Layer / File(s) Summary
Canonicalize temporary repository paths
tests/test_helper.bash, CHANGELOG.md
setup_integration_repo resolves the temporary repository path with cd -P before assigning TEST_REPO. The changelog records the macOS nested worktree cleanup test fix.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: natoboram

Merge Risk: ⚪ Minimal · up to 804aa

This test-only change aligns macOS path spellings without changing product behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: canonicalizing the temporary repository path in the integration test helper.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tommy/fix-macos-clean-path-test

A rabbit hops where temp paths wind
Git finds the true path every time
Mac links no longer cause a fight
Green test rows bloom in morning light
The helper keeps the trail just right

Comment @coderabbitai help to get the list of available commands.

@helizaga
helizaga merged commit d576398 into main Sep 15, 2026
4 checks passed
@helizaga
helizaga deleted the tommy/fix-macos-clean-path-test branch September 15, 2026 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant