Clean up tests: (#679)

- switch reflect.DeepEqual to cmp.Diff for better readability
- some cmp.Diff arguments were reversed(!)
- coverted one test into a table test
- cleaned up some misleading error messages
- preferred testing.Error to testing.Fatal to enable collecting more errors at once

function_migrations_unit_test.go

@@ -10,35 +10,38 @@ import (
 // TestMigrated ensures that the .Migrated() method returns whether or not the
 // migrations were applied based on its self-reported .Version member.
 func TestMigrated(t *testing.T) {
-	// A Function with no migration stamp
-	f := Function{}
-	if f.Migrated() {
-		t.Fatal("function with no version stamp should be not migrated.")
-	}
+	vNext := semver.New(latestMigrationVersion())
+	vNext.BumpMajor()
 
-	// A Function with a migration stamp that is explicitly less than the
-	// latest known.
-	f = Function{Version: "0.0.1"}
-	if f.Migrated() {
-		t.Fatalf("function with version %v when latest is %v should be !migrated",
-			f.Version, latestMigrationVersion())
-	}
+	tests := []struct {
+		name     string
+		f        Function
+		migrated bool
+	}{{
+		name:     "no migration stamp",
+		f:        Function{},
+		migrated: false, // function with no version stamp should be not migrated.
+	}, {
+		name:     "explicit small version",
+		f:        Function{Version: "0.0.1"},
+		migrated: false,
+	}, {
+		name:     "latest version",
+		f:        Function{Version: latestMigrationVersion()},
+		migrated: true,
+	}, {
+		name:     "future version",
+		f:        Function{Version: vNext.String()},
+		migrated: true,
+	}}
 
-	// A Function with a version stamp equivalent to the latest is up-to-date
-	// and should be considered migrated.
-	f = Function{Version: latestMigrationVersion()}
-	if !f.Migrated() {
-		t.Fatalf("function version %v should me considered migrated (latest is %v)",
-			f.Version, latestMigrationVersion())
-	}
-
-	// A Function with a version stamp beyond what is recognized by the current
-	// codebase is considered fully migrated, for purposes of this version of func
-	v0 := semver.New(latestMigrationVersion())
-	v0.BumpMajor()
-	f = Function{Version: v0.String()}
-	if !f.Migrated() {
-		t.Fatalf("Function with version %v should be considered migrated when latest known by this codebase is %v", f.Version, latestMigrationVersion())
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if test.f.Migrated() != test.migrated {
+				t.Errorf("Expected %q.Migrated() to be %t when latest is %q",
+					test.f.Version, test.migrated, latestMigrationVersion())
+			}
+		})
 	}
 }
 

function_test.go

@@ -4,9 +4,10 @@
 package function_test
 
 import (
-	"reflect"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	fn "knative.dev/kn-plugin-func"
 	. "knative.dev/kn-plugin-func/testing"
 )
@@ -41,8 +42,8 @@ func TestWriteIdempotency(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !reflect.DeepEqual(f1, f2) {
-		t.Fatalf("function differs after reload.")
+	if diff := cmp.Diff(f1, f2); diff != "" {
+		t.Error("function differs after reload (-before, +after):", diff)
 	}
 }
 

function_volumes_unit_test.go

@@ -3,7 +3,9 @@
 
 package function
 
-import "testing"
+import (
+	"testing"
+)
 
 func Test_validateVolumes(t *testing.T) {
 

repositories_test.go

@@ -6,9 +6,10 @@ package function_test
 import (
 	"os"
 	"path/filepath"
-	"reflect"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	fn "knative.dev/kn-plugin-func"
 	. "knative.dev/kn-plugin-func/testing"
 )
@@ -125,8 +126,8 @@ func TestRepositoriesAdd(t *testing.T) {
 		t.Fatal(err)
 	}
 	expected := []string{"default", "example"}
-	if !reflect.DeepEqual(rr, expected) {
-		t.Fatalf("Expected '%v', got %v", expected, rr)
+	if diff := cmp.Diff(expected, rr); diff != "" {
+		t.Error("Repositories differed (-want, +got):", diff)
 	}
 
 	// assert a file exists at the location as well indicating it was added to
@@ -165,8 +166,8 @@ func TestRepositoriesAddDeafultName(t *testing.T) {
 		t.Fatal(err)
 	}
 	expected := []string{"default", RepositoriesTestRepo}
-	if !reflect.DeepEqual(rr, expected) {
-		t.Fatalf("Expected '%v', got %v", expected, rr)
+	if diff := cmp.Diff(expected, rr); diff != "" {
+		t.Error("Repositories differed (-want, +got):", diff)
 	}
 }
 
@@ -201,8 +202,8 @@ func TestRepositoriesAddWithManifest(t *testing.T) {
 		t.Fatal(err)
 	}
 	expected := []string{"default", expectedName}
-	if !reflect.DeepEqual(rr, expected) {
-		t.Fatalf("Expected '%v', got %v", expected, rr)
+	if diff := cmp.Diff(expected, rr); diff != "" {
+		t.Error("Repositories differed (-want, +got):", diff)
 	}
 }
 

repository_test.go

@@ -4,7 +4,6 @@
 package function_test
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -74,18 +73,18 @@ func TestRepositoryInheritance(t *testing.T) {
 
 	// Assert Template A reflects repo-level settings
 	if tA.Readiness != "/repoReadiness" {
-		t.Fatalf("Repository-level HealthEndpoint not loaded to template")
+		t.Errorf("Repository-level HealthEndpoint not loaded to template, got %q", tA.Readiness)
 	}
-	if !reflect.DeepEqual(tA.Buildpacks, []string{"repoBuildpack"}) {
-		t.Fatalf("Repository-level HealthEndpoint not loaded to template")
+	if diff := cmp.Diff([]string{"repoBuildpack"}, tA.Buildpacks); diff != "" {
+		t.Errorf("Repository-level Buildpack differs (-want, +got): %s", diff)
 	}
 
 	// Assert Template B reflects runtime-level settings
 	if tB.Readiness != "/runtimeReadiness" {
-		t.Fatalf("Repository-level HealthEndpoint not loaded to template")
+		t.Errorf("Runtime-level HealthEndpoint not loaded to template, got %q", tB.Readiness)
 	}
-	if !reflect.DeepEqual(tB.Buildpacks, []string{"runtimeBuildpack"}) {
-		t.Fatalf("Repository-level HealthEndpoint not loaded to template")
+	if diff := cmp.Diff([]string{"runtimeBuildpack"}, tB.Buildpacks); diff != "" {
+		t.Errorf("Runtime-level Buildpack differs (-want, +got): %s", diff)
 	}
 
 	envVarName := "TEST_RUNTIME_VARIABLE"
@@ -97,18 +96,15 @@ func TestRepositoryInheritance(t *testing.T) {
 		},
 	}
 
-	if !reflect.DeepEqual(tB.BuildEnvs, envs) {
-		if diff := cmp.Diff(tB.BuildEnvs, envs); diff != "" {
-			t.Fatalf("Unexpected difference between repository's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
-		}
+	if diff := cmp.Diff(envs, tB.BuildEnvs); diff != "" {
+		t.Fatalf("Unexpected difference between repository's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
 	}
 
 	// Assert Template C reflects template-level settings
 	if tC.Readiness != "/templateReadiness" {
-		t.Fatalf("Repository-level HealthEndpoint not loaded to template")
+		t.Fatalf("Template-level HealthEndpoint not loaded to template, got %q", tC.Readiness)
 	}
-	if !reflect.DeepEqual(tC.Buildpacks, []string{"templateBuildpack"}) {
-		t.Fatalf("Repository-level HealthEndpoint not loaded to template")
+	if diff := cmp.Diff([]string{"templateBuildpack"}, tC.Buildpacks); diff != "" {
+		t.Fatalf("Template-level Buildpack differs (-want, +got): %s", diff)
 	}
-
 }

templates_test.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"reflect"
 	"runtime"
 	"testing"
 
@@ -41,10 +40,8 @@ func TestTemplatesList(t *testing.T) {
 		"customTemplateRepo/customTemplate",
 	}
 
-	if !reflect.DeepEqual(templates, expected) {
-		t.Logf("expected: %v", expected)
-		t.Logf("received: %v", templates)
-		t.Fatal("Expected templates list not received.")
+	if diff := cmp.Diff(expected, templates); diff != "" {
+		t.Error("Unexpected templates (-want, +got):", diff)
 	}
 }
 
@@ -65,10 +62,8 @@ func TestTemplatesListExtendedNotFound(t *testing.T) {
 		"http",
 	}
 
-	if !reflect.DeepEqual(templates, expected) {
-		t.Logf("expected: %v", expected)
-		t.Logf("received: %v", templates)
-		t.Fatal("Expected templates list not received.")
+	if diff := cmp.Diff(expected, templates); diff != "" {
+		t.Error("Unexpected templates (-want, +got):", diff)
 	}
 }
 
@@ -423,10 +418,8 @@ func TestRuntimeManifestBuildEnvs(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !reflect.DeepEqual(f.BuildEnvs, envs) {
-		if diff := cmp.Diff(f.BuildEnvs, envs); diff != "" {
-			t.Fatalf("Unexpected difference between runtime's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
-		}
+	if diff := cmp.Diff(envs, f.BuildEnvs); diff != "" {
+		t.Fatalf("Unexpected difference between runtime's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
 	}
 }
 
@@ -472,10 +465,8 @@ func TestTemplateManifestBuildEnvs(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !reflect.DeepEqual(f.BuildEnvs, envs) {
-		if diff := cmp.Diff(f.BuildEnvs, envs); diff != "" {
-			t.Fatalf("Unexpected difference between template's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
-		}
+	if diff := cmp.Diff(envs, f.BuildEnvs); diff != "" {
+		t.Fatalf("Unexpected difference between template's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
 	}
 }
 
@@ -521,10 +512,7 @@ func TestRepositoryManifestBuildEnvs(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if !reflect.DeepEqual(f.BuildEnvs, envs) {
-		if diff := cmp.Diff(f.BuildEnvs, envs); diff != "" {
-			t.Fatalf("Unexpected difference between repository's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
-		}
-
+	if diff := cmp.Diff(envs, f.BuildEnvs); diff != "" {
+		t.Fatalf("Unexpected difference between repository's manifest.yaml buildEnvs and Function BuildEnvs (-want, +got): %v", diff)
 	}
 }