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

[github-actions]

Overview

Item	Value
File	pkg/agentdrain/anomaly_test.go
Source pair	pkg/agentdrain/anomaly.go
Test functions	7
LOC	584
assert.* calls	32
require.* calls	30

Strengths

• Excellent table-driven coverage in TestAnomalyDetector_Analyze with boundary conditions (exact-threshold, zero-threshold, nil-cluster).
• anomalyScore helper mirrors production constants — gives compile-time sync between test expectations and source weights.
• Appropriate require.* for setup / fatal conditions and assert.* for non-fatal field checks.
• TestAnalyze_FlagMutualExclusivity explicitly guards the IsNewTemplate && LowSimilarity invariant.

Prioritized Improvements

1. Missing / High-Value Tests

a) Score clamping path is never exercised

Analyze contains a defensive clamp (if score > AnomalyMaxScore { score = AnomalyMaxScore }). Production logic prevents this today (mutual exclusivity of IsNewTemplate/LowSimilarity), but future weight changes could trigger it. Calling buildReason directly with all three flags set doesn't exercise the clamp in Analyze. A dedicated test should call Analyze via a test double or by temporarily constructing a scenario where the flags breach AnomalyMaxScore.

Since the flag invariant is enforced inside Analyze, the simplest approach is testing the score never exceeds 1.0 with a property-style loop over all flag combinations. For example:

func TestAnalyze_ScoreAlwaysNormalized(t *testing.T) {
    t.Parallel()
    d, err := NewAnomalyDetector(0.0, 100) // both thresholds trigger easily
    require.NoError(t, err)

    result := &MatchResult{ClusterID: 1, Similarity: 0.0}
    cluster := &Cluster{ID: 1, Size: 1}
    for _, isNew := range []bool{true, false} {
        report := d.Analyze(result, isNew, cluster)
        require.NotNil(t, report)
        assert.LessOrEqual(t, report.AnomalyScore, 1.0, "AnomalyScore must not exceed 1.0 (isNew=%v)", isNew)
        assert.GreaterOrEqual(t, report.AnomalyScore, 0.0, "AnomalyScore must be non-negative (isNew=%v)", isNew)
    }
}

b) NewAnomalyDetector — valid boundary values not stored-field verified

Current TestNewAnomalyDetector_ThresholdBoundaries only checks that valid calls return no error and a non-nil detector. It never verifies that the stored threshold and rareThreshold are actually used in a subsequent Analyze call. Consider extending the success cases:

// After successful construction, confirm the thresholds are wired correctly.
detector, err := NewAnomalyDetector(0.0, 0)
require.NoError(t, err)
// A similarity of 0.0 is not < 0.0 → LowSimilarity must be false.
r := detector.Analyze(&MatchResult{Similarity: 0.0}, false, nil)
assert.False(t, r.LowSimilarity, "similarity == threshold=0.0 should not be flagged")

2. Testify Assertion Upgrades

TestBuildReason — use require for the nil guard

buildReason currently accepts a pointer (*AnomalyReport). If the pointer is nil, it panics. The test never exercises nil input, but more importantly, test helpers creating &AnomalyReport{} inline could be riskier if the type changes. Low risk today, but worth documenting with a require.NotNil before dereferencing in any future pointer-passing helper:

// Before (implicit trust in the literal):
r := &AnomalyReport{...}
got := buildReason(r)
assert.Equal(t, tt.wantReason, got)

// After (explicit guard):
r := &AnomalyReport{...}
require.NotNil(t, r) // defensive; cheap, documents intent
got := buildReason(r)
assert.Equal(t, tt.wantReason, got)

3. Table-Driven Refactors

TestAnalyzeEvent — shared mutable state is fragile

TestAnalyzeEvent uses three sequential sub-tests that mutate a shared *Miner. The comment correctly warns against t.Parallel(), but the pattern means:

• A failure in sub-test 1 silently invalidates sub-tests 2 and 3 (state is dirty).
• Adding a 4th step requires reasoning about global miner state.

Recommendation: Either:

• Use require gates already present (good), and add a comment explaining this is intentional stateful integration, OR
• Extract into a top-level integration-style test with explicit state-transition assertions, keeping TestAnalyzeEvent_Variants parallel and stateless.

Sketch of the stateful integration test as named steps

func TestAnalyzeEvent_StateProgression(t *testing.T) {
    // NOT parallel — tests miner state machine progression.
    cfg := DefaultConfig()
    m, err := NewMiner(cfg)
    require.NoError(t, err)

    evt := AgentEvent{Stage: "plan", Fields: map[string]string{"action": "start"}}

    // Step 1: first observation creates new template.
    r1, rep1, err1 := m.AnalyzeEvent(evt)
    require.NoError(t, err1, "step 1")
    require.NotNil(t, r1)
    require.True(t, rep1.IsNewTemplate, "step 1: must be new")

    // Step 2: identical observation reuses template.
    r2, rep2, err2 := m.AnalyzeEvent(evt)
    require.NoError(t, err2, "step 2")
    require.NotNil(t, r2)
    assert.False(t, rep2.IsNewTemplate, "step 2: must reuse template")
    assert.Equal(t, r1.ClusterID, r2.ClusterID, "step 2: same cluster expected")
}

4. Organization / Readability

• TestBuildReason is missing t.Parallel() at the top level (the sub-tests add it, but the outer t could also be parallelized since it only creates the sub-test table without shared state).

// Add at the top of TestBuildReason:
func TestBuildReason(t *testing.T) {
    t.Parallel() // ← add this
    tests := []struct { ... }{ ... }
    ...
}

Acceptance Checklist

• Add TestAnalyze_ScoreAlwaysNormalized covering both isNew=true and isNew=false paths with thresholds that maximize all applicable flags.
• Extend TestNewAnomalyDetector_ThresholdBoundaries success cases to verify thresholds are actually applied in Analyze.
• Add t.Parallel() to TestBuildReason outer function.
• Add a comment or refactor TestAnalyzeEvent to make the intentional sequential/stateful dependency explicit and resilient to future additions.
• Run make test-unit and confirm all tests pass with no race conditions (go test -race ./pkg/agentdrain/...).

Generated by 🧪 Daily Testify Uber Super Expert · 36.7 AIC · ⌖ 13.8 AIC · ⊞ 5.2K · ◷

• expires on Jul 21, 2026, 10:22 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! 🍪

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• pi.dev

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pi.dev"

See Network Configuration for more information.

🍪 Om nom nom by Issue Monster · 0.675 AIC · ⌖ 4.88 AIC · ⊞ 6K · ◷