From f0f282adf87bb9fccbcc4547c831bf73a4e5fd41 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Mon, 1 Dec 2025 20:44:00 +0000 Subject: [PATCH] test: Add tests for parseContainerMountMetadata, convertContainerDiskInfo, StalenessScore - parseContainerMountMetadata: parts without equals, mountpoint key variant, whitespace handling, single source value (96.4% -> 100%) - convertContainerDiskInfo: nil metadata, device from metadata, negative free clamping, whitespace label handling (95.1% -> 97.6%, remaining is dead code) - StalenessScore: negative maxStale default, metrics lookup failure, score clamping edge cases (95.7% - remaining is defensive dead code) --- internal/monitoring/container_parsing_test.go | 228 ++++++++++++++++++ internal/monitoring/staleness_tracker_test.go | 135 +++++++++++ 2 files changed, 363 insertions(+) diff --git a/internal/monitoring/container_parsing_test.go b/internal/monitoring/container_parsing_test.go index 68f5c799f..9b3e84d62 100644 --- a/internal/monitoring/container_parsing_test.go +++ b/internal/monitoring/container_parsing_test.go @@ -619,6 +619,84 @@ func TestParseContainerMountMetadataEdgeCases(t *testing.T) { }, }, }, + { + name: "part without equals sign skipped", + input: map[string]interface{}{ + "mp0": "local:volume,readonly,mp=/data,backup", + }, + want: map[string]containerMountMetadata{ + "mp0": { + Key: "mp0", + Mountpoint: "/data", + Source: "local:volume", + }, + }, + }, + { + name: "rootfs without mountpoint defaults to slash", + input: map[string]interface{}{ + "rootfs": "local:100/vm-100-disk-0.raw,size=8G", + }, + want: map[string]containerMountMetadata{ + "rootfs": { + Key: "rootfs", + Mountpoint: "/", + Source: "local:100/vm-100-disk-0.raw", + }, + }, + }, + { + name: "non-rootfs without mountpoint has empty mountpoint", + input: map[string]interface{}{ + "mp1": "local:volume,size=10G", + }, + want: map[string]containerMountMetadata{ + "mp1": { + Key: "mp1", + Mountpoint: "", + Source: "local:volume", + }, + }, + }, + { + name: "key case insensitive", + input: map[string]interface{}{ + "ROOTFS": "local:disk,size=8G", + "MP0": "local:vol,mp=/mnt", + }, + want: map[string]containerMountMetadata{ + "rootfs": { + Key: "rootfs", + Mountpoint: "/", + Source: "local:disk", + }, + "mp0": { + Key: "mp0", + Mountpoint: "/mnt", + Source: "local:vol", + }, + }, + }, + { + name: "whitespace-only value treated as empty", + input: map[string]interface{}{ + "mp0": " ", + }, + want: nil, + }, + { + name: "single source value no comma parts", + input: map[string]interface{}{ + "rootfs": "local:disk", + }, + want: map[string]containerMountMetadata{ + "rootfs": { + Key: "rootfs", + Mountpoint: "/", + Source: "local:disk", + }, + }, + }, } for _, tt := range tests { @@ -1145,6 +1223,156 @@ func TestConvertContainerDiskInfo(t *testing.T) { }, }, }, + { + name: "nil metadata does not panic", + status: &proxmox.Container{ + DiskInfo: map[string]proxmox.ContainerDiskUsage{ + "rootfs": { + Total: 1000, + Used: 500, + }, + }, + RootFS: "local:disk", + }, + metadata: nil, + want: []models.Disk{ + { + Total: 1000, + Used: 500, + Free: 500, + Usage: 50.0, + Mountpoint: "/", + Type: "rootfs", + Device: "local:disk", + }, + }, + }, + { + name: "disk gets device from metadata when not set", + status: &proxmox.Container{ + DiskInfo: map[string]proxmox.ContainerDiskUsage{ + "mp0": { + Total: 1000, + Used: 500, + }, + }, + }, + metadata: map[string]containerMountMetadata{ + "mp0": { + Mountpoint: "/data", + Source: "nfs:shared-volume", + }, + }, + want: []models.Disk{ + { + Total: 1000, + Used: 500, + Free: 500, + Usage: 50.0, + Mountpoint: "/data", + Type: "mp0", + Device: "nfs:shared-volume", + }, + }, + }, + { + name: "negative free clamped to zero", + status: &proxmox.Container{ + DiskInfo: map[string]proxmox.ContainerDiskUsage{ + "rootfs": { + Total: 0, + Used: 500, // used > total=0, free = -500 clamped to 0 + }, + }, + }, + want: []models.Disk{ + { + Total: 0, + Used: 500, // Not clamped because total == 0 + Free: 0, // Clamped from -500 to 0 + Usage: 0, // No calculation when total == 0 + Mountpoint: "/", + Type: "rootfs", + }, + }, + }, + { + name: "whitespace label trimmed and treated as rootfs", + status: &proxmox.Container{ + DiskInfo: map[string]proxmox.ContainerDiskUsage{ + " ": { // whitespace only, trims to empty + Total: 1000, + Used: 500, + }, + }, + }, + want: []models.Disk{ + { + Total: 1000, + Used: 500, + Free: 500, + Usage: 50.0, + Mountpoint: "/", + Type: "rootfs", + }, + }, + }, + { + name: "rootfs gets device from RootFS when metadata has empty source", + status: &proxmox.Container{ + DiskInfo: map[string]proxmox.ContainerDiskUsage{ + "rootfs": { + Total: 1000, + Used: 500, + }, + }, + RootFS: "local:100/disk.raw,size=8G", + }, + metadata: map[string]containerMountMetadata{ + "rootfs": { + Mountpoint: "/", + Source: "", // empty source + }, + }, + want: []models.Disk{ + { + Total: 1000, + Used: 500, + Free: 500, + Usage: 50.0, + Mountpoint: "/", + Type: "rootfs", + Device: "local:100/disk.raw", // Falls back to RootFS, sanitized + }, + }, + }, + { + name: "non-rootfs with whitespace label gets type disk", + status: &proxmox.Container{ + DiskInfo: map[string]proxmox.ContainerDiskUsage{ + "mp0": { + Total: 1000, + Used: 500, + }, + }, + }, + metadata: map[string]containerMountMetadata{ + "mp0": { + Mountpoint: "/mnt/storage", + Source: "", + }, + }, + want: []models.Disk{ + { + Total: 1000, + Used: 500, + Free: 500, + Usage: 50.0, + Mountpoint: "/mnt/storage", + Type: "mp0", + }, + }, + }, } for _, tt := range tests { diff --git a/internal/monitoring/staleness_tracker_test.go b/internal/monitoring/staleness_tracker_test.go index f37a384cc..217584a9e 100644 --- a/internal/monitoring/staleness_tracker_test.go +++ b/internal/monitoring/staleness_tracker_test.go @@ -472,6 +472,141 @@ func TestStalenessTracker_StalenessScore_MetricsNotUsedWhenLastSuccessZero(t *te } } +func TestStalenessTracker_StalenessScore_NegativeMaxStaleUsesDefault(t *testing.T) { + // Create tracker and directly set maxStale to negative to test the defensive fallback + tracker := &StalenessTracker{ + entries: make(map[string]FreshnessSnapshot), + maxStale: -5 * time.Minute, // Force negative to test default fallback + } + + // Set a 2.5 minute old success + oldTime := time.Now().Add(-150 * time.Second) + tracker.setSnapshot(FreshnessSnapshot{ + InstanceType: InstanceTypePVE, + Instance: "neg-max-test", + LastSuccess: oldTime, + }) + + score, ok := tracker.StalenessScore(InstanceTypePVE, "neg-max-test") + if !ok { + t.Fatal("staleness score should be available") + } + + // With default 5 minute maxStale, 2.5 minutes should give ~0.5 score + expected := 150.0 / 300.0 // 150s / 300s (5 min) + tolerance := 0.05 + if score < expected-tolerance || score > expected+tolerance { + t.Errorf("staleness score = %f, want ~%f (using default 5 min maxStale)", score, expected) + } +} + +func TestStalenessTracker_StalenessScore_MetricsLookupFails(t *testing.T) { + // Create a minimal PollMetrics with lastSuccessByKey support + pm := &PollMetrics{ + lastSuccessByKey: make(map[metricKey]time.Time), + } + + tracker := NewStalenessTracker(pm) + tracker.SetBounds(10*time.Second, 60*time.Second) + + // Set snapshot with non-zero LastSuccess + oldTime := time.Now().Add(-30 * time.Second) + tracker.setSnapshot(FreshnessSnapshot{ + InstanceType: InstanceTypePVE, + Instance: "lookup-fail-test", + LastSuccess: oldTime, + }) + + // Don't store anything in metrics - the lookup will fail + + score, ok := tracker.StalenessScore(InstanceTypePVE, "lookup-fail-test") + if !ok { + t.Fatal("staleness score should be available") + } + + // Should use the tracker's LastSuccess since metrics lookup failed: 30s / 60s = 0.5 + expected := 30.0 / 60.0 + tolerance := 0.05 + if score < expected-tolerance || score > expected+tolerance { + t.Errorf("staleness score = %f, want ~%f (using tracker time, metrics lookup failed)", score, expected) + } +} + +func TestStalenessTracker_StalenessScore_ScoreClampedBetweenZeroAndOne(t *testing.T) { + // Test that score is always in [0, 1] range + tests := []struct { + name string + age time.Duration + maxStale time.Duration + wantScore float64 + wantCapped bool + }{ + { + name: "age much older than maxStale is capped at 1", + age: 10 * time.Minute, + maxStale: 1 * time.Minute, + wantScore: 1.0, + wantCapped: true, + }, + { + name: "age at exactly maxStale gives 1", + age: 5 * time.Minute, + maxStale: 5 * time.Minute, + wantScore: 1.0, + wantCapped: false, + }, + { + name: "age at half maxStale gives 0.5", + age: 2*time.Minute + 30*time.Second, + maxStale: 5 * time.Minute, + wantScore: 0.5, + wantCapped: false, + }, + { + name: "very small age gives near-zero score", + age: 1 * time.Millisecond, + maxStale: 5 * time.Minute, + wantScore: 0.0, + wantCapped: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tracker := NewStalenessTracker(nil) + tracker.SetBounds(10*time.Second, tt.maxStale) + + tracker.setSnapshot(FreshnessSnapshot{ + InstanceType: InstanceTypePVE, + Instance: "clamp-test", + LastSuccess: time.Now().Add(-tt.age), + }) + + score, ok := tracker.StalenessScore(InstanceTypePVE, "clamp-test") + if !ok { + t.Fatal("staleness score should be available") + } + + // Check score is in [0, 1] range + if score < 0 || score > 1 { + t.Errorf("score = %f, want score in [0, 1] range", score) + } + + tolerance := 0.05 + if score < tt.wantScore-tolerance || score > tt.wantScore+tolerance { + t.Errorf("score = %f, want ~%f", score, tt.wantScore) + } + }) + } +} + +// Note on coverage: The `if score < 0 { score = 0 }` branch (line 145) is mathematically +// unreachable because: +// 1. If age <= 0, we return early with score=0 (line 133-135) +// 2. If max <= 0, we default to 5 minutes making max positive (line 138-140) +// 3. Therefore score = age.Seconds() / max.Seconds() is always non-negative +// This is defensive code that guards against future refactoring mistakes. + func TestTrackerKey(t *testing.T) { tests := []struct { instanceType InstanceType