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

[github-actions]

Overview

File: pkg/agentdrain/anomaly_test.go
Source pair: pkg/agentdrain/anomaly.go ✅
Test functions: 5 (TestAnomalyDetector_Analyze, TestNewAnomalyDetector_ThresholdBoundaries, TestBuildReason, TestAnalyzeEvent, TestAnalyze_FlagMutualExclusivity)
LOC: 465
Table-driven tests: 4 of 5 functions use tables (13 + 5 + 8 + 3 cases)

Strengths

• Excellent boundary-condition coverage (threshold = 0, exactly at threshold, just below threshold).
• Correct require / assert split: setup steps use require, behavioral assertions use assert.
• Inline score arithmetic comments explain expected values clearly.
• Nil-cluster guard covered explicitly ("nil cluster does not trigger rare cluster flag").

Prioritized Improvements

1. Missing / High-Value Tests

1a. Analyze with a nil result panics — no test covers this

In anomaly.go line 42 the implementation accesses result.Similarity without guarding against result == nil:

LowSimilarity: !isNew && result.Similarity < d.threshold,

Passing nil for result causes an immediate nil-pointer dereference. Add either a defensive guard in the source or a test that documents the expected behaviour (panic / error / zero-value report). A test making the contract explicit:

// Before (no test)
// After
func TestAnalyze_NilResult_Panics(t *testing.T) {
    d, err := NewAnomalyDetector(0.4, 2)
    require.NoError(t, err)
    assert.Panics(t, func() {
        d.Analyze(nil, false, nil)
    }, "Analyze should panic on nil result until a nil guard is added")
}

Alternatively, harden the source and add a success-path test once the guard exists.

1b. TestAnalyzeEvent uses a single sequential scenario — no error-path coverage

The existing TestAnalyzeEvent is a straight-line narrative test that does not cover:

• AnalyzeEvent with an empty AgentEvent.Stage.
• AnalyzeEvent after the miner has been trained to a state where similarity drops below the threshold for a known template.
• What happens when Fields is nil vs empty map.

Suggested table:

func TestAnalyzeEvent_Variants(t *testing.T) {
    tests := []struct {
        name          string
        event         AgentEvent
        wantErr       bool
        wantNewTpl    bool
    }{
        {
            name:       "empty stage is handled without panic",
            event:      AgentEvent{Stage: "", Fields: map[string]string{"k": "v"}},
            wantNewTpl: true,
        },
        {
            name:       "nil fields map is handled",
            event:      AgentEvent{Stage: "plan", Fields: nil},
            wantNewTpl: true,
        },
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            m, err := NewMiner(DefaultConfig())
            require.NoError(t, err)
            _, report, err := m.AnalyzeEvent(tt.event)
            if tt.wantErr {
                require.Error(t, err)
                return
            }
            require.NoError(t, err)
            assert.Equal(t, tt.wantNewTpl, report.IsNewTemplate)
        })
    }
}

1c. Score normalization ceiling (score > maxScore) path is never exercised

The defensive if score > maxScore { score = maxScore } guard in anomaly.go lines 61–63 is dead code under current flag semantics. The comment acknowledges this. A table-driven unit test that calls Analyze via reflection or a mock to trigger score > 1.0 would preserve the intent — or the comment should be updated to say the guard is intentionally untested until weights change.

2. Testify Assertion Upgrades

2a. Replace paired assert.True / assert.False with assert.Equal for bool fields already covered by table fields

In TestAnalyze_FlagMutualExclusivity lines 460–461:

// Before
assert.False(t, report.IsNewTemplate && report.LowSimilarity,
    "IsNewTemplate and LowSimilarity must be mutually exclusive")
assert.GreaterOrEqual(t, report.AnomalyScore, 0.0, "AnomalyScore must stay within [0,1]")
assert.LessOrEqual(t, report.AnomalyScore, 1.0, "AnomalyScore must stay within [0,1]")

// After — consolidate range check
assert.InDelta(t, 0.0, min(report.AnomalyScore, 0.0), 1.0,
    "AnomalyScore must be in [0,1]")
// Or (clearer):
assert.True(t, report.AnomalyScore >= 0.0 && report.AnomalyScore <= 1.0,
    "AnomalyScore=%v must be in [0,1]", report.AnomalyScore)

Alternatively, add wantScore float64 to the table and assert with assert.InDelta (as done in TestAnomalyDetector_Analyze) so the range is implicitly verified by the exact value.

2b. assert.Nil on detector in error-path: use require.Nil to stop execution earlier

In TestNewAnomalyDetector_ThresholdBoundaries lines 277–280, the error path uses:

require.Error(t, err, ...)
assert.Contains(t, err.Error(), tt.wantErr, ...)
assert.Nil(t, detector, ...)   // <-- could be require.Nil
return

Since return follows immediately, assert.Nil vs require.Nil makes no practical difference here, but using require.Nil is semantically clearer (the detector being non-nil is a setup invariant, not a soft assertion).

3. Table-Driven Refactor

3a. TestAnalyzeEvent should be converted to a table (or broken into sub-tests)

The current implementation is a sequential narrative test with three manually labelled steps. A table-driven or sub-test approach makes each step independently re-runnable and filterable with -run:

// Before: one monolithic function with Step 1/2/3 comments
func TestAnalyzeEvent(t *testing.T) {
    // Step 1 ... Step 2 ... Step 3
}

// After: sub-tests
func TestAnalyzeEvent(t *testing.T) {
    t.Run("first occurrence is new template", func(t *testing.T) { ... })
    t.Run("second identical event is not new template", func(t *testing.T) { ... })
    t.Run("distinct event creates separate template", func(t *testing.T) { ... })
}

Note: each sub-test shares the same miner instance in this case (ordered by design), so keep them in a parent function rather than independent table rows.

4. Organization / Readability

4a. Add t.Parallel() to independent table-driven sub-tests

All four table-driven test functions create a fresh AnomalyDetector per sub-test; they are stateless and safe to run in parallel. Adding t.Parallel() at the sub-test level would cut wall-clock time on multi-core runners:

for _, tt := range tests {
    tt := tt // capture range variable (Go <1.22)
    t.Run(tt.name, func(t *testing.T) {
        t.Parallel()
        // ... existing assertions
    })
}

TestAnalyzeEvent must remain sequential (shared miner state).

4b. Extract score computation helper to reduce duplication

Several table-driven cases spell out the arithmetic inline (e.g., // score = (1.0 + 0.3) / 2.0 = 0.65). The comment is helpful but fragile — if the weights in anomaly.go change, the test comments become misleading. Consider a small unexported helper in the test file:

func anomalyScore(newTpl, lowSim, rare bool) float64 {
    var s float64
    if newTpl { s += 1.0 }
    if lowSim  { s += 0.7 }
    if rare    { s += 0.3 }
    const max = 2.0
    if s > max { s = max }
    return s / max
}

Then table rows set wantScore: anomalyScore(true, false, true) instead of a hardcoded float. This links test expectations directly to the weighting logic.

Acceptance Checklist

• TestAnalyze_NilResult_Panics (or a source-level nil-guard with matching test) added.
• TestAnalyzeEvent_Variants table added covering empty stage and nil fields.
• TestAnalyzeEvent refactored to use t.Run sub-tests.
• t.Parallel() added to sub-tests in stateless table-driven tests.
• assert.Nil → require.Nil in error path of TestNewAnomalyDetector_ThresholdBoundaries.
• Optional: anomalyScore helper introduced to keep expected-score values in sync with weights.
• make test-unit passes with no failures.

Generated by 🧪 Daily Testify Uber Super Expert · 64.9 AIC · ⌖ 19.9 AIC · ⊞ 5.1K · ◷

• expires on Jul 10, 2026, 10:33 AM UTC-08:00

[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 · 1.16 AIC · ⌖ 4.78 AIC · ⊞ 5.5K · ◷