test: improve Go test idioms across the codebase [RHIDP-14400]

internal/cli/root_test.go

@@ -84,23 +84,26 @@ func TestBuildScriptList_ExcludeAll(t *testing.T) {
 
 func TestHeapDumpMethodValidation(t *testing.T) {
 	tests := []struct {
+		name    string
 		method  string
 		wantErr bool
 	}{
-		{"inspector", false},
-		{"sigusr2", false},
-		{"invalid", true},
-		{"", true},
+		{"inspector", "inspector", false},
+		{"sigusr2", "sigusr2", false},
+		{"invalid", "invalid", true},
+		{"empty", "", true},
 	}
 
 	for _, tt := range tests {
-		cmd := newRootCmd()
-		cmd.RunE = func(cmd *cobra.Command, args []string) error { return nil }
-		cmd.SetArgs([]string{"--heap-dump-method", tt.method})
-		err := cmd.Execute()
-		if (err != nil) != tt.wantErr {
-			t.Errorf("method=%q: got err=%v, wantErr=%v", tt.method, err, tt.wantErr)
-		}
+		t.Run(tt.name, func(t *testing.T) {
+			cmd := newRootCmd()
+			cmd.RunE = func(cmd *cobra.Command, args []string) error { return nil }
+			cmd.SetArgs([]string{"--heap-dump-method", tt.method})
+			err := cmd.Execute()
+			if (err != nil) != tt.wantErr {
+				t.Errorf("method=%q: got err=%v, wantErr=%v", tt.method, err, tt.wantErr)
+			}
+		})
 	}
 }
 

internal/collector/clusterinfo_test.go

@@ -0,0 +1,98 @@
+package collector
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestClusterInfo_Name(t *testing.T) {
+	c := &ClusterInfo{}
+	if got := c.Name(); got != "cluster-info" {
+		t.Errorf("Name() = %q, want %q", got, "cluster-info")
+	}
+}
+
+func TestWriteYAML(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "sub", "test.yaml")
+
+	data := map[string]string{"key": "value"}
+	writeYAML(path, data)
+
+	content, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("reading file: %v", err)
+	}
+	if len(content) == 0 {
+		t.Fatal("expected non-empty output")
+	}
+}
+
+func TestWriteYAML_CreatesParentDirs(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "a", "b", "c", "test.yaml")
+
+	writeYAML(path, "hello")
+
+	if _, err := os.Stat(path); err != nil {
+		t.Errorf("expected file to exist: %v", err)
+	}
+}
+
+func TestClusterInfo_Run(t *testing.T) {
+	dir := t.TempDir()
+
+	ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-ns"}}
+	pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test-pod", Namespace: "test-ns"}}
+
+	cfg := newTestConfig(t, dir, withTypedObjs(ns, pod))
+
+	c := &ClusterInfo{}
+	if err := c.Run(context.Background(), cfg); err != nil {
+		t.Fatal(err)
+	}
+
+	clusterDir := filepath.Join(dir, "cluster-info")
+	if _, err := os.Stat(clusterDir); err != nil {
+		t.Fatal("cluster-info directory not created")
+	}
+
+	nsDir := filepath.Join(clusterDir, "test-ns")
+	if _, err := os.Stat(filepath.Join(nsDir, "pods.yaml")); err != nil {
+		t.Error("pods.yaml not created for namespace")
+	}
+	if _, err := os.Stat(filepath.Join(nsDir, "events.yaml")); err != nil {
+		t.Error("events.yaml not created for namespace")
+	}
+}
+
+func TestClusterInfo_Run_Interrupted(t *testing.T) {
+	dir := t.TempDir()
+
+	ns1 := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns1"}}
+	ns2 := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns2"}}
+
+	cfg := newTestConfig(t, dir, withTypedObjs(ns1, ns2), withInterrupted())
+
+	c := &ClusterInfo{}
+	if err := c.Run(context.Background(), cfg); err != nil {
+		t.Fatal(err)
+	}
+
+	clusterDir := filepath.Join(dir, "cluster-info")
+	entries, _ := os.ReadDir(clusterDir)
+	nsCount := 0
+	for _, e := range entries {
+		if e.IsDir() {
+			nsCount++
+		}
+	}
+	if nsCount > 0 {
+		t.Error("expected no namespace directories when interrupted before processing")
+	}
+}

internal/collector/collector_test.go

@@ -0,0 +1,176 @@
+package collector
+
+import (
+	"sync/atomic"
+	"testing"
+	"time"
+
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestConfig_IsInterrupted(t *testing.T) {
+	tests := []struct {
+		name string
+		cfg  *Config
+		want bool
+	}{
+		{
+			name: "nil interrupted",
+			cfg:  &Config{},
+			want: false,
+		},
+		{
+			name: "not interrupted",
+			cfg:  &Config{Interrupted: new(atomic.Bool)},
+			want: false,
+		},
+		{
+			name: "interrupted",
+			cfg: func() *Config {
+				b := new(atomic.Bool)
+				b.Store(true)
+				return &Config{Interrupted: b}
+			}(),
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.cfg.IsInterrupted(); got != tt.want {
+				t.Errorf("IsInterrupted() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestConfig_Namespaces(t *testing.T) {
+	t.Run("nil", func(t *testing.T) {
+		cfg := &Config{}
+		if got := cfg.Namespaces(); got != nil {
+			t.Errorf("Namespaces() = %v, want nil", got)
+		}
+	})
+
+	t.Run("populated", func(t *testing.T) {
+		cfg := &Config{TargetNamespaces: []string{"ns1", "ns2"}}
+		got := cfg.Namespaces()
+		if len(got) != 2 || got[0] != "ns1" || got[1] != "ns2" {
+			t.Errorf("Namespaces() = %v, want [ns1 ns2]", got)
+		}
+	})
+}
+
+func TestConfig_ShouldInclude(t *testing.T) {
+	tests := []struct {
+		name       string
+		namespaces []string
+		ns         string
+		want       bool
+	}{
+		{
+			name:       "no filter includes all",
+			namespaces: nil,
+			ns:         "anything",
+			want:       true,
+		},
+		{
+			name:       "match",
+			namespaces: []string{"ns1", "ns2"},
+			ns:         "ns2",
+			want:       true,
+		},
+		{
+			name:       "no match",
+			namespaces: []string{"ns1", "ns2"},
+			ns:         "ns3",
+			want:       false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := &Config{TargetNamespaces: tt.namespaces}
+			if got := cfg.ShouldInclude(tt.ns); got != tt.want {
+				t.Errorf("ShouldInclude(%q) = %v, want %v", tt.ns, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestConfig_ApplyLogSince(t *testing.T) {
+	t.Run("since duration", func(t *testing.T) {
+		cfg := &Config{Since: 5 * time.Minute}
+		opts := &corev1.PodLogOptions{}
+		cfg.ApplyLogSince(opts)
+
+		if opts.SinceSeconds == nil {
+			t.Fatal("expected SinceSeconds to be set")
+		}
+		if *opts.SinceSeconds != 300 {
+			t.Errorf("SinceSeconds = %d, want 300", *opts.SinceSeconds)
+		}
+		if opts.SinceTime != nil {
+			t.Error("expected SinceTime to be nil")
+		}
+	})
+
+	t.Run("since time", func(t *testing.T) {
+		cfg := &Config{SinceTime: "2024-01-15T10:00:00Z"}
+		opts := &corev1.PodLogOptions{}
+		cfg.ApplyLogSince(opts)
+
+		if opts.SinceTime == nil {
+			t.Fatal("expected SinceTime to be set")
+		}
+		if opts.SinceTime.Year() != 2024 || opts.SinceTime.Month() != 1 {
+			t.Errorf("SinceTime = %v, want 2024-01-15", opts.SinceTime)
+		}
+	})
+
+	t.Run("neither set", func(t *testing.T) {
+		cfg := &Config{}
+		opts := &corev1.PodLogOptions{}
+		cfg.ApplyLogSince(opts)
+
+		if opts.SinceSeconds != nil {
+			t.Error("expected SinceSeconds to be nil")
+		}
+		if opts.SinceTime != nil {
+			t.Error("expected SinceTime to be nil")
+		}
+	})
+
+	t.Run("invalid since time", func(t *testing.T) {
+		cfg := &Config{SinceTime: "not-a-time"}
+		opts := &corev1.PodLogOptions{}
+		cfg.ApplyLogSince(opts)
+
+		if opts.SinceTime != nil {
+			t.Error("expected SinceTime to be nil for invalid input")
+		}
+	})
+}
+
+func TestRegistry(t *testing.T) {
+	expected := []string{
+		"platform", "route", "ingress", "cluster-info",
+		"operator", "orchestrator", "helm", "namespace-inspect",
+	}
+
+	for _, name := range expected {
+		t.Run(name, func(t *testing.T) {
+			c, ok := Registry[name]
+			if !ok {
+				t.Fatalf("Registry missing collector %q", name)
+			}
+			if c.Name() != name {
+				t.Errorf("Name() = %q, want %q", c.Name(), name)
+			}
+		})
+	}
+
+	if len(Registry) != len(expected) {
+		t.Errorf("Registry has %d collectors, want %d", len(Registry), len(expected))
+	}
+}

internal/collector/heapdump_test.go

@@ -6,20 +6,23 @@ import (
 
 func TestMatchesInstance(t *testing.T) {
 	tests := []struct {
+		label   string
 		name    string
 		pattern string
 		want    bool
 	}{
-		{"rhdhsupp-308-backstage", "rhdhsupp-308", true},
-		{"rhdhsupp-308", "rhdhsupp-308", true},
-		{"my-backstage", "rhdhsupp-308", false},
-		{"", "rhdhsupp-308", false},
-		{"rhdhsupp-308-backstage", "", false},
+		{"prefix match", "rhdhsupp-308-backstage", "rhdhsupp-308", true},
+		{"exact match", "rhdhsupp-308", "rhdhsupp-308", true},
+		{"no match", "my-backstage", "rhdhsupp-308", false},
+		{"empty name", "", "rhdhsupp-308", false},
+		{"empty pattern", "rhdhsupp-308-backstage", "", false},
 	}
 	for _, tt := range tests {
-		if got := matchesInstance(tt.name, tt.pattern); got != tt.want {
-			t.Errorf("matchesInstance(%q, %q) = %v, want %v", tt.name, tt.pattern, got, tt.want)
-		}
+		t.Run(tt.label, func(t *testing.T) {
+			if got := matchesInstance(tt.name, tt.pattern); got != tt.want {
+				t.Errorf("matchesInstance(%q, %q) = %v, want %v", tt.name, tt.pattern, got, tt.want)
+			}
+		})
 	}
 }
 
@@ -75,19 +78,22 @@ func TestHeapDumpTimeout(t *testing.T) {
 
 func TestHumanSize(t *testing.T) {
 	tests := []struct {
+		name  string
 		bytes int64
 		want  string
 	}{
-		{0, "0B"},
-		{512, "512B"},
-		{1024, "1KB"},
-		{1536, "1KB"},
-		{1048576, "1MB"},
-		{104857600, "100MB"},
+		{"zero", 0, "0B"},
+		{"bytes", 512, "512B"},
+		{"1KB", 1024, "1KB"},
+		{"rounds down", 1536, "1KB"},
+		{"1MB", 1048576, "1MB"},
+		{"100MB", 104857600, "100MB"},
 	}
 	for _, tt := range tests {
-		if got := humanSize(tt.bytes); got != tt.want {
-			t.Errorf("humanSize(%d) = %q, want %q", tt.bytes, got, tt.want)
-		}
+		t.Run(tt.name, func(t *testing.T) {
+			if got := humanSize(tt.bytes); got != tt.want {
+				t.Errorf("humanSize(%d) = %q, want %q", tt.bytes, got, tt.want)
+			}
+		})
 	}
 }