Skip to content

Tighten anomaly detector test coverage and subtest structure #43364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Copilot wants to merge 10 commits into from
+1,632 −1,541
Closed

Tighten anomaly detector test coverage and subtest structure #43364

Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b2555d6
Initial plan
Copilot Jul 4, 2026
8d50d05
Improve anomaly test coverage
Copilot Jul 4, 2026
3dccf72
Refine anomaly test subtests
Copilot Jul 4, 2026
5743392
Polish anomaly test naming
Copilot Jul 4, 2026
c515a68
Merge branch 'main' into copilot/testify-expert-improve-test-quality
github-actions[bot] Jul 4, 2026
d186359
fix: add explicit nil guard in Analyze with descriptive panic message
Copilot Jul 4, 2026
898e438
refactor: address github-actions review feedback on anomaly_test.go
Copilot Jul 4, 2026
a5037b2
Merge branch 'main' into copilot/testify-expert-improve-test-quality
github-actions[bot] Jul 4, 2026
25325e1
Apply remaining changes
Copilot Jul 4, 2026
24957b1
Apply remaining changes
Copilot Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 93 additions & 29 deletions pkg/agentdrain/anomaly_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,17 @@ import (
"github.com/stretchr/testify/require"
)

func assertAnomalyScore(t *testing.T, want, got float64) {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The helper signature assertAnomalyScore(t, want, got) diverges from testify's own convention of (t, expected, actual) — but more practically, the argument order is the opposite of how assert.InDelta is called internally (assert.InDelta(t, want, got, ...)), which is correct. The concern is that callers reading the signature may accidentally swap want/got and get silent false-positives.

💡 Suggestion

Consider renaming the parameters to match testify convention (expected, actual) or adding a brief doc comment:

// assertAnomalyScore checks that actual equals expected.
// Pass expected first, actual second — matching testify convention.
func assertAnomalyScore(t *testing.T, expected, actual float64) {

@copilot please address this.

t.Helper()

if want == 0 {
assert.Zero(t, got, "AnomalyScore mismatch")
return
}

assert.InDelta(t, want, got, 1e-9, "AnomalyScore mismatch")
}

func TestAnomalyDetector_Analyze(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -227,12 +238,22 @@ func TestAnomalyDetector_Analyze(t *testing.T) {
assert.Equal(t, tt.wantIsNewTemplate, report.IsNewTemplate, "IsNewTemplate mismatch")
assert.Equal(t, tt.wantLowSimilarity, report.LowSimilarity, "LowSimilarity mismatch")
assert.Equal(t, tt.wantRareCluster, report.RareCluster, "RareCluster mismatch")
assert.InDelta(t, tt.wantScore, report.AnomalyScore, 1e-9, "AnomalyScore mismatch")
assertAnomalyScore(t, tt.wantScore, report.AnomalyScore)
assert.Equal(t, tt.wantReason, report.Reason, "Reason mismatch")
})
}
}

func TestAnomalyDetector_Analyze_PanicsOnNilResultParameter(t *testing.T) {
detector, err := NewAnomalyDetector(0.4, 2)
require.NoError(t, err, "NewAnomalyDetector should succeed")
require.NotNil(t, detector, "NewAnomalyDetector should return a non-nil detector")

assert.Panics(t, func() {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test correctly documents the current behavior, but it encodes a silent nil-dereference panic as "expected." The Analyze function guards cluster with cluster != nil, but there is no equivalent guard for result.

Consider one of:

  1. Preferred – Add a nil guard in Analyze so a nil result is treated as "no existing match":
LowSimilarity: !isNew && result != nil && result.Similarity < d.threshold,
  1. Explicit contract – Add a godoc note and deliberate panic call so callers know this is a programming error, not a runtime edge-case.

As written the test normalises a latent API hazard in the production code without any comment in anomaly.go explaining that callers must never pass (nil, false, ...).

@copilot please address this.

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] assert.Panics here documents a latent nil-dereference as expected behavior, which could mislead future readers into treating the panic as an intentional contract rather than a defect.

💡 Suggestion

If the goal is purely to document existing behavior, add an explicit comment:

// This test pins the current panic; a follow-up should make Analyze return an
// error instead so callers can handle the failure gracefully.

Or, open a companion issue and reference it here so the debt is trackable.

@copilot please address this.

detector.Analyze(nil, false, &Cluster{ID: 1, Template: []string{"a"}, Size: 1})
}, "Analyze should panic when result is nil for an existing template")
}

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panic test codifies a nil-dereference bug as an intentional API contract. The underlying cause is an unconditional nil pointer dereference in Analyze, not deliberate defensive behavior — shipping this as a passing test means future callers get a silent crash in production instead of a recoverable error.

💡 Analysis and suggested fix

The production code panics here:

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

When isNew=false and result is nil, this is a plain nil pointer dereference — there is no explicit panic or documented contract. The isNew=true path silently works fine with nil due to short-circuit evaluation, making the behavior asymmetric and surprising.

A test that asserts Panics on this path tells future developers this is correct behavior to preserve, not a bug to fix. Any future caller who passes nil (e.g., from a new code path or refactor) will get a silent crash in production.

Option 1 — Fix the bug instead of testing it:

func (d *AnomalyDetector) Analyze(result *MatchResult, isNew bool, cluster *Cluster) (*AnomalyReport, error) {
    if !isNew && result == nil {
        return nil, errors.New("agentdrain: Analyze: result must not be nil when isNew=false")
    }
    // ... rest of function
}

Option 2 — If the panic is truly intentional, add a doc comment and rename the test:

// TestAnalyzeEvent_NilResult_KnownPanic documents a known limitation: passing nil result
// with isNew=false causes a panic. TODO: replace panic with error return (#NNN).

At minimum, a passing assert.Panics test should never be the end state for unexpected nil dereferences.


func TestNewAnomalyDetector_ThresholdBoundaries(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -275,7 +296,7 @@ func TestNewAnomalyDetector_ThresholdBoundaries(t *testing.T) {
detector, err := NewAnomalyDetector(tt.simThreshold, tt.rareThreshold)
if tt.wantErr != "" {
require.Error(t, err, "NewAnomalyDetector should reject invalid thresholds")
assert.Contains(t, err.Error(), tt.wantErr, "error should describe invalid threshold")
require.ErrorContains(t, err, tt.wantErr, "error should describe invalid threshold")
assert.Nil(t, detector, "NewAnomalyDetector should return nil detector on validation error")
return
}
Expand Down Expand Up @@ -371,9 +392,6 @@ func TestBuildReason(t *testing.T) {

func TestAnalyzeEvent(t *testing.T) {
cfg := DefaultConfig()

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared evtPlan and evtFinish variables are defined in the outer function scope (lines 397-403) and captured by all subtests. The values are immutable structs so this is safe, but each subtest also creates a fresh miner while reusing the same event variables.

The issue is that the setupEvents mechanism relies on inserting evtPlan as setup before testing evtPlan again. Because the miner is fresh each time, the setup correctly seeds the miner before the actual assertion. This is fine.

However, the comment (or lack thereof) at the top of the table makes the setupEvents pattern non-obvious. Consider documenting the intent, e.g.:

// setupEvents seeds the miner before the test event so it sees a pre-warmed state.
// Every subtest gets a fresh miner; setup events are re-applied per case.

This will prevent future maintainers from removing the per-subtest miner construction (and breaking isolation) thinking it is unnecessary. @copilot please address this.

m, err := NewMiner(cfg)
require.NoError(t, err, "NewMiner should succeed")
require.NotNil(t, m, "NewMiner should return a non-nil miner")

evtPlan := AgentEvent{
Stage: "plan",
Expand All @@ -384,32 +402,78 @@ func TestAnalyzeEvent(t *testing.T) {
Fields: map[string]string{"status": "ok"},
}

// Step 1: first occurrence trains the template and is flagged as new.
resultFirst, reportFirst, errFirst := m.AnalyzeEvent(evtPlan)
require.NoError(t, errFirst, "AnalyzeEvent should not fail for first event")
require.NotNil(t, resultFirst, "AnalyzeEvent should return a non-nil result")
require.NotNil(t, reportFirst, "AnalyzeEvent should return a non-nil report")
assert.True(t, reportFirst.IsNewTemplate, "IsNewTemplate mismatch for first event")
assert.InDelta(t, 0.65, reportFirst.AnomalyScore, 1e-9, "AnomalyScore mismatch for first event")
assert.Equal(t, "new log template discovered; rare cluster (few observations)", reportFirst.Reason, "Reason mismatch for first event")
tests := []struct {
name string
setupEvents []AgentEvent
event AgentEvent
wantIsNew bool
wantScore float64
wantReason string
}{
{
name: "first occurrence trains a new template",
event: evtPlan,

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored table test drops wantLowSimilarity and wantRareCluster assertions, leaving gaps in flag-level coverage. The original linear test didn't verify these fields either, but the table struct has room and the PR description calls out flag semantics specifically.

💡 Why this matters and how to fix it

The test verifies IsNewTemplate, AnomalyScore, and Reason — but not LowSimilarity or RareCluster. These are the underlying boolean flags that drive the score and reason. A future change that flips a flag and compensates by adjusting score weights could make the reason string match while the flags are wrong, and this test would not catch it.

The existing TestAnomalyDetector_Analyze table already tests flags exhaustively via the detector directly. The gap here is that TestAnalyzeEvent goes through the full Miner path — that's a different integration surface — and it silently skips flag assertions.

Adding two fields and two assertions closes this gap with minimal effort:

tests := []struct {
    name             string
    setupEvents      []AgentEvent
    event            AgentEvent
    wantIsNew        bool
    wantLowSimilarity bool  // add
    wantRareCluster  bool   // add
    wantScore        float64
    wantReason       string
}{
    {
        name:            "first occurrence trains a new template",
        event:           evtPlan,
        wantIsNew:       true,
        wantLowSimilarity: false,
        wantRareCluster: true,
        wantScore:       0.65,
        wantReason:      "new log template discovered; rare cluster (few observations)",
    },
    // ...
}
// in the assertion block:
assert.Equal(t, tt.wantLowSimilarity, report.LowSimilarity, "LowSimilarity mismatch")
assert.Equal(t, tt.wantRareCluster, report.RareCluster, "RareCluster mismatch")

wantIsNew: true,
wantScore: 0.65,
wantReason: "new log template discovered; rare cluster (few observations)",
},
{
name: "second identical occurrence reuses the template",
setupEvents: []AgentEvent{evtPlan},
event: evtPlan,
wantIsNew: false,
wantScore: 0.15,
wantReason: "rare cluster (few observations)",
},
{
name: "distinct event creates a separate template",
setupEvents: []AgentEvent{evtPlan},
event: evtFinish,
wantIsNew: true,
wantScore: 0.65,
wantReason: "new log template discovered; rare cluster (few observations)",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new table-driven subtests are a clear improvement in isolation, but none of them call t.Parallel() — the outer loop captures tt by reference, so this is actually a pre-Go-1.22 loop-variable hazard if parallelism were added later.

💡 Suggestion

Either add t.Parallel() now (which also exercises the isolation you've built) or pin the loop variable explicitly to make it safe if parallelism is added later:

for _, tt := range tests {
    tt := tt  // pin for parallel safety (pre-Go 1.22)
    t.Run(tt.name, func(t *testing.T) {
        t.Parallel()
        // ...
    })
}

If intentionally sequential (e.g., shared miner state), add a comment explaining why.

@copilot please address this.

m, err := NewMiner(cfg)
require.NoError(t, err, "NewMiner should succeed")
require.NotNil(t, m, "NewMiner should return a non-nil miner")

for _, setupEvent := range tt.setupEvents {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The setup loop silently discards the result values; if a setup AnalyzeEvent returns non-nil results, that's unverified. More importantly, if setup fails we know it failed but we have no diagnostic about which setup event caused it.

💡 Suggestion

Include an index or event identifier in the error message so failures are easier to diagnose:

for i, setupEvent := range tt.setupEvents {
    _, _, err := m.AnalyzeEvent(setupEvent)
    require.NoError(t, err, "AnalyzeEvent setup[%d] should not fail", i)
}

@copilot please address this.

_, _, err := m.AnalyzeEvent(setupEvent)
require.NoError(t, err, "AnalyzeEvent setup should not fail")
}

result, report, err := m.AnalyzeEvent(tt.event)
require.NoError(t, err, "AnalyzeEvent should not fail")
require.NotNil(t, result, "AnalyzeEvent should return a non-nil result")
require.NotNil(t, report, "AnalyzeEvent should return a non-nil report")
assert.Equal(t, tt.wantIsNew, report.IsNewTemplate, "IsNewTemplate mismatch")
assertAnomalyScore(t, tt.wantScore, report.AnomalyScore)
assert.Equal(t, tt.wantReason, report.Reason, "Reason mismatch")
})
}
}

func TestAnalyzeEvent_EmptyAfterMasking(t *testing.T) {
cfg := DefaultConfig()
cfg.MaskRules = []MaskRule{{
Name: "eraseAllTokens",
Pattern: ".+",
Replacement: "",
}}

// Step 2: second identical occurrence reuses the trained template.
resultSecond, reportSecond, errSecond := m.AnalyzeEvent(evtPlan)
require.NoError(t, errSecond, "AnalyzeEvent should not fail for second identical event")
require.NotNil(t, resultSecond, "AnalyzeEvent should return a non-nil result")
require.NotNil(t, reportSecond, "AnalyzeEvent should return a non-nil report")
assert.False(t, reportSecond.IsNewTemplate, "IsNewTemplate mismatch for second identical event")
assert.InDelta(t, 0.15, reportSecond.AnomalyScore, 1e-9, "AnomalyScore mismatch for second identical event")
assert.Equal(t, "rare cluster (few observations)", reportSecond.Reason, "Reason mismatch for second identical event")
m, err := NewMiner(cfg)
require.NoError(t, err, "NewMiner should succeed")
require.NotNil(t, m, "NewMiner should return a non-nil miner")

// Step 3: a distinct event creates a separate new template.
resultDistinct, reportDistinct, errDistinct := m.AnalyzeEvent(evtFinish)
require.NoError(t, errDistinct, "AnalyzeEvent should not fail for distinct event")
require.NotNil(t, resultDistinct, "AnalyzeEvent should return a non-nil result")
require.NotNil(t, reportDistinct, "AnalyzeEvent should return a non-nil report")
assert.True(t, reportDistinct.IsNewTemplate, "IsNewTemplate mismatch for distinct event")
assert.InDelta(t, 0.65, reportDistinct.AnomalyScore, 1e-9, "AnomalyScore mismatch for distinct event")
assert.Equal(t, "new log template discovered; rare cluster (few observations)", reportDistinct.Reason, "Reason mismatch for distinct event")
result, report, err := m.AnalyzeEvent(AgentEvent{Stage: "plan"})

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The masking test uses AgentEvent{Stage: "plan"} with no Fields. Because FlattenEvent emits "stage=plan" even without fields, the ".+" mask rule erases it, so the test correctly triggers the empty-after-masking path. However, this is non-obvious — a reader might wonder why no fields are needed.

💡 Suggestion

Add a brief inline comment to make the intent explicit:

// No Fields needed: the mask rule ".+" erases the stage token too,
// leaving an empty string after masking.
result, report, err := m.AnalyzeEvent(AgentEvent{Stage: "plan"})

Also consider adding a second case where Fields is set to show that masking applies to the full flattened string, not just the fields portion.

@copilot please address this.

require.Error(t, err, "AnalyzeEvent should reject events that become empty after masking")

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestAnalyzeEvent_EmptyAfterMasking may silently pass for the wrong reason. The regex .+ does NOT match an empty string or a string of only whitespace, but FlattenEvent always prepends stage=plan — so the test works today. However, the test has no assertion that the input to Analyze is actually empty; it relies entirely on the error message string.

💡 Robustness concern

FlattenEvent for AgentEvent{Stage: "plan"} produces "stage=plan". The mask rule Pattern: ".+" erases that to "", so Tokenize("") returns nil, and the empty-tokens guard fires correctly.

This is working now, but the test is fragile in two ways:

  1. If FlattenEvent ever omits stage= from the output (e.g., Stage becomes an excluded field), the flattened event would be empty before masking, and AnalyzeEvent would still return the same error — but via a different code path. The test would still pass, masking a behavioral regression.

  2. The regex .+ does not match empty string or whitespace-only strings. An event that flattens to only whitespace (possible if field values are empty) would produce tokens even with this mask rule, and the test would not cover that edge case.

Consider using a mask rule that is explicitly designed to test the empty-after-masking path, and add a comment explaining why this rule is chosen:

// Replace every non-whitespace character, leaving only spaces which Tokenize strips.
cfg.MaskRules = []MaskRule{{
    Name:        "eraseAllNonWhitespace",
    Pattern:     `\S+`,
    Replacement: "",
}}

Or alternatively, verify that the test input actually reaches the masking step by inspecting intermediate state, or adding a second case with a whitespace-only flattened event.

require.ErrorContains(t, err, "empty event after masking", "AnalyzeEvent should report the masking failure")
assert.Nil(t, result, "AnalyzeEvent should not return a result on masking failure")
assert.Nil(t, report, "AnalyzeEvent should not return a report on masking failure")
}

func TestAnalyze_FlagMutualExclusivity(t *testing.T) {
Expand Down
Loading