Adding test cases for serving pkg (#1376)

pkg/serving/config_changes_test.go

@@ -382,6 +382,13 @@ func TestUpdateAnnotationsRemoveExisting(t *testing.T) {
 	assert.DeepEqual(t, expected, actual)
 }
 
+func TestString(t *testing.T) {
+	vt := ConfigMapVolumeSourceType
+	assert.Equal(t, "config-map", vt.String())
+	vt = -1
+	assert.Equal(t, "unknown", vt.String())
+}
+
 //
 // =========================================================================================================
 

pkg/serving/revision_template_test.go

@@ -18,9 +18,11 @@ import (
 	"testing"
 
 	"gotest.tools/v3/assert"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"knative.dev/serving/pkg/apis/autoscaling"
+	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
 type scalingInfoTest struct {
@@ -71,3 +73,29 @@ func TestScalingInfo(t *testing.T) {
 
 	}
 }
+
+func TestAnnotations(t *testing.T) {
+	m := &metav1.ObjectMeta{}
+	m.Annotations = map[string]string{UserImageAnnotationKey: "mockImageVal",
+		autoscaling.TargetAnnotationKey:            "1",
+		autoscaling.TargetUtilizationPercentageKey: "2",
+		autoscaling.WindowAnnotationKey:            "mockWindowVal"}
+	assert.Equal(t, "mockImageVal", UserImage(m))
+	assert.Equal(t, 1, *ConcurrencyTarget(m))
+	assert.Equal(t, 2, *ConcurrencyTargetUtilization(m))
+	assert.Equal(t, "mockWindowVal", AutoscaleWindow(m))
+}
+
+func TestPort(t *testing.T) {
+	revisionSpec := &servingv1.RevisionSpec{
+		PodSpec:              corev1.PodSpec{},
+		ContainerConcurrency: new(int64),
+		TimeoutSeconds:       new(int64),
+	}
+	assert.Equal(t, (*int32)(nil), Port(revisionSpec))
+	revisionSpec.PodSpec.Containers = append(revisionSpec.PodSpec.Containers, corev1.Container{})
+	assert.Equal(t, (*int32)(nil), Port(revisionSpec))
+	port := corev1.ContainerPort{ContainerPort: 42}
+	revisionSpec.PodSpec.Containers[0].Ports = []corev1.ContainerPort{port}
+	assert.Equal(t, (int32)(42), *Port(revisionSpec))
+}

pkg/serving/v1/apply_test.go

@@ -16,6 +16,7 @@ package v1
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -47,6 +48,9 @@ func TestApplyServiceCreate(t *testing.T) {
 	serving.AddReactor("get", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
 			name := a.(clienttesting.GetAction).GetName()
+			if name == "new-service-fail" {
+				return true, nil, errors.NewInternalError(fmt.Errorf("mock internal error"))
+			}
 			return true, nil, errors.NewNotFound(servingv1.Resource("service"), name)
 		})
 
@@ -59,6 +63,15 @@ func TestApplyServiceCreate(t *testing.T) {
 	hasChanged, err := client.ApplyService(context.Background(), serviceNew)
 	assert.NilError(t, err)
 	assert.Assert(t, hasChanged, "service has changed")
+
+	serviceNew = newServiceWithImage("new-service-fail", "test/image")
+	serving.AddReactor("get", "services",
+		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			return true, nil, errors.NewInternalError(fmt.Errorf("mock internal error"))
+		})
+	hasChanged, err = client.ApplyService(context.Background(), serviceNew)
+	assert.ErrorType(t, err, errors.IsInternalError)
+	assert.Assert(t, !hasChanged)
 }
 
 func TestApplyServiceUpdate(t *testing.T) {
@@ -66,15 +79,37 @@ func TestApplyServiceUpdate(t *testing.T) {
 
 	serviceOld := newServiceWithImage("my-service", "test/image")
 	serviceNew := newServiceWithImage("my-service", "test/new-image")
+	serviceConflict := newServiceWithImage("conflict-service", "test/image")
+	serviceErr := newServiceWithImage("err-service", "test/image")
 	serving.AddReactor("get", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
 			name := a.(clienttesting.GetAction).GetName()
-			assert.Equal(t, name, "my-service")
+			var svc *servingv1.Service
-			return true, serviceOld, nil
+			var err error
+			switch name {
+			case "my-service":
+				svc = serviceOld
+			case "conflict-service":
+				svc = serviceConflict
+			case "err-service":
+				svc = serviceErr
+				err = errors.NewInternalError(fmt.Errorf("internal error"))
+			default:
+				t.FailNow()
+			}
+			return true, svc, err
 		})
 
 	serving.AddReactor("patch", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			name := a.(clienttesting.GetAction).GetName()
+			conflictErr := errors.NewConflict(servingv1.Resource("service"), "conflict-service", fmt.Errorf("error patching service"))
+			if name == "conflict-service" {
+				return true, serviceConflict, conflictErr
+			}
+			if name == "err-service" {
+				return true, serviceErr, conflictErr
+			}
 			serviceNew.Generation = 2
 			serviceNew.Status.ObservedGeneration = 1
 			return true, serviceNew, nil
@@ -83,6 +118,24 @@ func TestApplyServiceUpdate(t *testing.T) {
 	hasChanged, err := client.ApplyService(context.Background(), serviceNew)
 	assert.NilError(t, err)
 	assert.Assert(t, hasChanged, "service has changed")
+
+	serviceOld.SetAnnotations(map[string]string{})
+	hasChanged, err = client.ApplyService(context.Background(), serviceNew)
+	assert.NilError(t, err)
+	assert.Assert(t, hasChanged, "service has changed")
+
+	serviceOld.SetAnnotations(map[string]string{corev1.LastAppliedConfigAnnotation: "never"})
+	hasChanged, err = client.ApplyService(context.Background(), serviceNew)
+	assert.ErrorContains(t, err, "Invalid JSON")
+	assert.Assert(t, !hasChanged, "service has not changed")
+
+	hasChanged, err = client.ApplyService(context.Background(), serviceConflict)
+	assert.ErrorType(t, err, errors.IsConflict)
+	assert.Assert(t, !hasChanged, "service has not changed")
+
+	hasChanged, err = client.ApplyService(context.Background(), serviceErr)
+	assert.ErrorType(t, err, errors.IsInternalError)
+	assert.Assert(t, !hasChanged, "service has not changed")
 }
 
 func newServiceWithImage(name string, image string) *servingv1.Service {

pkg/serving/v1/client_test.go

@@ -20,6 +20,8 @@ import (
 	"testing"
 	"time"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+
 	"knative.dev/pkg/apis"
 	duck "knative.dev/pkg/apis/duck/v1"
 
@@ -135,6 +137,20 @@ func TestListService(t *testing.T) {
 
 }
 
+func TestListServiceError(t *testing.T) {
+	serving, client := setup()
+
+	serving.AddReactor("list", "services",
+		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			assert.Equal(t, testNamespace, a.GetNamespace())
+			return true, nil, apierrors.NewInternalError(fmt.Errorf("internal error"))
+		})
+
+	listServices, err := client.ListServices(context.Background())
+	assert.ErrorType(t, err, apierrors.IsInternalError)
+	assert.Assert(t, listServices == nil)
+}
+
 func TestCreateService(t *testing.T) {
 	serving, client := setup()
 
@@ -193,18 +209,90 @@ func TestUpdateService(t *testing.T) {
 	})
 }
 
+func TestUpdateServiceWithRetry(t *testing.T) {
+	serving, client := setup()
+	serviceUpdate := newService("update-service")
+	serviceUpdate.ObjectMeta.Generation = 2
+
+	conflictService := newService("conflict-service")
+
+	serving.AddReactor("update", "services",
+		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			assert.Equal(t, testNamespace, a.GetNamespace())
+			name := a.(clienttesting.UpdateAction).GetObject().(metav1.Object).GetName()
+			if name == serviceUpdate.Name {
+				serviceReturn := newService("update-service")
+				return true, serviceReturn, nil
+			}
+			if name == conflictService.Name {
+				return true, nil, apierrors.NewConflict(servingv1.Resource("service"), "conflict-service", fmt.Errorf("error updating because of conflict"))
+			}
+			return true, nil, fmt.Errorf("error while updating service %s", name)
+		})
+	t.Run("updating a service without error", func(t *testing.T) {
+		_, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) {
+			svc.Name = "update-service"
+			return svc, nil
+		}, 10)
+		assert.NilError(t, err)
+	})
+	t.Run("updating a service with conflict error", func(t *testing.T) {
+		_, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) {
+			svc.Name = "conflict-service"
+			return svc, nil
+		}, 10)
+		assert.ErrorContains(t, err, "conflict")
+	})
+
+	t.Run("updating a service with update func error", func(t *testing.T) {
+		_, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) {
+			svc.Name = "update-service"
+			return svc, fmt.Errorf("update error")
+		}, 10)
+		assert.ErrorContains(t, err, "update error")
+	})
+
+	serving.AddReactor("get", "services",
+		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			name := a.(clienttesting.GetAction).GetName()
+			if name == serviceUpdate.Name {
+				serviceUpdate.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+				return true, serviceUpdate, nil
+			}
+			return true, nil, errors.NewNotFound(servingv1.Resource("service"), name)
+		})
+
+	t.Run("updating a service with deletion error", func(t *testing.T) {
+		_, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) {
+			svc.Name = "update-service"
+			return svc, nil
+		}, 10)
+		assert.ErrorContains(t, err, "marked for deletion")
+	})
+
+	t.Run("updating a service with not found error", func(t *testing.T) {
+		_, err := client.UpdateServiceWithRetry(context.Background(), "unknown-service", func(svc *servingv1.Service) (*servingv1.Service, error) {
+			svc.Name = "unknown-service"
+			return svc, nil
+		}, 10)
+		assert.ErrorContains(t, err, "unknown")
+	})
+}
+
 func TestDeleteService(t *testing.T) {
 	serving, client := setup()
 	const (
 		serviceName            = "test-service"
 		nonExistingServiceName = "no-service"
 		deletedServiceName     = "deleted-service"
+		errServiceName         = "err-service"
 	)
+	delErr := fmt.Errorf("failed to delete service")
 
 	serving.AddReactor("get", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
 			name := a.(clienttesting.GetAction).GetName()
-			if name == serviceName {
+			if name == serviceName || name == errServiceName {
 				// Don't handle existing service, just continue to next
 				return false, nil, nil
 			}
@@ -224,6 +312,9 @@ func TestDeleteService(t *testing.T) {
 			if name == serviceName {
 				return true, nil, nil
 			}
+			if name == errServiceName {
+				return true, nil, delErr
+			}
 			return false, nil, nil
 		})
 	serving.AddWatchReactor("services",
@@ -256,6 +347,11 @@ func TestDeleteService(t *testing.T) {
 		assert.ErrorContains(t, err, "marked for deletion")
 		assert.ErrorContains(t, err, deletedServiceName)
 	})
+
+	t.Run("delete existing service returns an error", func(t *testing.T) {
+		err := client.DeleteService(context.Background(), errServiceName, time.Duration(10)*time.Second)
+		assert.ErrorType(t, err, delErr)
+	})
 }
 
 func TestDeleteServiceNoWait(t *testing.T) {
@@ -393,6 +489,11 @@ func TestDeleteRevision(t *testing.T) {
 		assert.NilError(t, err)
 	})
 
+	t.Run("deleting revision with timeout returns no error", func(t *testing.T) {
+		err := client.DeleteRevision(context.Background(), revisionName, time.Second*10)
+		assert.NilError(t, err)
+	})
+
 	t.Run("trying to delete non-existing revision returns error", func(t *testing.T) {
 		err := client.DeleteRevision(context.Background(), nonExistingRevisionName, 0)
 		assert.ErrorContains(t, err, "not found")
@@ -495,6 +596,23 @@ func TestListRevisions(t *testing.T) {
 	})
 }
 
+func TestListRevisionsError(t *testing.T) {
+	serving, client := setup()
+
+	serving.AddReactor("list", "revisions",
+		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			assert.Equal(t, testNamespace, a.GetNamespace())
+			return true, nil, apierrors.NewInternalError(fmt.Errorf("internal error"))
+		})
+
+	t.Run("list revisions returns an internal error", func(t *testing.T) {
+
+		revisions, err := client.ListRevisions(context.Background())
+		assert.ErrorType(t, err, apierrors.IsInternalError)
+		assert.Assert(t, revisions == nil)
+	})
+}
+
 func TestListRevisionForService(t *testing.T) {
 	fakeServing, client := setup()
 
@@ -609,27 +727,59 @@ func TestListRoutes(t *testing.T) {
 	})
 }
 
+func TestListRoutesError(t *testing.T) {
+	serving, client := setup()
+
+	serving.AddReactor("list", "routes",
+		func(a clienttesting.Action) (bool, runtime.Object, error) {
+			assert.Equal(t, testNamespace, a.GetNamespace())
+			return true, nil, apierrors.NewInternalError(fmt.Errorf("internal error"))
+		})
+
+	t.Run("list routes returns an internal error", func(t *testing.T) {
+
+		routes, err := client.ListRoutes(context.Background())
+		assert.ErrorType(t, err, apierrors.IsInternalError)
+		assert.Assert(t, routes == nil)
+	})
+}
+
 func TestWaitForService(t *testing.T) {
 	serving, client := setup()
 
 	serviceName := "test-service"
+	notFoundServiceName := "not-found-service"
+	internalErrorServiceName := "internal-error-service"
 
 	serving.AddWatchReactor("services",
 		func(a clienttesting.Action) (bool, watch.Interface, error) {
 			watchAction := a.(clienttesting.WatchAction)
-			_, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name")
+			val, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name")
 			if !found {
 				return true, nil, fmt.Errorf("no field selector on metadata.name found")
 			}
-			w := wait.NewFakeWatch(getServiceEvents(serviceName))
+			w := wait.NewFakeWatch(getServiceEvents(val))
 			w.Start()
 			return true, w, nil
 		})
 	serving.AddReactor("get", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
 			getAction := a.(clienttesting.GetAction)
-			assert.Equal(t, getAction.GetName(), serviceName)
+			var err error
-			return true, newService(serviceName), nil
+			var svc *servingv1.Service
+			switch getAction.GetName() {
+			case serviceName:
+				err = nil
+				svc = newService(serviceName)
+			case notFoundServiceName:
+				err = apierrors.NewNotFound(servingv1.Resource("service"), notFoundServiceName)
+			case internalErrorServiceName:
+				err = apierrors.NewInternalError(fmt.Errorf(internalErrorServiceName))
+			default:
+				t.Log("Service name didn't match any of the given patterns")
+				t.FailNow()
+			}
+			return true, svc, err
 		})
 
 	t.Run("wait on a service to become ready with success", func(t *testing.T) {
@@ -637,6 +787,16 @@ func TestWaitForService(t *testing.T) {
 		assert.NilError(t, err)
 		assert.Assert(t, duration > 0)
 	})
+	t.Run("wait on a service to become ready with not found error", func(t *testing.T) {
+		err, duration := client.WaitForService(context.Background(), notFoundServiceName, 60*time.Second, wait.NoopMessageCallback())
+		assert.NilError(t, err)
+		assert.Assert(t, duration > 0)
+	})
+	t.Run("wait on a service to become ready with internal error", func(t *testing.T) {
+		err, duration := client.WaitForService(context.Background(), internalErrorServiceName, 60*time.Second, wait.NoopMessageCallback())
+		assert.ErrorType(t, err, apierrors.IsInternalError)
+		assert.Assert(t, duration == 0)
+	})
 }
 
 type baseRevisionCase struct {
@@ -758,3 +918,13 @@ func getServiceEvents(name string) []watch.Event {
 		{Type: watch.Modified, Object: wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
 	}
 }
+
+func TestCreateRevision(t *testing.T) {
+	_, client := setup()
+	rev := newRevision("mockRevision")
+	assert.NilError(t, client.CreateRevision(context.Background(), rev))
+	rev.Labels = map[string]string{
+		"mockKey": "mockVal",
+	}
+	assert.NilError(t, client.UpdateRevision(context.Background(), rev))
+}

pkg/serving/v1/gitops_test.go

@@ -51,6 +51,7 @@ func TestGitOpsOperations(t *testing.T) {
 	fooSvc := libtest.BuildServiceWithOptions("foo", servingtest.WithConfigSpec(buildConfiguration()))
 	barSvc := libtest.BuildServiceWithOptions("bar", servingtest.WithConfigSpec(buildConfiguration()))
 	fooUpdateSvc := libtest.BuildServiceWithOptions("foo", servingtest.WithConfigSpec(buildConfiguration()), servingtest.WithEnv(corev1.EnvVar{Name: "a", Value: "mouse"}))
+	mockUpdateSvc := libtest.BuildServiceWithOptions("mock", servingtest.WithConfigSpec(buildConfiguration()), servingtest.WithEnv(corev1.EnvVar{Name: "a", Value: "mouse"}))
 
 	fooserviceList := getServiceList([]servingv1.Service{*barSvc, *fooSvc})
 	allServices := getServiceList([]servingv1.Service{*barSvc, *barSvc, *fooSvc})
@@ -129,6 +130,11 @@ func TestGitOpsOperations(t *testing.T) {
 		_, err := fooclient.GetService(context.Background(), "foo")
 		assert.ErrorType(t, err, apierrors.IsNotFound)
 	})
+
+	t.Run("update service foo", func(t *testing.T) {
+		_, err := fooclient.UpdateService(context.Background(), mockUpdateSvc)
+		assert.ErrorType(t, err, apierrors.IsNotFound)
+	})
 }
 
 func TestGitOpsSingleFile(t *testing.T) {
@@ -139,6 +145,8 @@ func TestGitOpsSingleFile(t *testing.T) {
 	fooclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "test.yaml"))
 	barclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "test.yml"))
 	bazclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "test.json"))
+	mockclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "mockfile"))
+	mockDirclient := NewKnServingGitOpsClient("", tmpDir)
 
 	// set up test services
 	testSvc := libtest.BuildServiceWithOptions("test", servingtest.WithConfigSpec(buildConfiguration()))
@@ -202,6 +210,12 @@ func TestGitOpsSingleFile(t *testing.T) {
 		result, err = bazclient.ListServices(context.Background())
 		assert.NilError(t, err)
 		assert.DeepEqual(t, svcList, result)
+
+		_, err = mockclient.ListServices(context.Background())
+		assert.ErrorContains(t, err, "no such file")
+
+		_, err = mockDirclient.ListServices(context.Background())
+		assert.NilError(t, err)
 	})
 	t.Run("delete service foo", func(t *testing.T) {
 		err := fooclient.DeleteService(context.Background(), "test", 5*time.Second)