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

[github-actions]

The test file pkg/agentdrain/miner_test.go has been selected for quality improvement. 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/mask.go, pkg/agentdrain/coordinator.go
• Test Functions: 13 test functions
• Lines of Code: 415 lines

Strengths ✅

1. Consistent require.* / assert.* usage — Setup calls use require.NoError, require.NotNil correctly, and validations use assert.*.
2. Good table-driven coverage — TestTrain, TestMasking, TestComputeSimilarity, TestMergeTemplate, and TestStageSequence all use t.Run() with descriptive names.
3. Concurrency test included — TestConcurrency exercises Miner.Train under concurrent load, which is essential for a mutex-guarded type.

🎯 Areas for Improvement

1. Complex Assertions in TestFlattenEvent

Current Issues:
TestFlattenEvent uses assert.True with compound boolean expressions that make failure messages cryptic and hard to debug:

// ❌ CURRENT — hard to diagnose on failure
assert.True(t, len(result) > 0 &&
    indexIn(result, "latency_ms=") < indexIn(result, "query=") &&
    indexIn(result, "query=") < indexIn(result, "tool="),
    "keys should be sorted alphabetically in flattened output: %q", result)
assert.True(t, len(result) >= len("stage=tool_call") &&
    result[:len("stage=tool_call")] == "stage=tool_call",
    "stage should appear first in flattened output: %q", result)

When this fails you cannot tell which condition violated the assertion. Additionally, the helper indexIn reimplements strings.Index — it should be removed in favour of the standard library.

Recommended Changes:

// ✅ IMPROVED — granular assertions with clear messages
assert.NotEmpty(t, result, "FlattenEvent should return a non-empty string")
assert.True(t, strings.HasPrefix(result, "stage=tool_call"),
    "stage should appear first in flattened output: %q", result)
assert.NotContains(t, result, "session_id",
    "excluded field should not appear in flattened output")
// Verify alphabetical ordering of remaining keys
assert.Less(t, strings.Index(result, "latency_ms="), strings.Index(result, "query="),
    "latency_ms should appear before query in sorted output")
assert.Less(t, strings.Index(result, "query="), strings.Index(result, "tool="),
    "query should appear before tool in sorted output")

Why this matters: Each assert.* call produces an independent failure message, making it immediately obvious which property is broken.

2. TestFlattenEvent Should Be Table-Driven

Current Issues:
The test covers only a single input scenario. There are several important edge cases for FlattenEvent that are untested:

• Empty Stage field
• Empty Fields map
• All fields excluded (result becomes stage-only or empty)
• No fields excluded
• Stage is empty and Fields is empty

Recommended Changes:

func TestFlattenEvent(t *testing.T) {
    tests := []struct {
        name          string
        evt           AgentEvent
        excludeFields []string
        wantPrefix    string
        wantContain   []string
        wantExclude   []string
    }{
        {
            name: "normal event with excluded fields",
            evt: AgentEvent{
                Stage:  "tool_call",
                Fields: map[string]string{"tool": "search", "query": "foo", "session_id": "abc"},
            },
            excludeFields: []string{"session_id"},
            wantPrefix:    "stage=tool_call",
            wantContain:   []string{"tool=search", "query=foo"},
            wantExclude:   []string{"session_id"},
        },
        {
            name:          "empty stage",
            evt:           AgentEvent{Stage: "", Fields: map[string]string{"key": "val"}},
            excludeFields: []string{},
            wantContain:   []string{"key=val"},
        },
        {
            name:          "all fields excluded",
            evt:           AgentEvent{Stage: "plan", Fields: map[string]string{"a": "1"}},
            excludeFields: []string{"a"},
            wantPrefix:    "stage=plan",
        },
        {
            name:          "empty event",
            evt:           AgentEvent{},
            excludeFields: []string{},
        },
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result := FlattenEvent(tt.evt, tt.excludeFields)
            if tt.wantPrefix != "" {
                assert.True(t, strings.HasPrefix(result, tt.wantPrefix),
                    "result should start with %q, got %q", tt.wantPrefix, result)
            }
            for _, want := range tt.wantContain {
                assert.Contains(t, result, want, "result should contain %q", want)
            }
            for _, exclude := range tt.wantExclude {
                assert.NotContains(t, result, exclude, "result should not contain %q", exclude)
            }
        })
    }
}

3. Missing Test: TestTokenize

Current Issues:
Tokenize is an exported function in mask.go with no dedicated test. It is exercised indirectly via TestTrain, but edge cases are never explicitly validated:

• Empty string → empty slice
• Multiple consecutive spaces
• Leading/trailing whitespace
• Single token

Recommended Test:

func TestTokenize(t *testing.T) {
    tests := []struct {
        name     string
        input    string
        expected []string
    }{
        {name: "empty string", input: "", expected: []string{}},
        {name: "single token", input: "hello", expected: []string{"hello"}},
        {name: "multiple tokens", input: "a b c", expected: []string{"a", "b", "c"}},
        {name: "extra whitespace", input: "  a  b  ", expected: []string{"a", "b"}},
        {name: "key=value tokens", input: "stage=plan action=start", expected: []string{"stage=plan", "action=start"}},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got := Tokenize(tt.input)
            if len(tt.expected) == 0 {
                assert.Empty(t, got, "Tokenize(%q) should return empty slice", tt.input)
            } else {
                assert.Equal(t, tt.expected, got, "Tokenize(%q) mismatch", tt.input)
            }
        })
    }
}

4. Missing Error-Path Test: Train with Empty Line

Current Issues:
Miner.Train returns an error when the input line is empty after masking. This error path is never tested:

// In miner.go:
if len(tokens) == 0 {
    return nil, errors.New("agentdrain: Train: empty line after masking")
}

Recommended Addition:

func TestTrainEmptyLine(t *testing.T) {
    m, err := NewMiner(DefaultConfig())
    require.NoError(t, err, "NewMiner should succeed")

    _, err = m.Train("")
    assert.Error(t, err, "Train should return an error for an empty line")
    assert.Contains(t, err.Error(), "empty line", "error message should mention empty line")
}

5. Missing Error-Path Test: NewMasker with Invalid Regex

Current Issues:
NewMasker returns an error when a mask rule pattern fails to compile, but no test covers this path.

Recommended Addition:

func TestNewMaskerInvalidPattern(t *testing.T) {
    rules := []MaskRule{
        {Name: "bad-rule", Pattern: "[invalid", Replacement: "<X>"},
    }
    _, err := NewMasker(rules)
    assert.Error(t, err, "NewMasker should return an error for an invalid regex pattern")
    assert.Contains(t, err.Error(), "bad-rule", "error should mention the rule name")
}

6. Remove the indexIn Helper

Current Issues:
The custom indexIn helper in the test file reimplements strings.Index without using the standard library. This adds dead code risk (the linter may flag it), and is harder to read than strings.Index.

Recommended Change: Remove indexIn and replace its usages in TestFlattenEvent with strings.Index (see improvement #1 above).

📋 Implementation Guidelines

Priority Order

1. High: Add TestTrainEmptyLine and TestNewMaskerInvalidPattern — these are untested error paths
2. High: Add TestTokenize — exported function with no dedicated test
3. Medium: Refactor TestFlattenEvent into a table-driven test and remove indexIn
4. Low: Split the complex assert.True compounds into granular assertions

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 — test real component interactions
• ✅ Always include helpful assertion messages

Testing Commands

# Run tests for this package
go test -v ./pkg/agentdrain/ -run TestFlattenEvent
go test -v ./pkg/agentdrain/ -run TestTokenize
go test -v ./pkg/agentdrain/ -run TestTrain
go test -v ./pkg/agentdrain/

# Run all unit tests
make test-unit

Acceptance Criteria

• TestFlattenEvent refactored to table-driven with at least 4 cases (empty stage, all excluded, normal, empty event)
• indexIn helper removed; strings.Index used directly
• TestTokenize added with edge cases (empty string, extra whitespace, single token, key=value)
• TestTrainEmptyLine added to cover the errors.New("empty line after masking") path
• TestNewMaskerInvalidPattern added to cover invalid regex error path
• All assertions include helpful messages
• Tests pass: go test -v ./pkg/agentdrain/
• 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

---

Priority: Medium
Effort: Small
Expected Impact: Better error-path coverage, clearer failure messages, easier maintenance

Files Involved:

• Test file: pkg/agentdrain/miner_test.go
• Source files: pkg/agentdrain/miner.go, pkg/agentdrain/mask.go

References:

• Workflow Run §24628015523

Generated by Daily Testify Uber Super Expert · ● 1.3M · ◷

• expires on Apr 21, 2026, 11:35 AM 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 · ● 25.6K · ◷