Remove redundant timeout tests and improve test coverage for instance timeout validation

pkg/instance/instance_test.go

@@ -91,38 +91,6 @@ func TestNewInstance_WithRestartOptions(t *testing.T) {
 	}
 }
 
-func TestNewInstance_ValidationAndDefaults(t *testing.T) {
-	globalSettings := &config.InstancesConfig{
-		LogsDir:             "/tmp/test",
-		DefaultAutoRestart:  true,
-		DefaultMaxRestarts:  3,
-		DefaultRestartDelay: 5,
-	}
-
-	// Test with invalid negative values
-	invalidMaxRestarts := -5
-	invalidRestartDelay := -10
-
-	options := &instance.CreateInstanceOptions{
-		MaxRestarts:  &invalidMaxRestarts,
-		RestartDelay: &invalidRestartDelay,
-		LlamaServerOptions: llamacpp.LlamaServerOptions{
-			Model: "/path/to/model.gguf",
-		},
-	}
-
-	instance := instance.NewInstance("test-instance", globalSettings, options)
-	opts := instance.GetOptions()
-
-	// Check that negative values were corrected to 0
-	if opts.MaxRestarts == nil || *opts.MaxRestarts != 0 {
-		t.Errorf("Expected MaxRestarts to be corrected to 0, got %v", opts.MaxRestarts)
-	}
-	if opts.RestartDelay == nil || *opts.RestartDelay != 0 {
-		t.Errorf("Expected RestartDelay to be corrected to 0, got %v", opts.RestartDelay)
-	}
-}
-
 func TestSetOptions(t *testing.T) {
 	globalSettings := &config.InstancesConfig{
 		LogsDir:             "/tmp/test",
@@ -164,33 +132,6 @@ func TestSetOptions(t *testing.T) {
 	}
 }
 
-func TestSetOptions_NilOptions(t *testing.T) {
-	globalSettings := &config.InstancesConfig{
-		LogsDir:             "/tmp/test",
-		DefaultAutoRestart:  true,
-		DefaultMaxRestarts:  3,
-		DefaultRestartDelay: 5,
-	}
-
-	options := &instance.CreateInstanceOptions{
-		LlamaServerOptions: llamacpp.LlamaServerOptions{
-			Model: "/path/to/model.gguf",
-		},
-	}
-
-	instance := instance.NewInstance("test-instance", globalSettings, options)
-	originalOptions := instance.GetOptions()
-
-	// Try to set nil options
-	instance.SetOptions(nil)
-
-	// Options should remain unchanged
-	currentOptions := instance.GetOptions()
-	if currentOptions.Model != originalOptions.Model {
-		t.Error("Options should not change when setting nil options")
-	}
-}
-
 func TestGetProxy(t *testing.T) {
 	globalSettings := &config.InstancesConfig{
 		LogsDir: "/tmp/test",
@@ -317,58 +258,6 @@ func TestUnmarshalJSON(t *testing.T) {
 	}
 }
 
-func TestUnmarshalJSON_PartialOptions(t *testing.T) {
-	jsonData := `{
-		"name": "test-instance",
-		"running": false,
-		"options": {
-			"model": "/path/to/model.gguf"
-		}
-	}`
-
-	var inst instance.Process
-	err := json.Unmarshal([]byte(jsonData), &inst)
-	if err != nil {
-		t.Fatalf("JSON unmarshal failed: %v", err)
-	}
-
-	opts := inst.GetOptions()
-	if opts.Model != "/path/to/model.gguf" {
-		t.Errorf("Expected model '/path/to/model.gguf', got %q", opts.Model)
-	}
-
-	// Note: Defaults are NOT applied during unmarshaling
-	// They should only be applied by NewInstance or SetOptions
-	if opts.AutoRestart != nil {
-		t.Error("Expected AutoRestart to be nil (no defaults applied during unmarshal)")
-	}
-}
-
-func TestUnmarshalJSON_NoOptions(t *testing.T) {
-	jsonData := `{
-		"name": "test-instance",
-		"running": false
-	}`
-
-	var inst instance.Process
-	err := json.Unmarshal([]byte(jsonData), &inst)
-	if err != nil {
-		t.Fatalf("JSON unmarshal failed: %v", err)
-	}
-
-	if inst.Name != "test-instance" {
-		t.Errorf("Expected name 'test-instance', got %q", inst.Name)
-	}
-	if inst.Running {
-		t.Error("Expected running to be false")
-	}
-
-	opts := inst.GetOptions()
-	if opts != nil {
-		t.Error("Expected options to be nil when not provided in JSON")
-	}
-}
-
 func TestCreateInstanceOptionsValidation(t *testing.T) {
 	tests := []struct {
 		name          string
@@ -377,13 +266,6 @@ func TestCreateInstanceOptionsValidation(t *testing.T) {
 		expectedMax   int
 		expectedDelay int
 	}{
-		{
-			name:          "nil values",
-			maxRestarts:   nil,
-			restartDelay:  nil,
-			expectedMax:   0, // Should remain nil, but we can't easily test nil in this structure
-			expectedDelay: 0,
-		},
 		{
 			name:          "valid positive values",
 			maxRestarts:   testutil.IntPtr(10),
@@ -424,20 +306,16 @@ func TestCreateInstanceOptionsValidation(t *testing.T) {
 			instance := instance.NewInstance("test", globalSettings, options)
 			opts := instance.GetOptions()
 
-			if tt.maxRestarts != nil {
+			if opts.MaxRestarts == nil {
-				if opts.MaxRestarts == nil {
+				t.Error("Expected MaxRestarts to be set")
-					t.Error("Expected MaxRestarts to be set")
+			} else if *opts.MaxRestarts != tt.expectedMax {
-				} else if *opts.MaxRestarts != tt.expectedMax {
+				t.Errorf("Expected MaxRestarts %d, got %d", tt.expectedMax, *opts.MaxRestarts)
-					t.Errorf("Expected MaxRestarts %d, got %d", tt.expectedMax, *opts.MaxRestarts)
-				}
 			}
 
-			if tt.restartDelay != nil {
+			if opts.RestartDelay == nil {
-				if opts.RestartDelay == nil {
+				t.Error("Expected RestartDelay to be set")
-					t.Error("Expected RestartDelay to be set")
+			} else if *opts.RestartDelay != tt.expectedDelay {
-				} else if *opts.RestartDelay != tt.expectedDelay {
+				t.Errorf("Expected RestartDelay %d, got %d", tt.expectedDelay, *opts.RestartDelay)
-					t.Errorf("Expected RestartDelay %d, got %d", tt.expectedDelay, *opts.RestartDelay)
-				}
 			}
 		})
 	}

pkg/instance/timeout_test.go

@@ -46,20 +46,6 @@ func TestUpdateLastRequestTime(t *testing.T) {
 
 	// Test that UpdateLastRequestTime doesn't panic
 	inst.UpdateLastRequestTime()
-
-	// Test concurrent calls to ensure thread safety
-	done := make(chan bool, 10)
-	for i := 0; i < 10; i++ {
-		go func() {
-			inst.UpdateLastRequestTime()
-			done <- true
-		}()
-	}
-
-	// Wait for all goroutines to complete
-	for i := 0; i < 10; i++ {
-		<-done
-	}
 }
 
 func TestShouldTimeout_NotRunning(t *testing.T) {
@@ -173,109 +159,6 @@ func TestShouldTimeout_ExceedsTimeLimit(t *testing.T) {
 	}
 }
 
-func TestShouldTimeout_TimeUnitConversion(t *testing.T) {
-	globalSettings := &config.InstancesConfig{
-		LogsDir: "/tmp/test",
-	}
-
-	tests := []struct {
-		name           string
-		idleTimeout    int // in minutes
-		lastRequestAge int // seconds ago
-		shouldTimeout  bool
-	}{
-		{"exactly at timeout boundary", 2, 120, false}, // 2 minutes = 120 seconds
-		{"just over timeout", 2, 121, true},            // 121 seconds > 120 seconds
-		{"well under timeout", 5, 60, false},           // 1 minute < 5 minutes
-		{"well over timeout", 1, 300, true},            // 5 minutes > 1 minute
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			options := &instance.CreateInstanceOptions{
-				IdleTimeout: &tt.idleTimeout,
-				LlamaServerOptions: llamacpp.LlamaServerOptions{
-					Model: "/path/to/model.gguf",
-				},
-			}
-
-			inst := instance.NewInstance("test-instance", globalSettings, options)
-			inst.Running = true
-
-			// Use MockTimeProvider to control time
-			mockTime := NewMockTimeProvider(time.Now())
-			inst.SetTimeProvider(mockTime)
-
-			// Set last request time to now
-			inst.UpdateLastRequestTime()
-
-			// Advance time by the specified amount
-			mockTime.SetTime(time.Now().Add(time.Duration(tt.lastRequestAge) * time.Second))
-
-			result := inst.ShouldTimeout()
-			if result != tt.shouldTimeout {
-				t.Errorf("Expected timeout=%v for %s, got %v", tt.shouldTimeout, tt.name, result)
-			}
-		})
-	}
-}
-
-func TestInstanceTimeoutInitialization(t *testing.T) {
-	globalSettings := &config.InstancesConfig{
-		LogsDir: "/tmp/test",
-	}
-
-	idleTimeout := 5
-	options := &instance.CreateInstanceOptions{
-		IdleTimeout: &idleTimeout,
-		LlamaServerOptions: llamacpp.LlamaServerOptions{
-			Model: "/path/to/model.gguf",
-		},
-	}
-
-	inst := instance.NewInstance("test-instance", globalSettings, options)
-	inst.Running = true
-
-	// Use MockTimeProvider to control time and verify initialization
-	mockTime := NewMockTimeProvider(time.Now())
-	inst.SetTimeProvider(mockTime)
-
-	// Update last request time (simulates what Start() does)
-	inst.UpdateLastRequestTime()
-
-	// Should not timeout immediately after initialization with a 5-minute timeout
-	if inst.ShouldTimeout() {
-		t.Error("Fresh instance should not timeout immediately")
-	}
-
-	// Now advance time by 6 minutes and verify it would timeout
-	mockTime.SetTime(time.Now().Add(6 * time.Minute))
-	if !inst.ShouldTimeout() {
-		t.Error("Instance should timeout after exceeding idle timeout period")
-	}
-}
-
-func TestTimeoutConfiguration_DefaultValue(t *testing.T) {
-	globalSettings := &config.InstancesConfig{
-		LogsDir: "/tmp/test",
-	}
-
-	// Create instance without specifying idle timeout
-	options := &instance.CreateInstanceOptions{
-		LlamaServerOptions: llamacpp.LlamaServerOptions{
-			Model: "/path/to/model.gguf",
-		},
-	}
-
-	inst := instance.NewInstance("test-instance", globalSettings, options)
-	opts := inst.GetOptions()
-
-	// Default should be 0 (disabled)
-	if opts.IdleTimeout == nil || *opts.IdleTimeout != 0 {
-		t.Errorf("Expected default IdleTimeout to be 0, got %v", opts.IdleTimeout)
-	}
-}
-
 func TestTimeoutConfiguration_Validation(t *testing.T) {
 	globalSettings := &config.InstancesConfig{
 		LogsDir: "/tmp/test",
@@ -286,6 +169,7 @@ func TestTimeoutConfiguration_Validation(t *testing.T) {
 		inputTimeout    *int
 		expectedTimeout int
 	}{
+		{"default value when nil", nil, 0},
 		{"positive value", testutil.IntPtr(10), 10},
 		{"zero value", testutil.IntPtr(0), 0},
 		{"negative value gets corrected", testutil.IntPtr(-5), 0},