chore: clean up tests

Author: mchmarny

janitor/api/v1alpha1/gpureset_types.go

@@ -32,6 +32,7 @@ type GPUSelector struct {
 	// PCIBusIDs is a list of GPU PCI bus IDs.
 	// Format: "domain:bus:device.function" (e.g., "0000:01:00.0").
 	// +optional
+	//nolint:lll // kubebuilder validation pattern
 	// +kubebuilder:validation:items:Pattern="^[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}\\.[0-9a-fA-F]{1}$"
 	PCIBusIDs []string `json:"pciBusIDs,omitempty"`
 }

janitor/api/v1alpha1/rebootnode_types_test.go

@@ -222,18 +222,45 @@ func TestRebootNode_SetCompletionTime(t *testing.T) {
 }
 
 func TestRebootNode_StatusFields(t *testing.T) {
-	t.Run("retry count and consecutive failures", func(t *testing.T) {
+	t.Run("status fields are initialized to zero values", func(t *testing.T) {
 		rn := &RebootNode{}
 
-		// Initial values
+		// Verify default zero values for status fields
 		assert.Equal(t, int32(0), rn.Status.RetryCount)
 		assert.Equal(t, int32(0), rn.Status.ConsecutiveFailures)
+		assert.Nil(t, rn.Status.StartTime)
+		assert.Nil(t, rn.Status.CompletionTime)
+		assert.Empty(t, rn.Status.Conditions)
+	})
+
+	t.Run("status fields can be set and retrieved independently", func(t *testing.T) {
+		rn := &RebootNode{}
+
+		// Set start time
+		rn.SetStartTime()
+		require.NotNil(t, rn.Status.StartTime)
 
-		// Modify values
-		rn.Status.RetryCount = 5
-		rn.Status.ConsecutiveFailures = 3
+		// Increment retry count
+		rn.Status.RetryCount++
+		assert.Equal(t, int32(1), rn.Status.RetryCount)
+
+		// Increment consecutive failures
+		rn.Status.ConsecutiveFailures++
+		assert.Equal(t, int32(1), rn.Status.ConsecutiveFailures)
+
+		// Set conditions
+		rn.SetInitialConditions()
+		assert.NotEmpty(t, rn.Status.Conditions)
+
+		// Set completion time
+		rn.SetCompletionTime()
+		require.NotNil(t, rn.Status.CompletionTime)
 
-		assert.Equal(t, int32(5), rn.Status.RetryCount)
-		assert.Equal(t, int32(3), rn.Status.ConsecutiveFailures)
+		// Verify all fields are set correctly
+		assert.Equal(t, int32(1), rn.Status.RetryCount)
+		assert.Equal(t, int32(1), rn.Status.ConsecutiveFailures)
+		assert.NotNil(t, rn.Status.StartTime)
+		assert.NotNil(t, rn.Status.CompletionTime)
+		assert.Len(t, rn.Status.Conditions, 2) // SignalSent and NodeReady
 	})
 }

janitor/pkg/config/config_test.go

@@ -199,46 +199,54 @@ global:
 	assert.Equal(t, metav1.LabelSelectorOpExists, config.RebootNode.NodeExclusions[1].MatchExpressions[0].Operator)
 }
 
-func TestConfig_Structures(t *testing.T) {
-	// Test that all config structures can be instantiated
-	config := &Config{
-		Global: GlobalConfig{
-			Timeout:    10 * time.Minute,
-			ManualMode: true,
-			Nodes: NodeConfig{
-				Exclusions: []metav1.LabelSelector{
-					{
-						MatchLabels: map[string]string{"test": "value"},
-					},
-				},
+func TestConfig_ZeroValues(t *testing.T) {
+	t.Run("zero value config has expected defaults", func(t *testing.T) {
+		config := &Config{}
+
+		// Global config zero values
+		assert.Equal(t, time.Duration(0), config.Global.Timeout)
+		assert.False(t, config.Global.ManualMode)
+		assert.Empty(t, config.Global.Nodes.Exclusions)
+
+		// RebootNode config zero values
+		assert.False(t, config.RebootNode.Enabled)
+		assert.False(t, config.RebootNode.ManualMode)
+		assert.Equal(t, time.Duration(0), config.RebootNode.Timeout)
+		assert.Empty(t, config.RebootNode.NodeExclusions)
+
+		// TerminateNode config zero values
+		assert.False(t, config.TerminateNode.Enabled)
+		assert.False(t, config.TerminateNode.ManualMode)
+		assert.Equal(t, time.Duration(0), config.TerminateNode.Timeout)
+		assert.Empty(t, config.TerminateNode.NodeExclusions)
+	})
+
+	t.Run("controller configs are independent", func(t *testing.T) {
+		config := &Config{
+			RebootNode: RebootNodeControllerConfig{
+				Enabled:    true,
+				ManualMode: false,
+				Timeout:    5 * time.Minute,
 			},
-		},
-		RebootNode: RebootNodeControllerConfig{
-			Enabled:    true,
-			ManualMode: false,
-			Timeout:    5 * time.Minute,
-			NodeExclusions: []metav1.LabelSelector{
-				{
-					MatchLabels: map[string]string{"test": "value"},
-				},
+			TerminateNode: TerminateNodeControllerConfig{
+				Enabled:    false,
+				ManualMode: true,
+				Timeout:    10 * time.Minute,
 			},
-		},
-		TerminateNode: TerminateNodeControllerConfig{
-			Enabled:    false,
-			ManualMode: true,
-			Timeout:    3 * time.Minute,
-			NodeExclusions: []metav1.LabelSelector{
-				{
-					MatchLabels: map[string]string{"test": "value"},
-				},
-			},
-		},
-	}
-
-	// Verify basic structure
-	assert.NotNil(t, config)
-	assert.Equal(t, 10*time.Minute, config.Global.Timeout)
-	assert.True(t, config.Global.ManualMode)
-	assert.True(t, config.RebootNode.Enabled)
-	assert.False(t, config.TerminateNode.Enabled)
+		}
+
+		// Verify each controller can have different settings
+		assert.True(t, config.RebootNode.Enabled)
+		assert.False(t, config.RebootNode.ManualMode)
+		assert.Equal(t, 5*time.Minute, config.RebootNode.Timeout)
+
+		assert.False(t, config.TerminateNode.Enabled)
+		assert.True(t, config.TerminateNode.ManualMode)
+		assert.Equal(t, 10*time.Minute, config.TerminateNode.Timeout)
+
+		// Modifying one doesn't affect the other
+		config.RebootNode.Timeout = 15 * time.Minute
+		assert.Equal(t, 15*time.Minute, config.RebootNode.Timeout)
+		assert.Equal(t, 10*time.Minute, config.TerminateNode.Timeout)
+	})
 }

janitor/pkg/metrics/metrics_test.go

@@ -18,10 +18,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/prometheus/client_golang/prometheus"
-	dto "github.com/prometheus/client_model/go"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestNewActionMetrics(t *testing.T) {
@@ -134,90 +131,62 @@ func TestMetricsConstants(t *testing.T) {
 }
 
 func TestActionMetrics_CounterIncrement(t *testing.T) {
-	// Create a test registry to verify metrics behavior
-	testRegistry := prometheus.NewRegistry()
-
-	// Create new counter for testing
-	testCounter := prometheus.NewCounterVec(
-		prometheus.CounterOpts{
-			Name: "test_janitor_actions_count",
-			Help: "Test counter for janitor actions",
-		},
-		[]string{"action_type", "status", "node"},
-	)
-
-	testRegistry.MustRegister(testCounter)
-
-	// Increment the counter multiple times
-	testCounter.With(prometheus.Labels{
-		"action_type": ActionTypeReboot,
-		"status":      StatusStarted,
-		"node":        "test-node",
-	}).Inc()
-
-	testCounter.With(prometheus.Labels{
-		"action_type": ActionTypeReboot,
-		"status":      StatusStarted,
-		"node":        "test-node",
-	}).Inc()
-
-	// Gather metrics
-	metricFamilies, err := testRegistry.Gather()
-	require.NoError(t, err)
-	require.Len(t, metricFamilies, 1)
-
-	// Verify the counter was incremented
-	metricFamily := metricFamilies[0]
-	assert.Equal(t, "test_janitor_actions_count", *metricFamily.Name)
-	require.Len(t, metricFamily.Metric, 1)
-
-	// Check that the counter value is 2
-	counter := metricFamily.Metric[0]
-	assert.Equal(t, float64(2), *counter.Counter.Value)
-}
-
-func TestActionMetrics_HistogramObservation(t *testing.T) {
-	// Create a test registry to verify histogram behavior
-	testRegistry := prometheus.NewRegistry()
-
-	// Create new histogram for testing
-	testHistogram := prometheus.NewHistogramVec(
-		prometheus.HistogramOpts{
-			Name:    "test_janitor_action_mttr_seconds",
-			Help:    "Test histogram for janitor action MTTR",
-			Buckets: prometheus.ExponentialBuckets(10, 2, 5),
-		},
-		[]string{"action_type"},
-	)
-
-	testRegistry.MustRegister(testHistogram)
+	t.Run("IncActionCount increments counter", func(t *testing.T) {
+		// Create metrics instance
+		m := &ActionMetrics{}
+
+		// Call the actual business logic
+		m.IncActionCount(ActionTypeReboot, StatusStarted, "test-node-1")
+		m.IncActionCount(ActionTypeReboot, StatusSucceeded, "test-node-1")
+		m.IncActionCount(ActionTypeTerminate, StatusStarted, "test-node-2")
+
+		// Note: We can't easily verify the exact counter values without accessing
+		// the internal prometheus registry, but we can verify the method executes
+		// without panic or error. This test verifies the method signature and basic
+		// functionality.
+	})
 
-	// Record some observations
-	testHistogram.With(prometheus.Labels{
-		"action_type": ActionTypeReboot,
-	}).Observe(30.0) // 30 seconds
+	t.Run("global IncActionCount function works", func(t *testing.T) {
+		// Test the convenience function that uses GlobalMetrics
+		IncActionCount(ActionTypeReboot, StatusStarted, "global-test-node")
+		IncActionCount(ActionTypeReboot, StatusSucceeded, "global-test-node")
+		IncActionCount(ActionTypeReboot, StatusFailed, "global-test-node")
 
-	testHistogram.With(prometheus.Labels{
-		"action_type": ActionTypeReboot,
-	}).Observe(120.0) // 2 minutes
+		// Verify different action types
+		IncActionCount(ActionTypeTerminate, StatusStarted, "global-test-node-2")
+	})
+}
 
-	// Gather metrics
-	metricFamilies, err := testRegistry.Gather()
-	require.NoError(t, err)
-	require.Len(t, metricFamilies, 1)
+func TestActionMetrics_HistogramObservation(t *testing.T) {
+	t.Run("RecordActionMTTR records duration", func(t *testing.T) {
+		// Create metrics instance
+		m := &ActionMetrics{}
+
+		// Call the actual business logic with various durations
+		m.RecordActionMTTR(ActionTypeReboot, 30*time.Second)
+		m.RecordActionMTTR(ActionTypeReboot, 2*time.Minute)
+		m.RecordActionMTTR(ActionTypeReboot, 5*time.Minute)
+
+		// Record different action types
+		m.RecordActionMTTR(ActionTypeTerminate, 10*time.Minute)
+
+		// Note: We can't easily verify the exact histogram values without accessing
+		// the internal prometheus registry, but we can verify the method executes
+		// without panic or error. This test verifies the method signature and basic
+		// functionality.
+	})
 
-	// Verify the histogram was updated
-	metricFamily := metricFamilies[0]
-	assert.Equal(t, "test_janitor_action_mttr_seconds", *metricFamily.Name)
-	assert.Equal(t, dto.MetricType_HISTOGRAM, *metricFamily.Type)
-	require.Len(t, metricFamily.Metric, 1)
+	t.Run("global RecordActionMTTR function works", func(t *testing.T) {
+		// Test the convenience function that uses GlobalMetrics
+		RecordActionMTTR(ActionTypeReboot, 45*time.Second)
+		RecordActionMTTR(ActionTypeTerminate, 3*time.Minute)
 
-	// Check histogram sample count
-	histogram := metricFamily.Metric[0].Histogram
-	assert.Equal(t, uint64(2), *histogram.SampleCount)
+		// Test with very short duration
+		RecordActionMTTR(ActionTypeReboot, 5*time.Second)
 
-	// Check sum of observations (30 + 120 = 150)
-	assert.Equal(t, float64(150), *histogram.SampleSum)
+		// Test with longer duration
+		RecordActionMTTR(ActionTypeTerminate, 15*time.Minute)
+	})
 }
 
 func TestGlobalMetrics_Initialization(t *testing.T) {