[testify-expert] Improve Test Quality: pkg/cli/git_test.go — migrate to testify assertions

[github-actions]

Overview

The test file pkg/cli/git_test.go (904 lines, 14 test functions) was selected for quality improvement. It tests Git-related utilities in the CLI package. While the file has solid test coverage and uses table-driven tests in some places, it entirely avoids testify assertions — relying on raw t.Fatal, t.Error, and t.Errorf throughout. Migrating to testify would improve test clarity, reduce verbosity, and align with the codebase standard described in scratchpad/testing.md.

Current State

• Test File: pkg/cli/git_test.go
• Source File: pkg/cli/git.go
• Test Functions: 14 test functions
• Lines of Code: 904 lines
• Testify usage: 0 require.* / assert.* calls
• Raw t.Fatal/t.Error calls: 106 occurrences

Test Quality Analysis

Strengths ✅

• Uses table-driven tests with t.Run() for pure functions (e.g., TestExtractHostFromRemoteURL, TestResolveRemoteURL)
• Good coverage of core Git operations: branch switching, commit, remote URL parsing, workflow file status
• Test isolation with testutil.TempDir and directory restoration via defer

🎯 Areas for Improvement

1. Replace All Raw Assertions with Testify

Current Issue: The entire file uses t.Fatalf, t.Fatal, t.Error, and t.Errorf (106 occurrences). Not a single require.* or assert.* call exists. This diverges from the codebase standard.

Examples:

// ❌ CURRENT — verbose and inconsistent
err := os.Chdir(tmpDir)
if err != nil {
    t.Fatalf("Failed to change to temp directory: %v", err)
}
// ...
if got != tt.expected {
    t.Errorf("extractHostFromRemoteURL(%q) = %q, want %q", tt.url, got, tt.expected)
}
// ...
if branch == "" {
    t.Error("getCurrentBranch() returned empty branch name")
}

// ✅ IMPROVED — testify equivalents
err := os.Chdir(tmpDir)
require.NoError(t, err, "should change to temp directory")
// ...
assert.Equal(t, tt.expected, got, "extractHostFromRemoteURL(%q)", tt.url)
// ...
assert.NotEmpty(t, branch, "getCurrentBranch() should return a branch name")

Required import addition:

import (
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

2. Use require.* for Setup, assert.* for Validations

Current Issue: All failures use t.Fatalf indiscriminately. The distinction between "setup must succeed" vs "this is the assertion" is lost.

Pattern to apply:

• Use require.* for os.Getwd, os.Chdir, os.WriteFile, git setup — these are preconditions and a failure should stop the test immediately
• Use assert.* for the actual test assertions (checking return values, booleans, strings)

// ✅ CORRECT distinction
originalDir, err := os.Getwd()
require.NoError(t, err, "should get current directory")  // setup

branch, err := getCurrentBranch()
require.NoError(t, err, "getCurrentBranch() should succeed")  // setup

assert.NotEmpty(t, branch, "should return a branch name")    // assertion
assert.Contains(t, []string{"main", "master"}, branch, "should be on main or master")  // assertion

3. Convert In-Table Assertions to Testify

Current Issue: Table-driven tests still use t.Errorf inside t.Run:

// ❌ CURRENT
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        got := extractHostFromRemoteURL(tt.url)
        if got != tt.expected {
            t.Errorf("extractHostFromRemoteURL(%q) = %q, want %q", tt.url, got, tt.expected)
        }
    })
}

// ✅ IMPROVED — cleaner and self-documenting
for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        got := extractHostFromRemoteURL(tt.url)
        assert.Equal(t, tt.expected, got, "extractHostFromRemoteURL(%q)", tt.url)
    })
}

4. Replace Struct-Field Boolean Assertions

Current Issue: Multi-condition boolean checks are verbose and unclear which field failed:

// ❌ CURRENT — hard to diagnose on failure
if status.IsModified || status.IsStaged || status.HasUnpushedCommits {
    t.Error("Expected empty status for untracked file")
}

// ✅ IMPROVED — separate assertions give precise failure messages
assert.False(t, status.IsModified, "IsModified should be false for untracked file")
assert.False(t, status.IsStaged, "IsStaged should be false for untracked file")
assert.False(t, status.HasUnpushedCommits, "HasUnpushedCommits should be false for untracked file")

5. Simplify Setup Code with Helpers

Current Issue: Every git-operation test has 20+ lines of boilerplate for creating a temp repo. This is duplicated across TestGetCurrentBranch, TestCreateAndSwitchBranch, TestSwitchBranch, TestCommitChanges, etc.

Recommended: Extract a setupTestRepo helper:

// setupTestRepo creates a temporary git repository and returns the cleanup function.
// Callers should defer the returned cleanup.
func setupTestRepo(t *testing.T) (dir string, cleanup func()) {
    t.Helper()
    tmpDir := testutil.TempDir(t, "test-git-*")
    originalDir, err := os.Getwd()
    require.NoError(t, err, "should get current directory")

    require.NoError(t, os.Chdir(tmpDir), "should change to temp directory")

    if err := exec.Command("git", "init").Run(); err != nil {
        t.Skip("Git not available")
    }
    exec.Command("git", "config", "user.name", "Test User").Run()
    exec.Command("git", "config", "user.email", "test@example.com").Run()

    // Create initial commit
    require.NoError(t, os.WriteFile("init.txt", []byte("init"), 0644))
    exec.Command("git", "add", "init.txt").Run()
    if err := exec.Command("git", "commit", "-m", "Initial commit").Run(); err != nil {
        t.Skip("Failed to create initial commit")
    }

    return tmpDir, func() { _ = os.Chdir(originalDir) }
}

This would reduce each test by ~25 lines of boilerplate.

📋 Implementation Guidelines

Priority Order

1. High: Add require and assert imports; replace t.Fatalf/t.Error/t.Errorf with testify equivalents
2. High: Split combined boolean checks into individual assert.False/assert.True calls
3. Medium: Extract setupTestRepo helper to reduce duplication
4. Low: Add more descriptive assertion messages where currently missing

Testing Commands

# Run tests for this file
go test -v -run "TestGetCurrentBranch|TestCreateAndSwitchBranch|TestSwitchBranch|TestCommitChanges|TestCheckWorkflowFileStatus|TestExtractHostFromRemoteURL|TestGetHostFromOriginRemote|TestResolveRemoteURL|TestGetRepositorySlug" ./pkg/cli/

# Run all CLI unit tests
go test -v ./pkg/cli/

# Full validation
make test-unit

Best Practices Reminder (from scratchpad/testing.md)

• ✅ Use require.* for critical setup (stops test on failure)
• ✅ Use assert.* for test validations (continues checking)
• ✅ Always include helpful assertion messages
• ✅ No mocks — test real interactions (this file does this well already)

Acceptance Criteria

• require and assert packages imported from github.com/stretchr/testify
• All t.Fatalf(...) in setup code replaced with require.NoError(t, err, "...")
• All t.Error(...) / t.Errorf(...) in assertion code replaced with assert.*
• Combined boolean checks split into individual assertions with descriptive messages
• Optional: setupTestRepo helper extracted to reduce boilerplate
• All tests pass: go test -v ./pkg/cli/ -run "TestGetCurrentBranch|TestCreateAndSwitch|TestSwitchBranch|TestCommitChanges|TestCheckWorkflowFileStatus|TestExtractHost|TestGetHost|TestResolveRemote|TestGetRepositorySlug"
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md
• Testify Documentation: https://github.com/stretchr/testify
• Similar well-tested files: pkg/stringutil/stringutil_test.go, pkg/cli/compile_args_test.go

---

Priority: Medium
Effort: Medium (mechanical replacement, ~904 lines)
Expected Impact: Cleaner test output, better failure messages, alignment with codebase standards

Files Involved:

• Test file: pkg/cli/git_test.go
• Source file: pkg/cli/git.go

References:

• §26414245935

Generated by 🧪 Daily Testify Uber Super Expert · sonnet46 2.8M · ◷

• expires on May 27, 2026, 6:30 PM UTC

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · haiku45 30.6K · ◷