[testify-expert] Improve Test Quality: pkg/repoutil/repoutil_test.go

The test file `pkg/repoutil/repoutil_test.go` was selected for quality improvement by the Daily Testify Uber Super Expert. The file uses **no testify assertions at all** — only the standard `testing` package — and has a number of actionable improvements available.

### Current State

- **Test File**: `pkg/repoutil/repoutil_test.go`
- **Source File**: `pkg/repoutil/repoutil.go` (1 exported function: `SplitRepoSlug`)
- **Test Functions**: 7 test functions + 3 benchmarks
- **Lines of Code**: 224 lines
- **Testify usage**: ❌ None — only `"testing"` imported

### Test Quality Analysis

### Strengths ✅

- Good table-driven test structure with descriptive names across most test functions
- Covers a broad range of cases: valid slugs, invalid slugs, special characters, whitespace, idempotency
- Benchmark tests included (`BenchmarkSplitRepoSlug`, `BenchmarkSplitRepoSlug_Valid`, `BenchmarkSplitRepoSlug_Invalid`)

<details>
<summary><b>🎯 Areas for Improvement</b></summary>

#### 1. No Testify Assertions — Replace All Manual Checks

The entire file uses raw `t.Errorf` / `t.Error` comparisons instead of testify's `require.*` and `assert.*` helpers. This is the single biggest issue.

**Current Issues:**

```go
// ❌ CURRENT — manual error check, no testify
if tt.expectError {
    if err == nil {
        t.Errorf("SplitRepoSlug(%q) expected error, got nil", tt.slug)
    }
} else {
    if err != nil {
        t.Errorf("SplitRepoSlug(%q) unexpected error: %v", tt.slug, err)
    }
    if owner != tt.expectedOwner {
        t.Errorf("SplitRepoSlug(%q) owner = %q; want %q", tt.slug, owner, tt.expectedOwner)
    }
    if repo != tt.expectedRepo {
        t.Errorf("SplitRepoSlug(%q) repo = %q; want %q", tt.slug, repo, tt.expectedRepo)
    }
}
```

**Recommended Changes:**

```go
// ✅ IMPROVED — testify with require/assert
if tt.expectError {
    require.Error(t, err, "SplitRepoSlug(%q) should return an error", tt.slug)
} else {
    require.NoError(t, err, "SplitRepoSlug(%q) should not return an error", tt.slug)
    assert.Equal(t, tt.expectedOwner, owner, "owner should match for slug %q", tt.slug)
    assert.Equal(t, tt.expectedRepo,  repo,  "repo should match for slug %q", tt.slug)
}
```

This pattern should be applied consistently to:
- `TestSplitRepoSlug` (the main table-driven test)
- `TestSplitRepoSlug_Whitespace`
- `TestSplitRepoSlug_SpecialCharacters`

**Why this matters**: Testify provides clearer failure output, is the standard used throughout this codebase (see `scratchpad/testing.md`), and `require.*` stops the test immediately on critical failures rather than continuing with a potentially broken state.

---

#### 2. `TestSplitRepoSlug_SpecialCharacters` — Split the Combined Assertion

The combined owner/repo check loses information when it fails:

```go
// ❌ CURRENT — single Errorf that doesn't tell you which field was wrong
if owner != tt.expectedOwner || repo != tt.expectedRepo {
    t.Errorf("SplitRepoSlug(%q) = (%q, %q); want (%q, %q)",
        tt.slug, owner, repo, tt.expectedOwner, tt.expectedRepo)
}
```

```go
// ✅ IMPROVED — separate assertions, easier to diagnose failures
require.NoError(t, err, "SplitRepoSlug(%q) should not return an error", tt.slug)
assert.Equal(t, tt.expectedOwner, owner, "owner should match for slug %q", tt.slug)
assert.Equal(t, tt.expectedRepo,  repo,  "repo should match for slug %q", tt.slug)
```

---

#### 3. `TestSplitRepoSlug_Idempotent` — Convert to Testify

The idempotent test uses a raw loop without `t.Run` and manual `t.Error`:

```go
// ❌ CURRENT — no t.Run, no testify, continues on error (risky)
for _, slug := range slugs {
    owner, repo, err := SplitRepoSlug(slug)
    if err != nil {
        t.Errorf("Unexpected error for slug %q: %v", slug, err)
        continue
    }
    rejoined := owner + "/" + repo
    if rejoined != slug {
        t.Errorf("Split and rejoin changed slug: %q -> %q", slug, rejoined)
    }
}
```

```go
// ✅ IMPROVED — table-driven with t.Run and testify
func TestSplitRepoSlug_Idempotent(t *testing.T) {
    slugs := []string{
        "owner/repo",
        "github-next/gh-aw",
        "my_org/my_repo",
        "org123/repo456",
    }

    for _, slug := range slugs {
        t.Run(slug, func(t *testing.T) {
            owner, repo, err := SplitRepoSlug(slug)
            require.NoError(t, err, "SplitRepoSlug(%q) should succeed", slug)
            assert.Equal(t, slug, owner+"/"+repo, "split-and-rejoin should be identity for %q", slug)
        })
    }
}
```

---

#### 4. `TestSplitRepoSlug_Whitespace` — Verify Returned Values

The whitespace tests only check whether an error occurred but never assert what `owner` and `repo` contain. Given the comments ("Will split but owner will have space"), the actual returned values should be asserted to document — and enforce — the behaviour:

```go
// ✅ IMPROVED — assert actual values, not just no-error
{
    name:          "leading whitespace",
    slug:          " owner/repo",
    expectedOwner: " owner",
    expectedRepo:  "repo",
    expectError:   false,
},
// ... etc.

// In the test body:
if tt.expectError {
    require.Error(t, err, "expected error for slug %q", tt.slug)
} else {
    require.NoError(t, err, "unexpected error for slug %q", tt.slug)
    assert.Equal(t, tt.expectedOwner, owner)
    assert.Equal(t, tt.expectedRepo,  repo)
}
```

---

#### 5. Missing `require` Import

The file currently only imports `"testing"`. After applying the changes above, the import block should be:

```go
import (
    "testing"

    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)
```

</details>

<details>
<summary><b>📋 Implementation Guidelines</b></summary>

#### Priority Order
1. **High**: Add testify imports and replace all `t.Errorf` / `t.Error` with `require.*` / `assert.*`
2. **High**: Split the combined owner/repo comparison in `TestSplitRepoSlug_SpecialCharacters`
3. **Medium**: Refactor `TestSplitRepoSlug_Idempotent` to use `t.Run` + testify
4. **Medium**: Add expected-value assertions to `TestSplitRepoSlug_Whitespace`

#### Best Practices from `scratchpad/testing.md`
- ✅ Use `require.*` for critical setup and error-path checks (stops test on failure)
- ✅ Use `assert.*` for value comparisons (continues checking all fields)
- ✅ Write table-driven tests with `t.Run()` and descriptive names
- ✅ Always include helpful assertion messages

#### Testing Commands
```bash
# Run only this package's tests
go test -v ./pkg/repoutil/

# Run with race detector
go test -race ./pkg/repoutil/

# Run full unit suite before committing
make test-unit
```

</details>

### Acceptance Criteria

- [ ] `"github.com/stretchr/testify/assert"` and `"github.com/stretchr/testify/require"` imported
- [ ] All `if err == nil { t.Errorf(...) }` / `if err != nil { t.Errorf(...) }` replaced with `require.Error` / `require.NoError`
- [ ] All manual value comparisons (`if owner != tt.expectedOwner { t.Errorf(...) }`) replaced with `assert.Equal`
- [ ] `TestSplitRepoSlug_Idempotent` uses `t.Run` and testify assertions
- [ ] `TestSplitRepoSlug_Whitespace` asserts the returned `owner` / `repo` values
- [ ] All assertions include helpful messages
- [ ] Tests pass: `go test -v ./pkg/repoutil/`
- [ ] Code follows patterns in `scratchpad/testing.md`

### Additional Context

- **Repository Testing Guidelines**: See `scratchpad/testing.md` for comprehensive testing patterns
- **Testify Documentation**: https://github.com/stretchr/testify
- **Example well-tested file**: `pkg/gitutil/gitutil_test.go` (same package style, already uses testify correctly)

---

**Priority**: Medium  
**Effort**: Small (mostly mechanical replacement)  
**Expected Impact**: Clearer test failure messages, consistent assertion style, easier to extend

**Files Involved:**
- Test file: `pkg/repoutil/repoutil_test.go`
- Source file: `pkg/repoutil/repoutil.go`

**References:**
- [§23898390063](https://github.com/github/gh-aw/actions/runs/23898390063)







> Generated by [Daily Testify Uber Super Expert](https://github.com/github/gh-aw/actions/runs/23898390063/agentic_workflow) · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw+is%3Aissue+%22gh-aw-workflow-call-id%3A+github%2Fgh-aw%2Fdaily-testify-uber-super-expert%22&type=issues)
> - [x] expires  on Apr 4, 2026, 11:42 AM UTC

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[testify-expert] Improve Test Quality: pkg/repoutil/repoutil_test.go #24079

Current State

Test Quality Analysis

Strengths ✅

1. No Testify Assertions — Replace All Manual Checks

2. `TestSplitRepoSlug_SpecialCharacters` — Split the Combined Assertion

3. `TestSplitRepoSlug_Idempotent` — Convert to Testify

4. `TestSplitRepoSlug_Whitespace` — Verify Returned Values

5. Missing `require` Import

Priority Order

Best Practices from `scratchpad/testing.md`

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

[testify-expert] Improve Test Quality: pkg/repoutil/repoutil_test.go #24079

Description

Current State

Test Quality Analysis

Strengths ✅

1. No Testify Assertions — Replace All Manual Checks

2. TestSplitRepoSlug_SpecialCharacters — Split the Combined Assertion

3. TestSplitRepoSlug_Idempotent — Convert to Testify

4. TestSplitRepoSlug_Whitespace — Verify Returned Values

5. Missing require Import

Priority Order

Best Practices from scratchpad/testing.md

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions

2. `TestSplitRepoSlug_SpecialCharacters` — Split the Combined Assertion

3. `TestSplitRepoSlug_Idempotent` — Convert to Testify

4. `TestSplitRepoSlug_Whitespace` — Verify Returned Values

5. Missing `require` Import

Best Practices from `scratchpad/testing.md`