Move the use of `VerifyType` in tests (#98)

* Change VerifyType to return an error instead of panicking

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

* Move the use of `VerifyType` in tests

Those calls to `duck.VerifyType` are done at runtime and thus could be
costly at program startup. Putting them under tests ensure we still
assert those types but during unit testing.

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

apis/duck/podspec_test.go

@@ -42,16 +42,6 @@ type WithPodSpec struct {
 	Template PodSpecable `json:"template,omitempty"`
 }
 
-// Check that our canonical type implements PodSpecable.
-var _ = duck.VerifyType(&WithPod{}, &PodSpecable{})
-
-// Check that several Kubernetes built-in type implement PodSpecable.
-var _ = duck.VerifyType(&appsv1.ReplicaSet{}, &PodSpecable{})
-var _ = duck.VerifyType(&appsv1.Deployment{}, &PodSpecable{})
-var _ = duck.VerifyType(&appsv1.StatefulSet{}, &PodSpecable{})
-var _ = duck.VerifyType(&appsv1.DaemonSet{}, &PodSpecable{})
-var _ = duck.VerifyType(&batchv1.Job{}, &PodSpecable{})
-
 var _ duck.Populatable = (*WithPod)(nil)
 var _ duck.Implementable = (*PodSpecable)(nil)
 
@@ -77,8 +67,18 @@ func (t *WithPod) Populate() {
 	}
 }
 
-func TestNothing(t *testing.T) {
+func TestImplementsPodSpecable(t *testing.T) {
-	// Don't test anything, this is simply a demonstration that we
+	instances := []interface{}{
-	// can Duck type core Kubernetes objects and initializing this
+		&WithPod{},
-	// package is sufficient to test that assertion.
+		&appsv1.ReplicaSet{},
+		&appsv1.Deployment{},
+		&appsv1.StatefulSet{},
+		&appsv1.DaemonSet{},
+		&batchv1.Job{},
+	}
+	for _, instance := range instances {
+		if err := duck.VerifyType(instance, &PodSpecable{}); err != nil {
+			t.Error(err)
+		}
+	}
 }

apis/duck/v1alpha1/channelable_types.go

@@ -43,8 +43,6 @@ type ChannelSubscriberSpec struct {
 	SinkableDomain string `json:"sinkableDomain,omitempty"`
 }
 
-// Implementations can verify that they implement Channelable via:
-var _ = duck.VerifyType(&Channel{}, &Channelable{})
 
 // Channelable is an Implementable "duck type".
 var _ duck.Implementable = (*Channelable)(nil)

apis/duck/v1alpha1/conditions_types.go

@@ -93,8 +93,6 @@ func (c *Condition) IsUnknown() bool {
 	return c.Status == corev1.ConditionUnknown
 }
 
-// Implementations can verify that they implement Conditions via:
-var _ = duck.VerifyType(&KResource{}, &Conditions{})
 
 // Conditions is an Implementable "duck type".
 var _ duck.Implementable = (*Conditions)(nil)

apis/duck/v1alpha1/generational_types.go

@@ -27,9 +27,6 @@ import (
 // Generation is the schema for the generational portion of the payload
 type Generation int64
 
-// Implementations can verify that they implement Generation via:
-var emptyGen Generation
-var _ = duck.VerifyType(&Generational{}, &emptyGen)
 
 // Generation is an Implementable "duck type".
 var _ duck.Implementable = (*Generation)(nil)

apis/duck/v1alpha1/implements_test.go

@@ -0,0 +1,44 @@
+/*
+Copyright 2018 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	"testing"
+
+	"github.com/knative/pkg/apis/duck"
+)
+
+func TestTypesImplements(t *testing.T) {
+	var emptyGen Generation
+	testCases := []struct {
+		instance interface{}
+		iface    duck.Implementable
+	}{
+		{instance: &Channel{}, iface: &Channelable{}},
+		{instance: &KResource{}, iface: &Conditions{}},
+		{instance: &Generational{}, iface: &emptyGen},
+		{instance: &LegacyTarget{}, iface: &LegacyTargetable{}},
+		{instance: &Sink{}, iface: &Sinkable{}},
+		{instance: &Subscription{}, iface: &Subscribable{}},
+		{instance: &Target{}, iface: &Targetable{}},
+	}
+	for _, tc := range testCases {
+		if err := duck.VerifyType(tc.instance, tc.iface); err != nil {
+			t.Error(err)
+		}
+	}
+}

apis/duck/v1alpha1/legacy_targetable_types.go

@@ -41,8 +41,6 @@ type LegacyTargetable struct {
 	DomainInternal string `json:"domainInternal,omitempty"`
 }
 
-// Implementations can verify that they implement LegacyTargetable via:
-var _ = duck.VerifyType(&LegacyTarget{}, &LegacyTargetable{})
 
 // LegacyTargetable is an Implementable "duck type".
 var _ duck.Implementable = (*LegacyTargetable)(nil)

apis/duck/v1alpha1/sinkable_types.go

@@ -33,8 +33,6 @@ type Sinkable struct {
 	DomainInternal string `json:"domainInternal,omitempty"`
 }
 
-// Implementations can verify that they implement Sinkable via:
-var _ = duck.VerifyType(&Sink{}, &Sinkable{})
 
 // Sinkable is an Implementable "duck type".
 var _ duck.Implementable = (*Sinkable)(nil)

apis/duck/v1alpha1/subscribable_types.go

@@ -36,8 +36,6 @@ type Subscribable struct {
 	Channelable corev1.ObjectReference `json:"channelable,omitempty"`
 }
 
-// Implementations can verify that they implement Subscribable via:
-var _ = duck.VerifyType(&Subscription{}, &Subscribable{})
 
 // Subscribable is an Implementable "duck type".
 var _ duck.Implementable = (*Subscribable)(nil)

apis/duck/v1alpha1/targetable_types.go

@@ -33,8 +33,6 @@ type Targetable struct {
 	DomainInternal string `json:"domainInternal,omitempty"`
 }
 
-// Implementations can verify that they implement Targetable via:
-var _ = duck.VerifyType(&Target{}, &Targetable{})
 
 // Targetable is an Implementable "duck type".
 var _ duck.Implementable = (*Targetable)(nil)

apis/duck/verify.go

@@ -48,11 +48,10 @@ type Populatable interface {
 //   type ConcreteResource struct { ... }
 //
 //   // Check that ConcreteResource properly implement Fooable.
-//   var _ = duck.VerifyType(&ConcreteResource{}, &something.Fooable{})
+//   err := duck.VerifyType(&ConcreteResource{}, &something.Fooable{})
 //
-// This will panic on startup if the duck typing is not satisfied.  The return
+// This will return an error if the duck typing is not satisfied.
-// value is purely cosmetic to enable the `var _ = ...` shorthand.
+func VerifyType(instance interface{}, iface Implementable) error {
-func VerifyType(instance interface{}, iface Implementable) (nothing interface{}) {
 	// Create instances of the full resource for our input and ultimate result
 	// that we will compare at the end.
 	input, output := iface.GetFullType(), iface.GetFullType()
@@ -63,24 +62,24 @@ func VerifyType(instance interface{}, iface Implementable) (nothing interface{})
 	// Serialize the input to JSON and deserialize that into the provided instance
 	// of the type that we are checking.
 	if before, err := json.Marshal(input); err != nil {
-		panic(fmt.Sprintf("Error serializing duck type %T", input))
+		return fmt.Errorf("error serializing duck type %T", input)
 	} else if err := json.Unmarshal(before, instance); err != nil {
-		panic(fmt.Sprintf("Error deserializing duck type %T into %T", input, instance))
+		return fmt.Errorf("error deserializing duck type %T into %T", input, instance)
 	}
 
 	// Serialize the instance we are checking to JSON and deserialize that into the
 	// output resource.
 	if after, err := json.Marshal(instance); err != nil {
-		panic(fmt.Sprintf("Error serializing %T", instance))
+		return fmt.Errorf("error serializing %T", instance)
 	} else if err := json.Unmarshal(after, output); err != nil {
-		panic(fmt.Sprintf("Error deserializing %T into dock type %T", instance, output))
+		return fmt.Errorf("error deserializing %T into dock type %T", instance, output)
 	}
 
 	// Now verify that we were able to roundtrip all of our fields through the type
 	// we are checking.
 	if diff := cmp.Diff(input, output); diff != "" {
-		panic(fmt.Sprintf("%T does not implement the duck type %T, the following fields were lost: %s",
+		return fmt.Errorf("%T does not implement the duck type %T, the following fields were lost: %s",
-			instance, iface, diff))
+			instance, iface, diff)
 	}
-	return
+	return nil
 }

apis/duck/verify_test.go

@@ -97,14 +97,10 @@ func TestMatches(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			// If we panic, turn it into a test failure
+			if err := VerifyType(test.instance, test.iface); err != nil {
-			defer func() {
+				t.Error(err)
-				if r := recover(); r != nil {
-					t.Errorf("panic: %v", r)
 			}
-			}()
 
-			VerifyType(test.instance, test.iface)
 		})
 	}
 }
@@ -146,15 +142,9 @@ func TestMismatches(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			// Catch panics, they are the failure mode we expect.
+			if err := VerifyType(test.instance, test.iface); err == nil {
-			defer func() {
-				if r := recover(); r != nil {
-					return
-				}
 				t.Errorf("Unexpected success %T implements %T", test.instance, test.iface)
-			}()
+			}
-
-			VerifyType(test.instance, test.iface)
 		})
 	}
 }

testing/resource.go

@@ -23,8 +23,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 
 	"github.com/knative/pkg/apis"
-	"github.com/knative/pkg/apis/duck"
-	duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
 )
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -44,9 +42,6 @@ var _ apis.Immutable = (*Resource)(nil)
 var _ apis.Listable = (*Resource)(nil)
 
 // Check that we implement the Generation duck type.
-var emptyGen duckv1alpha1.Generation
-var _ = duck.VerifyType(&Resource{}, &emptyGen)
-
 type ResourceSpec struct {
 	Generation int64 `json:"generation,omitempty"`
 

testing/resource_test.go

@@ -0,0 +1,31 @@
+/*
+Copyright 2018 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package testing
+
+import (
+	"testing"
+
+	"github.com/knative/pkg/apis/duck"
+	duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
+)
+
+func TestResourceImplementsGenerational(t *testing.T) {
+	var emptyGen duckv1alpha1.Generation
+	if err := duck.VerifyType(&Resource{}, &emptyGen); err != nil {
+		t.Error(err)
+	}
+}

webhook/webhook.go

@@ -275,7 +275,9 @@ func (ac *AdmissionController) Run(stop <-chan struct{}) error {
 	for _, crd := range ac.Handlers {
 		cp := crd.DeepCopyObject()
 		var emptyGen duckv1alpha1.Generation
-		duck.VerifyType(cp, &emptyGen)
+		if err := duck.VerifyType(cp, &emptyGen); err != nil {
+			return err
+		}
 	}
 
 	select {