From 94e4889ff10cfedf5f4ea4cbc3fe7af2e087bc49 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 2 Oct 2026 10:00:16 +0200 Subject: [PATCH 1/2] test: improve Go test idioms across the codebase Add missing test files for untested source files (clusterinfo.go, route.go, output.go, collector.go), add t.Run subtests to all table-driven tests that were missing them for better failure diagnostics, consolidate duplicate test helpers into a shared testhelper_test.go with functional options, unify the two nearly identical factory helpers in kube/client_test.go, and consolidate 12 individual test functions in namespace/filter_test.go into 4 table-driven tests. Assisted-by: Claude --- internal/cli/root_test.go | 25 +-- internal/collector/clusterinfo_test.go | 98 ++++++++++ internal/collector/collector_test.go | 176 ++++++++++++++++++ internal/collector/heapdump_test.go | 40 ++-- internal/collector/helm_test.go | 27 +-- internal/collector/namespace_inspect_test.go | 21 ++- internal/collector/operator_test.go | 31 ++-- internal/collector/output_test.go | 185 +++++++++++++++++++ internal/collector/platform_test.go | 94 ++-------- internal/collector/route_test.go | 141 ++++++++++++++ internal/collector/testhelper_test.go | 96 ++++++++++ internal/collector/workload_test.go | 17 +- internal/kube/client_test.go | 124 ++++++------- internal/namespace/filter_test.go | 141 +++++++------- 14 files changed, 930 insertions(+), 286 deletions(-) create mode 100644 internal/collector/clusterinfo_test.go create mode 100644 internal/collector/collector_test.go create mode 100644 internal/collector/output_test.go create mode 100644 internal/collector/route_test.go create mode 100644 internal/collector/testhelper_test.go diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index b30d3749..fbeb7fe1 100644 --- a/internal/cli/root_test.go +++ b/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) + } + }) } } diff --git a/internal/collector/clusterinfo_test.go b/internal/collector/clusterinfo_test.go new file mode 100644 index 00000000..4ae84ae8 --- /dev/null +++ b/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") + } +} diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go new file mode 100644 index 00000000..60f59758 --- /dev/null +++ b/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)) + } +} diff --git a/internal/collector/heapdump_test.go b/internal/collector/heapdump_test.go index 1c147a7e..ebdac452 100644 --- a/internal/collector/heapdump_test.go +++ b/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) + } + }) } } diff --git a/internal/collector/helm_test.go b/internal/collector/helm_test.go index 931b6133..dad0435a 100644 --- a/internal/collector/helm_test.go +++ b/internal/collector/helm_test.go @@ -211,25 +211,26 @@ func TestSelectPrimaryDeployment(t *testing.T) { func TestIsSecretDocument(t *testing.T) { tests := []struct { + name string yaml string want bool }{ - {"kind: Secret\napiVersion: v1\nmetadata:\n name: s", true}, - {"kind: ConfigMap\napiVersion: v1\nmetadata:\n name: c", false}, - {"kind: Deployment\napiVersion: apps/v1\nmetadata:\n name: d", false}, + {"secret", "kind: Secret\napiVersion: v1\nmetadata:\n name: s", true}, + {"configmap", "kind: ConfigMap\napiVersion: v1\nmetadata:\n name: c", false}, + {"deployment", "kind: Deployment\napiVersion: apps/v1\nmetadata:\n name: d", false}, } for _, tt := range tests { - // We need to simulate what the YAML decoder produces - // Testing the filterSecretsFromYAML function indirectly instead - result := filterSecretsFromYAML(tt.yaml) - hasContent := strings.TrimSpace(result) != "" - if tt.want && hasContent { - t.Errorf("expected Secret to be filtered from: %s", tt.yaml) - } - if !tt.want && !hasContent { - t.Errorf("expected non-Secret to be preserved: %s", tt.yaml) - } + t.Run(tt.name, func(t *testing.T) { + result := filterSecretsFromYAML(tt.yaml) + hasContent := strings.TrimSpace(result) != "" + if tt.want && hasContent { + t.Errorf("expected Secret to be filtered from: %s", tt.yaml) + } + if !tt.want && !hasContent { + t.Errorf("expected non-Secret to be preserved: %s", tt.yaml) + } + }) } } diff --git a/internal/collector/namespace_inspect_test.go b/internal/collector/namespace_inspect_test.go index 724a2a21..2e93b95f 100644 --- a/internal/collector/namespace_inspect_test.go +++ b/internal/collector/namespace_inspect_test.go @@ -17,20 +17,23 @@ func TestMatchesAnyPattern(t *testing.T) { patterns := []string{"backstage", "rhdh", "developer-hub"} tests := []struct { + name string value string want bool }{ - {"backstage-chart-1.0", true}, - {"RHDH-Helm", true}, - {"developer-hub-app", true}, - {"postgres", false}, - {"", false}, - {"Backstage", true}, + {"backstage prefix", "backstage-chart-1.0", true}, + {"rhdh case insensitive", "RHDH-Helm", true}, + {"developer-hub prefix", "developer-hub-app", true}, + {"unrelated", "postgres", false}, + {"empty string", "", false}, + {"capitalized", "Backstage", true}, } for _, tt := range tests { - if got := matchesAnyPattern(tt.value, patterns); got != tt.want { - t.Errorf("matchesAnyPattern(%q) = %v, want %v", tt.value, got, tt.want) - } + t.Run(tt.name, func(t *testing.T) { + if got := matchesAnyPattern(tt.value, patterns); got != tt.want { + t.Errorf("matchesAnyPattern(%q) = %v, want %v", tt.value, got, tt.want) + } + }) } } diff --git a/internal/collector/operator_test.go b/internal/collector/operator_test.go index 6349f242..c7fd82e3 100644 --- a/internal/collector/operator_test.go +++ b/internal/collector/operator_test.go @@ -84,24 +84,27 @@ func TestOwnedOKPDeployments_IAOnly(t *testing.T) { func TestIsRHDHRelated(t *testing.T) { tests := []struct { - name string - want bool + label string + name string + want bool }{ - {"rhdh-operator.v1.5.0", true}, - {"backstage-operator.v1.0.0", true}, - {"developer-hub-operator.v1.0.0", true}, - {"RHDH-Operator", true}, - {"my-Backstage-app", true}, - {"some-other-operator", false}, - {"cert-manager.v1.0.0", false}, - {"", false}, + {"rhdh operator", "rhdh-operator.v1.5.0", true}, + {"backstage operator", "backstage-operator.v1.0.0", true}, + {"developer hub operator", "developer-hub-operator.v1.0.0", true}, + {"case insensitive", "RHDH-Operator", true}, + {"backstage in name", "my-Backstage-app", true}, + {"unrelated operator", "some-other-operator", false}, + {"cert manager", "cert-manager.v1.0.0", false}, + {"empty string", "", false}, } for _, tt := range tests { - got := isRHDHRelated(tt.name) - if got != tt.want { - t.Errorf("isRHDHRelated(%q) = %v, want %v", tt.name, got, tt.want) - } + t.Run(tt.label, func(t *testing.T) { + got := isRHDHRelated(tt.name) + if got != tt.want { + t.Errorf("isRHDHRelated(%q) = %v, want %v", tt.name, got, tt.want) + } + }) } } diff --git a/internal/collector/output_test.go b/internal/collector/output_test.go new file mode 100644 index 00000000..1c056513 --- /dev/null +++ b/internal/collector/output_test.go @@ -0,0 +1,185 @@ +package collector + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestSetGVK(t *testing.T) { + pod := &corev1.Pod{} + setGVK(pod, "Pod", "v1") + + gvk := pod.GetObjectKind().GroupVersionKind() + if gvk.Kind != "Pod" { + t.Errorf("Kind = %q, want Pod", gvk.Kind) + } + if gvk.Version != "v1" { + t.Errorf("Version = %q, want v1", gvk.Version) + } +} + +func TestWriteResource(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "nested", "resource.yaml") + + data := map[string]string{"name": "test"} + writeResource(path, data) + + content, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading file: %v", err) + } + if !strings.Contains(string(content), "name: test") { + t.Errorf("content = %q, want 'name: test'", string(content)) + } +} + +func TestWriteCollectError(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "sub", "error.txt") + + writeCollectError(path, "list pods", os.ErrPermission) + + content, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading file: %v", err) + } + s := string(content) + if !strings.Contains(s, "list pods") { + t.Error("expected description in output") + } + if !strings.Contains(s, "permission denied") { + t.Error("expected error message in output") + } +} + +func TestKnownGroupKinds(t *testing.T) { + tests := []struct { + input string + kind string + group string + }{ + {"pod", "Pod", ""}, + {"pods", "Pod", ""}, + {"deployment", "Deployment", "apps"}, + {"deployments", "Deployment", "apps"}, + {"statefulset", "StatefulSet", "apps"}, + {"replicaset", "ReplicaSet", "apps"}, + {"configmap", "ConfigMap", ""}, + {"configmaps", "ConfigMap", ""}, + {"crd", "CustomResourceDefinition", "apiextensions.k8s.io"}, + {"controllerrevision", "ControllerRevision", "apps"}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + gk, ok := knownGroupKinds[tt.input] + if !ok { + t.Fatalf("knownGroupKinds missing %q", tt.input) + } + if gk.Kind != tt.kind { + t.Errorf("Kind = %q, want %q", gk.Kind, tt.kind) + } + if gk.Group != tt.group { + t.Errorf("Group = %q, want %q", gk.Group, tt.group) + } + }) + } +} + +func TestListResourceNames(t *testing.T) { + pod1 := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod-a", Namespace: "ns1"}} + pod2 := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod-b", Namespace: "ns1"}} + pod3 := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod-c", Namespace: "ns2"}} + dep1 := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "dep-a", Namespace: "ns1"}} + + cfg := newTestConfig(t, "", withTypedObjs(pod1, pod2, pod3, dep1)) + + t.Run("pods in namespace", func(t *testing.T) { + names, err := listResourceNames(context.Background(), cfg, + schema.GroupKind{Kind: "Pod"}, "ns1", "") + if err != nil { + t.Fatal(err) + } + if len(names) != 2 { + t.Fatalf("got %d names, want 2", len(names)) + } + }) + + t.Run("deployments in namespace", func(t *testing.T) { + names, err := listResourceNames(context.Background(), cfg, + schema.GroupKind{Group: "apps", Kind: "Deployment"}, "ns1", "") + if err != nil { + t.Fatal(err) + } + if len(names) != 1 || names[0] != "dep-a" { + t.Errorf("got %v, want [dep-a]", names) + } + }) + + t.Run("empty namespace", func(t *testing.T) { + names, err := listResourceNames(context.Background(), cfg, + schema.GroupKind{Kind: "Pod"}, "ns-empty", "") + if err != nil { + t.Fatal(err) + } + if len(names) != 0 { + t.Errorf("got %d names, want 0", len(names)) + } + }) + + t.Run("unknown kind", func(t *testing.T) { + _, err := listResourceNames(context.Background(), cfg, + schema.GroupKind{Kind: "Unknown"}, "ns1", "") + if err == nil { + t.Error("expected error for unknown kind") + } + }) +} + +func TestResolveCRDType(t *testing.T) { + cfg := newTestConfig(t, "", + withAPIGroups("rhdh.redhat.com/v1alpha3", "sonataflow.org/v1alpha08"), + ) + + t.Run("fully qualified", func(t *testing.T) { + gvr, err := resolveCRDType(cfg, "sonataflow.sonataflow.org") + if err != nil { + t.Fatal(err) + } + if gvr.Group != "sonataflow.org" { + t.Errorf("Group = %q, want sonataflow.org", gvr.Group) + } + if gvr.Resource != "sonataflows" { + t.Errorf("Resource = %q, want sonataflows", gvr.Resource) + } + }) + + t.Run("short name backstage", func(t *testing.T) { + gvr, err := resolveCRDType(cfg, "backstage") + if err != nil { + t.Fatal(err) + } + if gvr.Group != "rhdh.redhat.com" { + t.Errorf("Group = %q, want rhdh.redhat.com", gvr.Group) + } + if gvr.Resource != "backstages" { + t.Errorf("Resource = %q, want backstages", gvr.Resource) + } + }) + + t.Run("unknown short name", func(t *testing.T) { + _, err := resolveCRDType(cfg, "unknown") + if err == nil { + t.Error("expected error for unknown CRD type") + } + }) +} diff --git a/internal/collector/platform_test.go b/internal/collector/platform_test.go index c419f64a..842724dd 100644 --- a/internal/collector/platform_test.go +++ b/internal/collector/platform_test.go @@ -5,27 +5,17 @@ import ( "encoding/json" "os" "path/filepath" - "sync/atomic" "testing" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - fakediscovery "k8s.io/client-go/discovery/fake" - fakedynamic "k8s.io/client-go/dynamic/fake" - fakeclientset "k8s.io/client-go/kubernetes/fake" - - "github.com/redhat-developer/rhdh-must-gather/internal/kube" ) func TestPlatform_VanillaK8s(t *testing.T) { dir := t.TempDir() cfg := newTestConfig(t, dir, - []string{"apps/v1"}, - []*corev1.Node{testNode("node1", "", nil)}, - nil, + withAPIGroups("apps/v1"), + withTypedObjs(testNode("node1", "", nil)), ) p := &Platform{} @@ -42,11 +32,10 @@ func TestPlatform_VanillaK8s(t *testing.T) { func TestPlatform_EKS(t *testing.T) { dir := t.TempDir() cfg := newTestConfig(t, dir, - []string{"apps/v1"}, - []*corev1.Node{testNode("node1", "aws://us-east-1/i-123", map[string]string{ + withAPIGroups("apps/v1"), + withTypedObjs(testNode("node1", "aws://us-east-1/i-123", map[string]string{ "eks.amazonaws.com/nodegroup": "my-nodegroup", - })}, - nil, + })), ) p := &Platform{} @@ -66,11 +55,10 @@ func TestPlatform_EKS(t *testing.T) { func TestPlatform_GKE(t *testing.T) { dir := t.TempDir() cfg := newTestConfig(t, dir, - []string{"apps/v1"}, - []*corev1.Node{testNode("node1", "gce://project/zone/instance", map[string]string{ + withAPIGroups("apps/v1"), + withTypedObjs(testNode("node1", "gce://project/zone/instance", map[string]string{ "cloud.google.com/gke-nodepool": "default-pool", - })}, - nil, + })), ) p := &Platform{} @@ -115,9 +103,14 @@ func TestPlatform_OCP(t *testing.T) { } cfg := newTestConfig(t, dir, - []string{"config.openshift.io/v1"}, - nil, - []runtime.Object{cv, infra}, + withAPIGroups("config.openshift.io/v1"), + withDynamicObjs( + map[schema.GroupVersionResource]string{ + clusterVersionGVR: "ClusterVersionList", + infrastructureGVR: "InfrastructureList", + }, + cv, infra, + ), ) p := &Platform{} @@ -143,9 +136,8 @@ func TestPlatform_OCP(t *testing.T) { func TestPlatform_OutputFiles(t *testing.T) { dir := t.TempDir() cfg := newTestConfig(t, dir, - []string{"apps/v1"}, - []*corev1.Node{testNode("node1", "", nil)}, - nil, + withAPIGroups("apps/v1"), + withTypedObjs(testNode("node1", "", nil)), ) p := &Platform{} @@ -193,53 +185,3 @@ func readPlatformJSON(t *testing.T, dir string) platformInfo { } return info } - -func newTestConfig(t *testing.T, basePath string, apiGroupVersions []string, nodes []*corev1.Node, dynamicObjs []runtime.Object) *Config { - t.Helper() - - var typedObjs []runtime.Object - for _, n := range nodes { - typedObjs = append(typedObjs, n) - } - fakeClient := fakeclientset.NewSimpleClientset(typedObjs...) - - fd := fakeClient.Discovery().(*fakediscovery.FakeDiscovery) - resources := make([]*metav1.APIResourceList, len(apiGroupVersions)) - for i, gv := range apiGroupVersions { - resources[i] = &metav1.APIResourceList{GroupVersion: gv} - } - fd.Resources = resources - - scheme := runtime.NewScheme() - dynClient := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(scheme, - map[schema.GroupVersionResource]string{ - clusterVersionGVR: "ClusterVersionList", - infrastructureGVR: "InfrastructureList", - }, - dynamicObjs...) - - return &Config{ - BasePath: basePath, - Interrupted: new(atomic.Bool), - Client: &kube.Client{ - Clientset: fakeClient, - Discovery: fd, - Dynamic: dynClient, - }, - } -} - -func testNode(name, providerID string, labels map[string]string) *corev1.Node { - if labels == nil { - labels = map[string]string{} - } - return &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Labels: labels, - }, - Spec: corev1.NodeSpec{ - ProviderID: providerID, - }, - } -} diff --git a/internal/collector/route_test.go b/internal/collector/route_test.go new file mode 100644 index 00000000..07254fd2 --- /dev/null +++ b/internal/collector/route_test.go @@ -0,0 +1,141 @@ +package collector + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestRoute_Name(t *testing.T) { + r := &Route{} + if got := r.Name(); got != "route" { + t.Errorf("Name() = %q, want %q", got, "route") + } +} + +func TestGetString(t *testing.T) { + tests := []struct { + name string + obj map[string]any + fields []string + want string + }{ + { + name: "nested value", + obj: map[string]any{"metadata": map[string]any{"name": "my-route"}}, + fields: []string{"metadata", "name"}, + want: "my-route", + }, + { + name: "missing field", + obj: map[string]any{"metadata": map[string]any{}}, + fields: []string{"metadata", "name"}, + want: "", + }, + { + name: "missing path", + obj: map[string]any{}, + fields: []string{"spec", "host"}, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u := unstructured.Unstructured{Object: tt.obj} + if got := getString(u, tt.fields...); got != tt.want { + t.Errorf("getString() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestRoute_Run_NoRouteAPI(t *testing.T) { + dir := t.TempDir() + cfg := newTestConfig(t, dir, withAPIGroups("apps/v1")) + + r := &Route{} + if err := r.Run(context.Background(), cfg); err != nil { + t.Fatal(err) + } + + data, err := os.ReadFile(filepath.Join(dir, "all-routes.txt")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "not an OpenShift cluster") { + t.Error("expected message about Route API not available") + } +} + +func TestRoute_Run_WithRoutes(t *testing.T) { + dir := t.TempDir() + + route := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "route.openshift.io/v1", + "kind": "Route", + "metadata": map[string]any{ + "name": "backstage", + "namespace": "rhdh", + }, + "spec": map[string]any{ + "host": "backstage.apps.example.com", + }, + }, + } + + cfg := newTestConfig(t, dir, + withAPIGroups("route.openshift.io/v1"), + withDynamicObjs( + map[schema.GroupVersionResource]string{routeGVR: "RouteList"}, + route, + ), + ) + + r := &Route{} + if err := r.Run(context.Background(), cfg); err != nil { + t.Fatal(err) + } + + data, err := os.ReadFile(filepath.Join(dir, "all-routes.txt")) + if err != nil { + t.Fatal(err) + } + content := string(data) + if !strings.Contains(content, "backstage") { + t.Error("expected route name in output") + } + if !strings.Contains(content, "backstage.apps.example.com") { + t.Error("expected route host in output") + } +} + +func TestRoute_Run_NoRoutes(t *testing.T) { + dir := t.TempDir() + + cfg := newTestConfig(t, dir, + withAPIGroups("route.openshift.io/v1"), + withDynamicObjs( + map[schema.GroupVersionResource]string{routeGVR: "RouteList"}, + ), + ) + + r := &Route{} + if err := r.Run(context.Background(), cfg); err != nil { + t.Fatal(err) + } + + data, err := os.ReadFile(filepath.Join(dir, "all-routes.txt")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "No resources found") { + t.Error("expected 'No resources found' for empty route list") + } +} diff --git a/internal/collector/testhelper_test.go b/internal/collector/testhelper_test.go new file mode 100644 index 00000000..9f19aec3 --- /dev/null +++ b/internal/collector/testhelper_test.go @@ -0,0 +1,96 @@ +package collector + +import ( + "sync/atomic" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + fakediscovery "k8s.io/client-go/discovery/fake" + fakedynamic "k8s.io/client-go/dynamic/fake" + fakeclientset "k8s.io/client-go/kubernetes/fake" + + "github.com/redhat-developer/rhdh-must-gather/internal/kube" +) + +func newTestConfig(t *testing.T, basePath string, opts ...testConfigOption) *Config { + t.Helper() + + o := &testConfigOpts{} + for _, fn := range opts { + fn(o) + } + + fakeClient := fakeclientset.NewSimpleClientset(o.typedObjs...) + + fd := fakeClient.Discovery().(*fakediscovery.FakeDiscovery) + resources := make([]*metav1.APIResourceList, len(o.apiGroupVersions)) + for i, gv := range o.apiGroupVersions { + resources[i] = &metav1.APIResourceList{GroupVersion: gv} + } + fd.Resources = resources + + scheme := runtime.NewScheme() + dynClient := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(scheme, o.dynamicListKinds, o.dynamicObjs...) + + interrupted := new(atomic.Bool) + if o.interrupted { + interrupted.Store(true) + } + + return &Config{ + BasePath: basePath, + Interrupted: interrupted, + Client: &kube.Client{ + Clientset: fakeClient, + Discovery: fd, + Dynamic: dynClient, + }, + } +} + +type testConfigOpts struct { + apiGroupVersions []string + typedObjs []runtime.Object + dynamicObjs []runtime.Object + dynamicListKinds map[schema.GroupVersionResource]string + interrupted bool +} + +type testConfigOption func(*testConfigOpts) + +func withAPIGroups(groups ...string) testConfigOption { + return func(o *testConfigOpts) { o.apiGroupVersions = groups } +} + +func withTypedObjs(objs ...runtime.Object) testConfigOption { + return func(o *testConfigOpts) { o.typedObjs = objs } +} + +func withDynamicObjs(listKinds map[schema.GroupVersionResource]string, objs ...runtime.Object) testConfigOption { + return func(o *testConfigOpts) { + o.dynamicListKinds = listKinds + o.dynamicObjs = objs + } +} + +func withInterrupted() testConfigOption { + return func(o *testConfigOpts) { o.interrupted = true } +} + +func testNode(name, providerID string, labels map[string]string) *corev1.Node { + if labels == nil { + labels = map[string]string{} + } + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: labels, + }, + Spec: corev1.NodeSpec{ + ProviderID: providerID, + }, + } +} diff --git a/internal/collector/workload_test.go b/internal/collector/workload_test.go index 580d4531..3f2f42ec 100644 --- a/internal/collector/workload_test.go +++ b/internal/collector/workload_test.go @@ -79,17 +79,20 @@ func TestFilterPodsByOwner_Empty(t *testing.T) { func TestOwnerRefKind(t *testing.T) { tests := []struct { + name string kind WorkloadKind want string }{ - {KindDeployment, "ReplicaSet"}, - {KindStatefulSet, "StatefulSet"}, - {"unknown", ""}, + {"deployment", KindDeployment, "ReplicaSet"}, + {"statefulset", KindStatefulSet, "StatefulSet"}, + {"unknown", "unknown", ""}, } for _, tt := range tests { - got := ownerRefKind(tt.kind) - if got != tt.want { - t.Errorf("ownerRefKind(%q) = %q, want %q", tt.kind, got, tt.want) - } + t.Run(tt.name, func(t *testing.T) { + got := ownerRefKind(tt.kind) + if got != tt.want { + t.Errorf("ownerRefKind(%q) = %q, want %q", tt.kind, got, tt.want) + } + }) } } diff --git a/internal/kube/client_test.go b/internal/kube/client_test.go index 52a99cfd..5b42d484 100644 --- a/internal/kube/client_test.go +++ b/internal/kube/client_test.go @@ -8,15 +8,13 @@ import ( fakeclientset "k8s.io/client-go/kubernetes/fake" ) -func newTestClient(groups ...string) *Client { +func newTestClient(groupVersions ...string) *Client { fakeClient := fakeclientset.NewSimpleClientset() fakeDiscovery := fakeClient.Discovery().(*fake.FakeDiscovery) - resources := make([]*metav1.APIResourceList, len(groups)) - for i, g := range groups { - resources[i] = &metav1.APIResourceList{ - GroupVersion: g + "/v1", - } + resources := make([]*metav1.APIResourceList, len(groupVersions)) + for i, gv := range groupVersions { + resources[i] = &metav1.APIResourceList{GroupVersion: gv} } fakeDiscovery.Resources = resources @@ -26,70 +24,64 @@ func newTestClient(groups ...string) *Client { } } -func TestHasAPIGroup_Found(t *testing.T) { - c := newTestClient("route.openshift.io", "apps", "config.openshift.io") - ok, err := c.HasAPIGroup("route.openshift.io") - if err != nil { - t.Fatal(err) - } - if !ok { - t.Error("expected route.openshift.io to be found") +func TestHasAPIGroup(t *testing.T) { + tests := []struct { + name string + groups []string + query string + want bool + }{ + { + name: "found", + groups: []string{"route.openshift.io/v1", "apps/v1", "config.openshift.io/v1"}, + query: "route.openshift.io", + want: true, + }, + { + name: "not found", + groups: []string{"apps/v1"}, + query: "route.openshift.io", + want: false, + }, + { + name: "empty groups", + groups: nil, + query: "anything", + want: false, + }, } -} -func TestHasAPIGroup_NotFound(t *testing.T) { - c := newTestClient("apps") - ok, err := c.HasAPIGroup("route.openshift.io") - if err != nil { - t.Fatal(err) - } - if ok { - t.Error("expected route.openshift.io not to be found") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := newTestClient(tt.groups...) + got, err := c.HasAPIGroup(tt.query) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Errorf("HasAPIGroup(%q) = %v, want %v", tt.query, got, tt.want) + } + }) } } -func TestHasAPIGroup_Empty(t *testing.T) { - c := newTestClient() - ok, err := c.HasAPIGroup("anything") - if err != nil { - t.Fatal(err) - } - if ok { - t.Error("expected no API groups to be found") - } -} - -func TestPreferredVersion_Found(t *testing.T) { - c := newTestClientWithVersions("rhdh.redhat.com/v1alpha3", "operators.coreos.com/v1alpha1") - ver, err := c.PreferredVersion("rhdh.redhat.com") - if err != nil { - t.Fatal(err) - } - if ver != "v1alpha3" { - t.Errorf("version = %q, want v1alpha3", ver) - } -} - -func TestPreferredVersion_NotFound(t *testing.T) { - c := newTestClient("apps") - _, err := c.PreferredVersion("rhdh.redhat.com") - if err == nil { - t.Error("expected error for missing group") - } -} - -func newTestClientWithVersions(groupVersions ...string) *Client { - fakeClient := fakeclientset.NewSimpleClientset() - fakeDiscovery := fakeClient.Discovery().(*fake.FakeDiscovery) - - resources := make([]*metav1.APIResourceList, len(groupVersions)) - for i, gv := range groupVersions { - resources[i] = &metav1.APIResourceList{GroupVersion: gv} - } - fakeDiscovery.Resources = resources +func TestPreferredVersion(t *testing.T) { + t.Run("found", func(t *testing.T) { + c := newTestClient("rhdh.redhat.com/v1alpha3", "operators.coreos.com/v1alpha1") + ver, err := c.PreferredVersion("rhdh.redhat.com") + if err != nil { + t.Fatal(err) + } + if ver != "v1alpha3" { + t.Errorf("version = %q, want v1alpha3", ver) + } + }) - return &Client{ - Clientset: fakeClient, - Discovery: fakeDiscovery, - } + t.Run("not found", func(t *testing.T) { + c := newTestClient("apps/v1") + _, err := c.PreferredVersion("rhdh.redhat.com") + if err == nil { + t.Error("expected error for missing group") + } + }) } diff --git a/internal/namespace/filter_test.go b/internal/namespace/filter_test.go index 6a7a73b7..2d9db93c 100644 --- a/internal/namespace/filter_test.go +++ b/internal/namespace/filter_test.go @@ -1,98 +1,93 @@ package namespace import ( + "reflect" "testing" ) -func TestParseNamespaces_Empty(t *testing.T) { - if ns := ParseNamespaces(""); ns != nil { - t.Errorf("got %v, want nil", ns) +func TestParseNamespaces(t *testing.T) { + tests := []struct { + name string + input string + want []string + }{ + {"empty", "", nil}, + {"single", "rhdh-prod", []string{"rhdh-prod"}}, + {"multiple with spaces", "ns1, ns2 ,ns3", []string{"ns1", "ns2", "ns3"}}, + {"whitespace only", " , , ", nil}, } -} -func TestParseNamespaces_Single(t *testing.T) { - ns := ParseNamespaces("rhdh-prod") - if len(ns) != 1 || ns[0] != "rhdh-prod" { - t.Errorf("got %v, want [rhdh-prod]", ns) - } -} - -func TestParseNamespaces_Multiple(t *testing.T) { - ns := ParseNamespaces("ns1, ns2 ,ns3") - expected := []string{"ns1", "ns2", "ns3"} - if len(ns) != len(expected) { - t.Fatalf("got %v, want %v", ns, expected) - } - for i, want := range expected { - if ns[i] != want { - t.Errorf("ns[%d] = %q, want %q", i, ns[i], want) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ParseNamespaces(tt.input) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("ParseNamespaces(%q) = %v, want %v", tt.input, got, tt.want) + } + }) } } -func TestParseNamespaces_WhitespaceOnly(t *testing.T) { - if ns := ParseNamespaces(" , , "); ns != nil { - t.Errorf("got %v, want nil", ns) +func TestIncludes(t *testing.T) { + tests := []struct { + name string + targets []string + ns string + want bool + }{ + {"no filter includes all", nil, "any-ns", true}, + {"match first", []string{"ns1", "ns2"}, "ns1", true}, + {"match second", []string{"ns1", "ns2"}, "ns2", true}, + {"no match", []string{"ns1", "ns2"}, "ns3", false}, } -} -func TestIncludes_NoFilter(t *testing.T) { - if !Includes(nil, "any-ns") { - t.Error("should include all namespaces when no filter set") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := Includes(tt.targets, tt.ns); got != tt.want { + t.Errorf("Includes(%v, %q) = %v, want %v", tt.targets, tt.ns, got, tt.want) + } + }) } } -func TestIncludes_Match(t *testing.T) { - targets := []string{"ns1", "ns2"} - if !Includes(targets, "ns1") { - t.Error("ns1 should be included") - } - if !Includes(targets, "ns2") { - t.Error("ns2 should be included") +func TestTargetNamespaces(t *testing.T) { + tests := []struct { + name string + env string + want []string + }{ + {"empty", "", nil}, + {"single", "rhdh-prod", []string{"rhdh-prod"}}, } -} -func TestIncludes_NoMatch(t *testing.T) { - targets := []string{"ns1", "ns2"} - if Includes(targets, "ns3") { - t.Error("ns3 should not be included") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("RHDH_TARGET_NAMESPACES", tt.env) + got := TargetNamespaces() + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("TargetNamespaces() = %v, want %v", got, tt.want) + } + }) } } -// Tests for the env-based wrappers. - -func TestTargetNamespaces_Empty(t *testing.T) { - t.Setenv("RHDH_TARGET_NAMESPACES", "") - if ns := TargetNamespaces(); ns != nil { - t.Errorf("got %v, want nil", ns) +func TestShouldInclude(t *testing.T) { + tests := []struct { + name string + env string + ns string + want bool + }{ + {"no filter", "", "any-ns", true}, + {"match", "ns1,ns2", "ns1", true}, + {"no match", "ns1,ns2", "ns3", false}, } -} - -func TestTargetNamespaces_Single(t *testing.T) { - t.Setenv("RHDH_TARGET_NAMESPACES", "rhdh-prod") - ns := TargetNamespaces() - if len(ns) != 1 || ns[0] != "rhdh-prod" { - t.Errorf("got %v, want [rhdh-prod]", ns) - } -} - -func TestShouldInclude_NoFilter(t *testing.T) { - t.Setenv("RHDH_TARGET_NAMESPACES", "") - if !ShouldInclude("any-ns") { - t.Error("should include all namespaces when no filter set") - } -} - -func TestShouldInclude_Match(t *testing.T) { - t.Setenv("RHDH_TARGET_NAMESPACES", "ns1,ns2") - if !ShouldInclude("ns1") { - t.Error("ns1 should be included") - } -} -func TestShouldInclude_NoMatch(t *testing.T) { - t.Setenv("RHDH_TARGET_NAMESPACES", "ns1,ns2") - if ShouldInclude("ns3") { - t.Error("ns3 should not be included") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("RHDH_TARGET_NAMESPACES", tt.env) + if got := ShouldInclude(tt.ns); got != tt.want { + t.Errorf("ShouldInclude(%q) = %v, want %v", tt.ns, got, tt.want) + } + }) } } From 068456f27a9e3aeeb20eb1553e7056a88f15dfb9 Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Fri, 2 Oct 2026 10:55:14 +0200 Subject: [PATCH 2/2] test(output): use served CRD version and assert resolved version Use v1alpha5 (the currently served Backstage CRD version) instead of v1alpha3 in TestResolveCRDType fixtures, and assert the resolved GVR version field in both subtests to verify the full return value. Assisted-by: Claude --- internal/collector/output_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/collector/output_test.go b/internal/collector/output_test.go index 1c056513..e814635a 100644 --- a/internal/collector/output_test.go +++ b/internal/collector/output_test.go @@ -147,7 +147,7 @@ func TestListResourceNames(t *testing.T) { func TestResolveCRDType(t *testing.T) { cfg := newTestConfig(t, "", - withAPIGroups("rhdh.redhat.com/v1alpha3", "sonataflow.org/v1alpha08"), + withAPIGroups("rhdh.redhat.com/v1alpha5", "sonataflow.org/v1alpha08"), ) t.Run("fully qualified", func(t *testing.T) { @@ -158,6 +158,9 @@ func TestResolveCRDType(t *testing.T) { if gvr.Group != "sonataflow.org" { t.Errorf("Group = %q, want sonataflow.org", gvr.Group) } + if gvr.Version != "v1alpha08" { + t.Errorf("Version = %q, want v1alpha08", gvr.Version) + } if gvr.Resource != "sonataflows" { t.Errorf("Resource = %q, want sonataflows", gvr.Resource) } @@ -171,6 +174,9 @@ func TestResolveCRDType(t *testing.T) { if gvr.Group != "rhdh.redhat.com" { t.Errorf("Group = %q, want rhdh.redhat.com", gvr.Group) } + if gvr.Version != "v1alpha5" { + t.Errorf("Version = %q, want v1alpha5", gvr.Version) + } if gvr.Resource != "backstages" { t.Errorf("Resource = %q, want backstages", gvr.Resource) }