[testify-expert] Improve Test Quality: pkg/agentdrain/miner_test.go

The test file `pkg/agentdrain/miner_test.go` has been selected for quality improvement by the Testify Uber Super Expert. This issue provides specific, actionable recommendations to enhance test quality, coverage, and maintainability using testify best practices.

### Current State

- **Test File**: `pkg/agentdrain/miner_test.go`
- **Source Files**: `pkg/agentdrain/miner.go`, `pkg/agentdrain/coordinator.go`, `pkg/agentdrain/mask.go`
- **Test Functions**: 8 test functions
- **Lines of Code**: 285 lines
- **Testify Usage**: None (sibling file `anomaly_test.go` uses testify correctly)

### Test Quality Analysis

### Strengths ✅

- Good concurrency test (`TestConcurrency`) covering race conditions with `sync.WaitGroup`
- Table-driven patterns already used in `TestMasking`, `TestComputeSimilarity`, `TestMergeTemplate`
- Tests cover core happy path and some edge cases (merge, routing)

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

#### 1. No Testify Assertions

The entire file uses raw Go testing patterns while the sibling file `anomaly_test.go` correctly uses `github.com/stretchr/testify`. This inconsistency makes test failures harder to debug.

**Current Issues (all 8 test functions):**
```go
// ❌ CURRENT anti-pattern throughout miner_test.go
if err != nil {
    t.Fatalf("NewMiner: unexpected error: %v", err)
}
if m == nil {
    t.Fatal("NewMiner: expected non-nil miner")
}
if m.ClusterCount() != 0 {
    t.Errorf("NewMiner: expected 0 clusters, got %d", m.ClusterCount())
}
```

**Recommended Changes:**
```go
// ✅ IMPROVED - using testify (matches anomaly_test.go pattern)
import (
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

func TestNewMiner(t *testing.T) {
    cfg := DefaultConfig()
    m, err := NewMiner(cfg)
    require.NoError(t, err, "NewMiner should succeed with default config")
    require.NotNil(t, m, "NewMiner should return a non-nil miner")
    assert.Equal(t, 0, m.ClusterCount(), "new miner should have no clusters")
}
```

**Why this matters**: `anomaly_test.go` in the same package already uses testify correctly — `miner_test.go` should be consistent. Testify provides clearer diff output when assertions fail, reducing debugging time.

#### 2. Verbose Slice Comparison in TestMergeTemplate

The `TestMergeTemplate` function manually iterates to compare slice contents when `assert.Equal` handles slice comparison natively (deep equality).

**Current Issues:**
```go
// ❌ CURRENT - verbose manual slice comparison
got := mergeTemplate(tt.existing, tt.incoming, param)
if len(got) != len(tt.expected) {
    t.Fatalf("mergeTemplate len = %d, want %d", len(got), len(tt.expected))
}
for i, tok := range got {
    if tok != tt.expected[i] {
        t.Errorf("mergeTemplate[%d] = %q, want %q", i, tok, tt.expected[i])
    }
}
```

**Recommended Changes:**
```go
// ✅ IMPROVED - single assert handles full slice equality with readable diff
got := mergeTemplate(tt.existing, tt.incoming, param)
assert.Equal(t, tt.expected, got, "mergeTemplate should produce expected token slice")
```

**Why this matters**: Testify's `assert.Equal` uses `reflect.DeepEqual` for slices, producing a full diff on failure (shows actual vs expected values) instead of reporting only the first differing index.

#### 3. Non-Table-Driven Tests Should Be Refactored

`TestTrain_ClusterCreation`, `TestTrain_ClusterMerge`, and `TestStageRouting` test multiple scenarios without using `t.Run()` subtests. These could be table-driven for easier extension.

**Current Issues:**
```go
// ❌ CURRENT - separate test functions for similar scenarios
func TestTrain_ClusterCreation(t *testing.T) { ... }
func TestTrain_ClusterMerge(t *testing.T) { ... }
```

**Recommended Changes:**
```go
// ✅ IMPROVED - table-driven test covers all Train scenarios
func TestTrain(t *testing.T) {
    tests := []struct {
        name          string
        cfgOverride   func(*Config)
        lines         []string
        wantClusters  int
        wantWildcard  bool
    }{
        {
            name:         "single line creates one cluster",
            lines:        []string{"stage=plan action=start"},
            wantClusters: 1,
            wantWildcard: false,
        },
        {
            name:  "similar lines merge into one cluster",
            cfgOverride: func(c *Config) { c.SimThreshold = 0.4 },
            lines:        []string{"stage=tool_call tool=search", "stage=tool_call tool=read_file"},
            wantClusters: 1,
            wantWildcard: true,
        },
        {
            name:  "dissimilar lines stay in separate clusters",
            cfgOverride: func(c *Config) { c.SimThreshold = 0.9 },
            lines:        []string{"stage=plan action=start", "stage=finish status=ok"},
            wantClusters: 2,
            wantWildcard: false,
        },
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            cfg := DefaultConfig()
            if tt.cfgOverride != nil {
                tt.cfgOverride(&cfg)
            }
            m, err := NewMiner(cfg)
            require.NoError(t, err, "NewMiner should succeed")

            var lastResult *MatchResult
            for _, line := range tt.lines {
                lastResult, err = m.Train(line)
                require.NoError(t, err, "Train should succeed for line: %q", line)
            }
            assert.Equal(t, tt.wantClusters, m.ClusterCount(), "cluster count mismatch")
            if tt.wantWildcard {
                require.NotNil(t, lastResult)
                assert.Contains(t, lastResult.Template, "<*>", "merged template should contain wildcard")
            }
        })
    }
}
```

#### 4. Missing Test Coverage

Several exported functions and methods lack any test coverage:

| Function/Method | File | Priority |
|---|---|---|
| `Miner.TrainEvent` | `miner.go` | **High** — separate from `Train`; uses `FlattenEvent` |
| `Miner.Clusters()` | `miner.go` | **High** — verifies snapshot of cluster state |
| `Coordinator.AnalyzeEvent` | `coordinator.go` | **High** — anomaly detection via coordinator |
| `Coordinator.AllClusters` | `coordinator.go` | **Medium** — per-stage cluster map |
| `Coordinator.SaveSnapshots` / `LoadSnapshots` | `coordinator.go` | **Medium** — persistence round-trip |
| `Coordinator.SaveWeightsJSON` / `LoadWeightsJSON` | `coordinator.go` | **Medium** — JSON serialization |
| `StageSequence` | `coordinator.go` | **Medium** — sequence string from events |
| `Tokenize` | `mask.go` | **Low** — already used internally |

**Example tests to add:**
```go
func TestMiner_TrainEvent(t *testing.T) {
    m, err := NewMiner(DefaultConfig())
    require.NoError(t, err)

    evt := AgentEvent{Stage: "plan", Fields: map[string]string{"action": "start"}}
    result, err := m.TrainEvent(evt)
    require.NoError(t, err, "TrainEvent should succeed")
    require.NotNil(t, result, "TrainEvent should return a result")
    assert.Equal(t, 1, m.ClusterCount(), "one cluster after first TrainEvent")
}

func TestMiner_Clusters(t *testing.T) {
    m, err := NewMiner(DefaultConfig())
    require.NoError(t, err)

    assert.Empty(t, m.Clusters(), "new miner should have empty clusters slice")

    _, err = m.Train("stage=plan action=start")
    require.NoError(t, err)
    clusters := m.Clusters()
    assert.Len(t, clusters, 1, "should have one cluster after training")
    assert.NotEmpty(t, clusters[0].Template, "cluster template should not be empty")
}

func TestCoordinator_PersistenceRoundTrip(t *testing.T) {
    stages := []string{"plan", "tool_call"}
    coord, err := NewCoordinator(DefaultConfig(), stages)
    require.NoError(t, err)

    evt := AgentEvent{Stage: "plan", Fields: map[string]string{"action": "start"}}
    _, err = coord.TrainEvent(evt)
    require.NoError(t, err)

    // SaveSnapshots + LoadSnapshots round-trip
    snapshots, err := coord.SaveSnapshots()
    require.NoError(t, err, "SaveSnapshots should succeed")
    assert.NotEmpty(t, snapshots, "snapshots should contain data")

    coord2, err := NewCoordinator(DefaultConfig(), stages)
    require.NoError(t, err)
    err = coord2.LoadSnapshots(snapshots)
    require.NoError(t, err, "LoadSnapshots should succeed")
    assert.Equal(t, coord.AllClusters(), coord2.AllClusters(), "loaded coordinator should have same clusters")
}

func TestStageSequence(t *testing.T) {
    tests := []struct {
        name     string
        events   []AgentEvent
        expected string
    }{
        {
            name:     "empty events",
            events:   []AgentEvent{},
            expected: "",
        },
        {
            name: "single event",
            events: []AgentEvent{
                {Stage: "plan"},
            },
            expected: "plan",
        },
        {
            name: "multiple events",
            events: []AgentEvent{
                {Stage: "plan"},
                {Stage: "tool_call"},
                {Stage: "finish"},
            },
            expected: "plan→tool_call→finish",
        },
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got := StageSequence(tt.events)
            assert.Equal(t, tt.expected, got, "StageSequence result mismatch")
        })
    }
}
```

#### 5. Assertion Messages Missing in Table-Driven Tests

`TestComputeSimilarity` and `TestMasking` compare values without assertion messages, making failures harder to pin to specific test cases.

**Current Issues:**
```go
// ❌ CURRENT - in TestComputeSimilarity
got := computeSimilarity(tt.a, tt.b, param)
if got != tt.expected {
    t.Errorf("computeSimilarity = %v, want %v", got, tt.expected)
}
```

**Recommended Changes:**
```go
// ✅ IMPROVED - testify with message
got := computeSimilarity(tt.a, tt.b, param)
assert.InDelta(t, tt.expected, got, 1e-9, "computeSimilarity(%v, %v) mismatch", tt.a, tt.b)
```

Note: `assert.InDelta` is preferred over `assert.Equal` for float comparisons to avoid floating-point precision issues — consistent with `anomaly_test.go` which uses `assert.InDelta(t, tt.wantScore, report.AnomalyScore, 1e-9, ...)`.

</details>

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

#### Priority Order
1. **High**: Add testify imports and replace all raw `t.Fatal`/`t.Errorf` calls
2. **High**: Add missing tests for `Miner.TrainEvent`, `Miner.Clusters()`, `Coordinator.AnalyzeEvent`
3. **Medium**: Add persistence round-trip tests (`SaveSnapshots`/`LoadSnapshots`, `SaveWeightsJSON`/`LoadWeightsJSON`)
4. **Medium**: Replace manual slice comparison in `TestMergeTemplate` with `assert.Equal`
5. **Medium**: Add `TestStageSequence` as a new table-driven test
6. **Low**: Use `assert.InDelta` for float comparisons in `TestComputeSimilarity`

#### Best Practices from `scratchpad/testing.md`
- ✅ Use `require.*` for critical setup (stops test on failure)
- ✅ Use `assert.*` for test validations (continues checking)
- ✅ Write table-driven tests with `t.Run()` and descriptive names
- ✅ No mocks or test suites — test real component interactions
- ✅ Always include helpful assertion messages

#### Reference: anomaly_test.go (same package, good pattern)
The sibling file `anomaly_test.go` demonstrates all the correct patterns this file should follow — use it as the reference implementation when refactoring.

#### Testing Commands
```bash
# Run tests for this package
go test -v ./pkg/agentdrain/ -run TestMiner
go test -v ./pkg/agentdrain/ -run TestCoordinator
go test -v ./pkg/agentdrain/ -run TestStageSequence

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

# Run all unit tests
make test-unit
```

</details>

### Acceptance Criteria

- [ ] `miner_test.go` imports `github.com/stretchr/testify/assert` and `github.com/stretchr/testify/require`
- [ ] All `t.Fatal`/`t.Fatalf` for setup replaced with `require.NoError` / `require.NotNil`
- [ ] All `t.Errorf` comparisons replaced with `assert.Equal` / `assert.Contains` / `assert.InDelta`
- [ ] Manual slice comparison loop in `TestMergeTemplate` replaced with `assert.Equal`
- [ ] `TestTrain` refactored to table-driven covering creation, merge, and non-merge scenarios
- [ ] New tests added for `TrainEvent`, `Clusters()`, `Coordinator.AnalyzeEvent`, `StageSequence`
- [ ] Persistence round-trip test added for `SaveSnapshots`/`LoadSnapshots`
- [ ] All assertions include descriptive messages
- [ ] `go test -race ./pkg/agentdrain/` passes
- [ ] `make test-unit` passes

### Additional Context

- **Repository Testing Guidelines**: See `scratchpad/testing.md` for comprehensive testing patterns
- **Reference Implementation**: `pkg/agentdrain/anomaly_test.go` — same package, correct testify usage
- **Testify Documentation**: https://github.com/stretchr/testify

---

**Priority**: Medium  
**Effort**: Medium (refactor ~285 lines + add ~120 lines of new coverage)  
**Expected Impact**: Improved debuggability, consistency with package conventions, better coverage of persistence and coordinator features

**Files Involved:**
- Test file: `pkg/agentdrain/miner_test.go`
- Source files: `pkg/agentdrain/miner.go`, `pkg/agentdrain/coordinator.go`, `pkg/agentdrain/mask.go`

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







> Generated by [Daily Testify Uber Super Expert](https://github.com/github/gh-aw/actions/runs/24030272316/agentic_workflow) · ● 2.3M · [◷](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 8, 2026, 11:44 AM UTC

Function/Method	File	Priority
`Miner.TrainEvent`	`miner.go`	High — separate from `Train`; uses `FlattenEvent`
`Miner.Clusters()`	`miner.go`	High — verifies snapshot of cluster state
`Coordinator.AnalyzeEvent`	`coordinator.go`	High — anomaly detection via coordinator
`Coordinator.AllClusters`	`coordinator.go`	Medium — per-stage cluster map
`Coordinator.SaveSnapshots` / `LoadSnapshots`	`coordinator.go`	Medium — persistence round-trip
`Coordinator.SaveWeightsJSON` / `LoadWeightsJSON`	`coordinator.go`	Medium — JSON serialization
`StageSequence`	`coordinator.go`	Medium — sequence string from events
`Tokenize`	`mask.go`	Low — already used internally

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[testify-expert] Improve Test Quality: pkg/agentdrain/miner_test.go #24868

Current State

Test Quality Analysis

Strengths ✅

1. No Testify Assertions

2. Verbose Slice Comparison in TestMergeTemplate

3. Non-Table-Driven Tests Should Be Refactored

4. Missing Test Coverage

5. Assertion Messages Missing in Table-Driven Tests

Priority Order

Best Practices from `scratchpad/testing.md`

Reference: anomaly_test.go (same package, good pattern)

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

[testify-expert] Improve Test Quality: pkg/agentdrain/miner_test.go #24868

Description

Current State

Test Quality Analysis

Strengths ✅

1. No Testify Assertions

2. Verbose Slice Comparison in TestMergeTemplate

3. Non-Table-Driven Tests Should Be Refactored

4. Missing Test Coverage

5. Assertion Messages Missing in Table-Driven Tests

Priority Order

Best Practices from scratchpad/testing.md

Reference: anomaly_test.go (same package, good pattern)

Testing Commands

Acceptance Criteria

Additional Context

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions

Best Practices from `scratchpad/testing.md`