Repository: knative/serving
Pull request #9243 (2020-09-01 00:44:59)

Cleanup route tests
As I start to ramp up for the gradual rollout project, I am reading things and fixing things.
here's a batch

- no need to copy the whole meta obect just to query lables (perhaps
even passing labels is a better approach)
- collapse test indentations
- add tests and coverage for the missing function
- rename expected to want — shorter :)

/lint

/assign mattmoor @tcnghia 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9243#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9243#pullrequestreview-479218166" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/9243#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [tcnghia,vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-serving-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/route/resources/labels/labels.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/9243/pull-knative-serving-go-coverage/1300595833456037888/artifacts/line-cov.html#file0) | 87.5% | 100.0% | 12.5


Patch:
diff --git a/pkg/reconciler/route/resources/labels/labels.go b/pkg/reconciler/route/resources/labels/labels.go
index 3a77f6017..c6cbeee38 100644
--- a/pkg/reconciler/route/resources/labels/labels.go
+++ b/pkg/reconciler/route/resources/labels/labels.go
@@ -22,8 +22,8 @@ import (
 )
 
 // IsObjectLocalVisibility returns whether an ObjectMeta is of cluster-local visibility
-func IsObjectLocalVisibility(meta v1.ObjectMeta) bool {
-	return meta.Labels != nil && meta.Labels[serving.VisibilityLabelKey] != ""
+func IsObjectLocalVisibility(meta *v1.ObjectMeta) bool {
+	return meta.Labels[serving.VisibilityLabelKey] != ""
 }
 
 // SetVisibility sets the visibility on an ObjectMeta
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index c4389dcdc..9f695f6fa 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -116,7 +116,7 @@ func (t *Config) GetRevisionTrafficTargets(ctx context.Context, r *v1.Route) ([]
 
 			labels.SetVisibility(meta, t.Visibility[tt.Tag] == netv1alpha1.IngressVisibilityClusterLocal)
 
-			// http is currently the only supported scheme
+			// HTTP is currently the only supported scheme.
 			fullDomain, err := domains.DomainNameFromTemplate(ctx, *meta, hostname)
 			if err != nil {
 				return nil, err
diff --git a/pkg/reconciler/route/visibility/visibility.go b/pkg/reconciler/route/visibility/visibility.go
index d6f8892d1..08665b0bc 100644
--- a/pkg/reconciler/route/visibility/visibility.go
+++ b/pkg/reconciler/route/visibility/visibility.go
@@ -99,7 +99,7 @@ func (b *Resolver) GetVisibility(ctx context.Context, route *v1.Route) (map[stri
 		ttVisibility := netv1alpha1.IngressVisibilityExternalIP
 		// Is there a visibility setting on the placeholder Service?
 		if svc, ok := services[hostname]; ok {
-			if labels.IsObjectLocalVisibility(svc.ObjectMeta) {
+			if labels.IsObjectLocalVisibility(&svc.ObjectMeta) {
 				ttVisibility = netv1alpha1.IngressVisibilityClusterLocal
 			}
 		}


Test patch:
diff --git a/pkg/reconciler/route/resources/labels/labels_test.go b/pkg/reconciler/route/resources/labels/labels_test.go
index 24e16c502..a5f8a68bd 100644
--- a/pkg/reconciler/route/resources/labels/labels_test.go
+++ b/pkg/reconciler/route/resources/labels/labels_test.go
@@ -24,47 +24,83 @@ import (
 	"knative.dev/serving/pkg/apis/serving"
 )
 
-func TestDeleteLabel(t *testing.T) {
+func TestIsObjectLocalVisibility(t *testing.T) {
 	tests := []struct {
-		name     string
-		meta     *v1.ObjectMeta
-		key      string
-		expected v1.ObjectMeta
-	}{
-		{
-			name:     "No labels in object meta",
-			meta:     &v1.ObjectMeta{},
-			key:      "key",
-			expected: v1.ObjectMeta{},
+		name string
+		meta *v1.ObjectMeta
+		want bool
+	}{{
+		name: "nil",
+		meta: &v1.ObjectMeta{},
+	}, {
+		name: "empty labels",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{},
+		},
+	}, {
+		name: "no matching labels",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{"frankie-goes": "to-hollywood"},
 		},
-		{
-			name: "No matching key",
-			meta: &v1.ObjectMeta{
-				Labels: map[string]string{"some label": "some value"},
-			},
-			key: "unknown",
-			expected: v1.ObjectMeta{
-				Labels: map[string]string{"some label": "some value"},
-			},
+	}, {
+		name: "false",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{serving.VisibilityLabelKey: ""},
 		},
-		{
-			name: "Has matching key",
-			meta: &v1.ObjectMeta{
-				Labels: map[string]string{"some label": "some value"},
-			},
-			key: "some label",
-			expected: v1.ObjectMeta{
-				Labels: map[string]string{},
-			},
+	}, {
+		name: "true",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{serving.VisibilityLabelKey: "set"},
 		},
+		want: true,
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if got, want := IsObjectLocalVisibility(test.meta), test.want; got != want {
+				t.Errorf("IsObjectLocalVisibility = %v, want: %v", got, want)
+			}
+		})
 	}
+}
+
+func TestDeleteLabel(t *testing.T) {
+	tests := []struct {
+		name string
+		meta *v1.ObjectMeta
+		key  string
+		want v1.ObjectMeta
+	}{{
+		name: "No labels in object meta",
+		meta: &v1.ObjectMeta{},
+		key:  "key",
+		want: v1.ObjectMeta{},
+	}, {
+		name: "No matching key",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{"some label": "some value"},
+		},
+		key: "unknown",
+		want: v1.ObjectMeta{
+			Labels: map[string]string{"some label": "some value"},
+		},
+	}, {
+		name: "Has matching key",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{"some label": "some value"},
+		},
+		key: "some label",
+		want: v1.ObjectMeta{
+			Labels: map[string]string{},
+		},
+	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			DeleteLabel(tt.meta, tt.key)
 
-			if !cmp.Equal(tt.expected, *tt.meta) {
+			if !cmp.Equal(tt.want, *tt.meta) {
 				t.Errorf("DeleteLabel (-want, +got) = %v",
-					cmp.Diff(tt.expected, *tt.meta))
+					cmp.Diff(tt.want, *tt.meta))
 			}
 		})
 	}
@@ -76,48 +112,43 @@ func TestSetLabel(t *testing.T) {
 		meta  *v1.ObjectMeta
 		key   string
 		value string
-
-		expected v1.ObjectMeta
-	}{
-		{
-			name:  "No labels in object meta",
-			meta:  &v1.ObjectMeta{},
-			key:   "key",
-			value: "value",
-			expected: v1.ObjectMeta{
-				Labels: map[string]string{"key": "value"},
-			},
+		want  v1.ObjectMeta
+	}{{
+		name:  "No labels in object meta",
+		meta:  &v1.ObjectMeta{},
+		key:   "key",
+		value: "value",
+		want: v1.ObjectMeta{
+			Labels: map[string]string{"key": "value"},
 		},
-		{
-			name: "Empty labels",
-			meta: &v1.ObjectMeta{
-				Labels: map[string]string{},
-			},
-			key:   "key",
-			value: "value",
-			expected: v1.ObjectMeta{
-				Labels: map[string]string{"key": "value"},
-			},
+	}, {
+		name: "Empty labels",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{},
 		},
-		{
-			name: "Conflicting labels",
-			meta: &v1.ObjectMeta{
-				Labels: map[string]string{"key": "old value"},
-			},
-			key:   "key",
-			value: "new value",
-			expected: v1.ObjectMeta{
-				Labels: map[string]string{"key": "new value"},
-			},
+		key:   "key",
+		value: "value",
+		want: v1.ObjectMeta{
+			Labels: map[string]string{"key": "value"},
 		},
-	}
+	}, {
+		name: "Conflicting labels",
+		meta: &v1.ObjectMeta{
+			Labels: map[string]string{"key": "old value"},
+		},
+		key:   "key",
+		value: "new value",
+		want: v1.ObjectMeta{
+			Labels: map[string]string{"key": "new value"},
+		},
+	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			SetLabel(tt.meta, tt.key, tt.value)
 
-			if !cmp.Equal(tt.expected, *tt.meta) {
+			if !cmp.Equal(tt.want, *tt.meta) {
 				t.Errorf("DeleteLabel (-want, +got) = %v",
-					cmp.Diff(tt.expected, *tt.meta))
+					cmp.Diff(tt.want, *tt.meta))
 			}
 		})
 	}
@@ -128,17 +159,16 @@ func TestSetVisibility(t *testing.T) {
 		name           string
 		meta           *v1.ObjectMeta
 		isClusterLocal bool
-		expected       v1.ObjectMeta
+		want           v1.ObjectMeta
 	}{{
 		name:           "Set cluster local true",
 		meta:           &v1.ObjectMeta{},
 		isClusterLocal: true,
-		expected:       v1.ObjectMeta{Labels: map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}},
+		want:           v1.ObjectMeta{Labels: map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}},
 	}, {
-		name:           "Set cluster local false",
-		meta:           &v1.ObjectMeta{Labels: map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}},
-		isClusterLocal: false,
-		expected:       v1.ObjectMeta{Labels: map[string]string{}},
+		name: "Set cluster local false",
+		meta: &v1.ObjectMeta{Labels: map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}},
+		want: v1.ObjectMeta{Labels: map[string]string{}},
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/test/e2e-common.sh b/test/e2e-common.sh
index 1426b02ff..f9d766343 100644
--- a/test/e2e-common.sh
+++ b/test/e2e-common.sh
@@ -165,7 +165,7 @@ function build_knative_from_source() {
   eval "${ENV_OUTPUT}"
 }
 
-# Installs Knative Serving in the current cluster, and waits for it to be ready.
+# Installs Knative Serving in the current cluster.
 # If no parameters are passed, installs the current source-based build, unless custom
 # YAML files were passed using the --custom-yamls flag.
 # Parameters: $1 - Knative Serving version "HEAD" or "latest-release". Default is "HEAD".
@@ -186,7 +186,7 @@ function install_knative_serving() {
   done
 }
 
-# Installs Knative Serving in the current cluster, and waits for it to be ready.
+# Installs Knative Serving in the current cluster.
 # If no parameters are passed, installs the current source-based build.
 # Parameters: $1 - Knative Serving version "HEAD" or "latest-release".
 #             $2 - Knative Monitoring YAML file (optional)
diff --git a/test/e2e/autoscale.go b/test/e2e/autoscale.go
index 3564f4ec1..ddb73b12c 100644
--- a/test/e2e/autoscale.go
+++ b/test/e2e/autoscale.go
@@ -287,7 +287,7 @@ func numberOfReadyPods(ctx *testContext) (float64, error) {
 	return float64(resources.ReadyAddressCount(eps)), nil
 }
 
-func checkPodScale(ctx *testContext, targetPods, minPods, maxPods float64, duration time.Duration) error {
+func checkPodScale(ctx *testContext, targetPods, minPods, maxPods float64, duration time.Duration, quick bool) error {
 	// Short-circuit traffic generation once we exit from the check logic.
 	done := time.After(duration)
 	ticker := time.NewTicker(2 * time.Second)
@@ -311,8 +311,14 @@ func checkPodScale(ctx *testContext, targetPods, minPods, maxPods float64, durat
 			// A quick test succeeds when the number of pods scales up to `targetPods`
 			// (and, for sanity check, no more than `maxPods`).
 			if got >= targetPods && got <= maxPods {
-				ctx.t.Logf("Got %v replicas, reached target of %v, exiting early", got, targetPods)
-				return nil
+				if quick {
+					// A quick test succeeds when the number of pods scales up to `targetPods`
+					// (and, for sanity check, no more than `maxPods`).
+					if got >= targetPods && got <= maxPods {
+						ctx.t.Logf("Quick Mode: got %v >= %v", got, targetPods)
+						return nil
+					}
+				}
 			}
 			if minPods < targetPods-1 {
 				// Increase `minPods`, but leave room to reduce flakiness.
@@ -363,7 +369,7 @@ func assertAutoscaleUpToNumPods(ctx *testContext, curPods, targetPods float64, d
 
 	grp.Go(func() error {
 		defer close(stopChan)
-		return checkPodScale(ctx, targetPods, minPods, maxPods, duration)
+		return checkPodScale(ctx, targetPods, minPods, maxPods, duration, quick)
 	})
 
 	if err := grp.Wait(); err != nil {
diff --git a/test/e2e/grpc_test.go b/test/e2e/grpc_test.go
index 6d0baf6cb..6f52305b6 100644
--- a/test/e2e/grpc_test.go
+++ b/test/e2e/grpc_test.go
@@ -259,7 +259,7 @@ func assertGRPCAutoscaleUpToNumPods(ctx *testContext, curPods, targetPods float6
 
 	grp.Go(func() error {
 		defer close(stopChan)
-		return checkPodScale(ctx, targetPods, minPods, maxPods, duration)
+		return checkPodScale(ctx, targetPods, minPods, maxPods, duration, true /* quick */)
 	})
 
 	if err := grp.Wait(); err != nil {