[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 (90 LOC, 3 exported symbols: NewAnomalyDetector, AnomalyDetector.Analyze, buildReason)
Test LOC: 465 | Test functions: 5 | Table cases: ~37

---

Strengths

• Excellent boundary testing (rareThreshold=0, size==threshold, size==threshold+1, similarity==threshold, similarity just-below)
• Correct require/assert split — constructor steps use require, behavior checks use assert
• Good mutual-exclusivity test (TestAnalyze_FlagMutualExclusivity)
• Descriptive case names with inline arithmetic comments

---

Prioritized Improvements

1. Missing / High-Value Tests

Expand missing coverage details

a) Analyze with a nil result argument

Analyze dereferences result.Similarity without a nil guard. Passing nil would panic. A test documenting expected behavior (or a defensive nil-check + test) should exist:

// Missing: nil MatchResult should either return a safe report or be documented as unsupported
{
    name:    "nil result with existing cluster",
    result:  nil, // currently would panic
    isNew:   false,
    cluster: &Cluster{ID: 1, Size: 1},
}

b) AnalyzeEvent — missing empty-event / masking-to-empty path

AnalyzeEvent returns "empty event after masking" error but no test exercises this path:

// Add to anomaly_test.go or miner_test.go:
evtEmpty := AgentEvent{Stage: "plan", Fields: map[string]string{}}
_, _, err := m.AnalyzeEvent(evtEmpty)
// With a mask rule that wipes all tokens, this should return an error.

c) Analyze clamping branch (score > maxScore)

anomaly.go:61 clamps score > maxScore but no test triggers it. Since this is a defensive guard for future weighting changes, a comment explaining it is untestable with current flag semantics is useful.

d) buildReason integration round-trip via Analyze

All 8 buildReason permutations are tested in isolation, but the "new template + low similarity" case is documented as never produced by Analyze. An explicit assertion in TestAnalyze_FlagMutualExclusivity checking report.Reason for each case would close this gap.

2. Testify Assertion Upgrades

Expand assertion improvement details

a) Use assert.Zero for exact-zero score cases

// Before
assert.InDelta(t, 0.0, report.AnomalyScore, 1e-9, "AnomalyScore mismatch")

// After (exact-zero cases only)
assert.Zero(t, report.AnomalyScore, "AnomalyScore should be zero when no anomaly")

b) Use assert.ErrorContains instead of assert.Contains(t, err.Error(), ...)

// Before
assert.Contains(t, err.Error(), tt.wantErr, "error should describe invalid threshold")

// After — avoids manual err.Error() unwrapping, idiomatic in testify v1.7.1+
assert.ErrorContains(t, err, tt.wantErr, "error should describe invalid threshold")

3. Table-Driven Refactor

Expand refactor details

TestAnalyzeEvent uses linear steps (Step 1, Step 2, Step 3) without t.Run. Extracting steps into a sub-table improves readability and extensibility:

// Before: flat linear steps with numbered comments
resultFirst, reportFirst, errFirst := m.AnalyzeEvent(evtPlan)
require.NoError(t, errFirst, "AnalyzeEvent should not fail for first event")
assert.True(t, reportFirst.IsNewTemplate, "IsNewTemplate mismatch for first event")

// After: sub-table with t.Run
steps := []struct {
    name       string
    evt        AgentEvent
    wantNew    bool
    wantScore  float64
    wantReason string
}{
    {"first event trains new template", evtPlan, true, 0.65, "new log template discovered; rare cluster (few observations)"},
    {"second identical event matches cluster", evtPlan, false, 0.15, "rare cluster (few observations)"},
    {"distinct event creates another template", evtFinish, true, 0.65, "new log template discovered; rare cluster (few observations)"},
}
for _, step := range steps {
    t.Run(step.name, func(t *testing.T) {
        result, report, err := m.AnalyzeEvent(step.evt)
        require.NoError(t, err)
        require.NotNil(t, result)
        require.NotNil(t, report)
        assert.Equal(t, step.wantNew, report.IsNewTemplate)
        assert.InDelta(t, step.wantScore, report.AnomalyScore, 1e-9)
        assert.Equal(t, step.wantReason, report.Reason)
    })
}

4. Organization / Readability

Expand readability details

• The score arithmetic comments (// score = (1.0 + 0.3) / 2.0 = 0.65) are helpful but detached from wantScore. A named constant or code comment co-located with wantScore would survive table reordering better.
• The TestBuildReason "all flags set" comment accurately notes the combination is unreachable via Analyze. Consider adding a direct assertion to make the invariant machine-checkable.

---

Acceptance Checklist

• Add test case for Analyze with a nil result (document expected panic or add nil-guard + test)
• Add test for AnalyzeEvent returning error on empty-after-masking event
• Replace assert.Contains(t, err.Error(), ...) with assert.ErrorContains(t, err, ...) in TestNewAnomalyDetector_ThresholdBoundaries
• Replace assert.InDelta(t, 0.0, ..., 1e-9, ...) with assert.Zero(t, ...) for exact-zero score cases
• Refactor TestAnalyzeEvent linear steps into a sub-table with t.Run
• Verify all changes pass: make test-unit

Generated by 🧪 Daily Testify Uber Super Expert · 76.5 AIC · ⌖ 23.8 AIC · ⊞ 5.2K · ◷

• expires on Jul 5, 2026, 10:30 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.45 AIC · ⌖ 4.27 AIC · ⊞ 8.5K · ◷

[github-actions]

This issue was automatically closed because it expired on 2026-07-05T18:30:53.561Z.

Closed by Workflow