Rationalize golden-output comparison

Create a single helper function: AssertMatchesFile

Also a few output files that weren't consistent.

cmd/kops/BUILD.bazel

@@ -179,6 +179,5 @@ go_test(
         "//vendor/golang.org/x/crypto/ssh:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
-        "//vendor/k8s.io/klog:go_default_library",
     ],
 )

cmd/kops/create_cluster_integration_test.go

@@ -19,7 +19,6 @@ package main
 import (
 	"bytes"
 	"io/ioutil"
-	"os"
 	"path"
 	"strings"
 	"testing"
@@ -27,14 +26,12 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
-	"k8s.io/klog"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/kops/cloudmock/aws/mockec2"
 	"k8s.io/kops/cmd/kops/util"
 	"k8s.io/kops/pkg/apis/kops"
-	"k8s.io/kops/pkg/diff"
 	"k8s.io/kops/pkg/kopscodecs"
 	"k8s.io/kops/pkg/testutils"
 	"k8s.io/kops/upup/pkg/fi"
@@ -245,37 +242,6 @@ func runCreateClusterIntegrationTest(t *testing.T, srcDir string, version string
 		yamlAll = append(yamlAll, actualYAML)
 	}
 
-	expectedYAMLBytes, err := ioutil.ReadFile(path.Join(srcDir, expectedClusterPath))
-	if err != nil {
-		t.Fatalf("unexpected error reading expected YAML: %v", err)
-	}
-
-	//on windows, with git set to autocrlf, the reference files on disk have windows line endings
-	expectedYAML := strings.Replace(strings.TrimSpace(string(expectedYAMLBytes)), "\r\n", "\n", -1)
-
 	actualYAML := strings.Join(yamlAll, "\n\n---\n\n")
-	if actualYAML != expectedYAML {
+	testutils.AssertMatchesFile(t, actualYAML, path.Join(srcDir, expectedClusterPath))
-		p := path.Join(srcDir, expectedClusterPath)
-
-		if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
-			t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", p)
-
-			// Format nicely - keep git happy
-			s := actualYAML
-			s = strings.TrimSpace(s)
-			s = s + "\n"
-
-			if err := ioutil.WriteFile(p, []byte(s), 0644); err != nil {
-				t.Errorf("error writing expected output %s: %v", p, err)
-			}
-		}
-
-		klog.Infof("Actual YAML:\n%s\n", actualYAML)
-
-		diffString := diff.FormatDiff(expectedYAML, actualYAML)
-		t.Logf("diff:\n%s\n", diffString)
-
-		t.Errorf("YAML differed from expected (%s)", p)
-	}
-
 }

cmd/kops/integration_test.go

@@ -307,27 +307,8 @@ func runTest(t *testing.T, h *testutils.IntegrationTestHarness, clusterName stri
 		if err != nil {
 			t.Fatalf("unexpected error reading actual terraform output: %v", err)
 		}
-		expectedTF, err := ioutil.ReadFile(path.Join(srcDir, testDataTFPath))
-		if err != nil {
-			t.Fatalf("unexpected error reading expected terraform output: %v", err)
-		}
-		expectedTF = bytes.Replace(expectedTF, []byte("\r\n"), []byte("\n"), -1)
 
-		if !bytes.Equal(actualTF, expectedTF) {
+		testutils.AssertMatchesFile(t, string(actualTF), path.Join(srcDir, testDataTFPath))
-			diffString := diff.FormatDiff(string(expectedTF), string(actualTF))
-			t.Logf("diff:\n%s\n", diffString)
-
-			if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
-				fp := path.Join(srcDir, testDataTFPath)
-				t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", fp)
-				if err := ioutil.WriteFile(fp, actualTF, 0644); err != nil {
-					t.Errorf("error writing terraform output: %v", err)
-				}
-				t.Errorf("terraform output differed from expected")
-				return // Avoid Fatalf as we want to keep going and update all files
-			}
-			t.Fatalf("terraform output differed from expected")
-		}
 	}
 
 	// Compare data files if they are provided
@@ -587,10 +568,6 @@ func runTestCloudformation(t *testing.T, clusterName string, srcDir string, vers
 		if err != nil {
 			t.Fatalf("unexpected error reading actual cloudformation output: %v", err)
 		}
-		expectedCF, err := ioutil.ReadFile(path.Join(srcDir, expectedCfPath))
-		if err != nil {
-			t.Fatalf("unexpected error reading expected cloudformation output: %v", err)
-		}
 
 		// Expand out the UserData base64 blob, as otherwise testing is painful
 		extracted := make(map[string]string)
@@ -625,28 +602,7 @@ func runTestCloudformation(t *testing.T, clusterName string, srcDir string, vers
 		}
 		actualCF = buf.Bytes()
 
-		expectedCFTrimmed := strings.Replace(strings.TrimSpace(string(expectedCF)), "\r\n", "\n", -1)
+		testutils.AssertMatchesFile(t, string(actualCF), path.Join(srcDir, expectedCfPath))
-		actualCFTrimmed := strings.TrimSpace(string(actualCF))
-		if actualCFTrimmed != expectedCFTrimmed {
-			diffString := diff.FormatDiff(expectedCFTrimmed, actualCFTrimmed)
-			t.Logf("diff:\n%s\n", diffString)
-
-			if os.Getenv("KEEP_TEMP_DIR") == "" {
-				t.Logf("(hint: setting KEEP_TEMP_DIR will preserve test output")
-			} else {
-				t.Logf("actual terraform output in %s", actualPath)
-			}
-
-			if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
-				fp := path.Join(srcDir, expectedCfPath)
-				t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", fp)
-				if err := ioutil.WriteFile(fp, actualCF, 0644); err != nil {
-					t.Errorf("error writing expected output file %q: %v", fp, err)
-				}
-			}
-
-			t.Fatalf("cloudformation output differed from expected. Test file: %s", path.Join(srcDir, expectedCfPath))
-		}
 
 		fp := path.Join(srcDir, expectedCfPath+".extracted.yaml")
 		expectedExtracted, err := ioutil.ReadFile(fp)
@@ -685,24 +641,7 @@ func runTestCloudformation(t *testing.T, clusterName string, srcDir string, vers
 			}
 		}
 
-		if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
+		testutils.AssertMatchesFile(t, string(actualCF), path.Join(srcDir, expectedCfPath))
-			t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", fp)
-
-			// We replace the \r characters so that the yaml output (should) be block-quoted
-			// Literal quoting is sadly unreadable...
-			for k, v := range actual {
-				actual[k] = strings.Replace(v, "\r", "", -1)
-			}
-
-			b, err := yaml.Marshal(actual)
-			if err != nil {
-				t.Errorf("error serializing cloudformation output: %v", err)
-			}
-			if err := ioutil.WriteFile(fp, b, 0644); err != nil {
-				t.Errorf("error writing cloudformation output: %v", err)
-			}
-		}
-
 	}
 }
 

hack/update-expected.sh

@@ -20,4 +20,4 @@ set -o pipefail
 KOPS_ROOT=$(git rev-parse --show-toplevel)
 cd ${KOPS_ROOT}
 
-UPDATE_CHANNEL_BUILDER_TEST_FIXTURES=true HACK_UPDATE_EXPECTED_IN_PLACE=1 go test ./...
+HACK_UPDATE_EXPECTED_IN_PLACE=1 go test ./...

nodeup/pkg/model/tests/kubelet/featuregates/tasks.yaml

@@ -28,4 +28,4 @@ definition: |
 enabled: true
 manageState: true
 running: true
-smartRestart: true
+smartRestart: true

pkg/model/BUILD.bazel

@@ -66,7 +66,7 @@ go_test(
     deps = [
         "//pkg/apis/kops:go_default_library",
         "//pkg/apis/nodeup:go_default_library",
-        "//pkg/diff:go_default_library",
+        "//pkg/testutils:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
     ],
 )

pkg/model/bootstrapscript_test.go

@@ -17,14 +17,12 @@ limitations under the License.
 package model
 
 import (
-	"io/ioutil"
-	"os"
 	"strings"
 	"testing"
 
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/apis/nodeup"
-	"k8s.io/kops/pkg/diff"
+	"k8s.io/kops/pkg/testutils"
 )
 
 func Test_ProxyFunc(t *testing.T) {
@@ -143,23 +141,7 @@ func TestBootstrapUserData(t *testing.T) {
 			continue
 		}
 
-		expectedBytes, err := ioutil.ReadFile(x.ExpectedFilePath)
+		testutils.AssertMatchesFile(t, actual, x.ExpectedFilePath)
-		if err != nil {
-			t.Fatalf("unexpected error reading ExpectedFilePath %q: %v", x.ExpectedFilePath, err)
-		}
-
-		if actual != string(expectedBytes) {
-			if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
-				t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", x.ExpectedFilePath)
-				if err := ioutil.WriteFile(x.ExpectedFilePath, []byte(actual), 0644); err != nil {
-					t.Errorf("error writing expected output: %v", err)
-				}
-			}
-
-			diffString := diff.FormatDiff(string(expectedBytes), actual)
-			t.Errorf("case %d failed, actual output differed from expected (%s).", i, x.ExpectedFilePath)
-			t.Logf("diff:\n%s\n", diffString)
-		}
 	}
 }
 

pkg/testutils/modelharness.go

@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
 	"sort"
 	"strings"
 	"testing"
@@ -95,27 +96,58 @@ func ValidateTasks(t *testing.T, basedir string, context *fi.ModelBuilderContext
 	}
 
 	actualTasksYaml := strings.Join(yamls, "\n---\n")
+	actualTasksYaml = strings.TrimSpace(actualTasksYaml)
 
 	tasksYamlPath := path.Join(basedir, "tasks.yaml")
-	expectedTasksYamlBytes, err := ioutil.ReadFile(tasksYamlPath)
+
+	AssertMatchesFile(t, actualTasksYaml, tasksYamlPath)
+}
+
+// AssertMatchesFile matches the actual value to a with expected file.
+// If HACK_UPDATE_EXPECTED_IN_PLACE is set, it will write the actual value to the expected file,
+// which is very handy when updating our tests.
+func AssertMatchesFile(t *testing.T, actual string, p string) {
+	actual = strings.TrimSpace(actual)
+
+	expectedBytes, err := ioutil.ReadFile(p)
 	if err != nil {
-		t.Fatalf("error reading file %q: %v", tasksYamlPath, err)
+		t.Fatalf("error reading file %q: %v", p, err)
+	}
+	expected := strings.TrimSpace(string(expectedBytes))
+
+	//on windows, with git set to autocrlf, the reference files on disk have windows line endings
+	expected = strings.Replace(expected, "\r\n", "\n", -1)
+
+	if actual == expected {
+		return
 	}
 
-	actualTasksYaml = strings.TrimSpace(actualTasksYaml)
+	if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
-	expectedTasksYaml := strings.TrimSpace(string(expectedTasksYamlBytes))
+		t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", p)
 
-	if expectedTasksYaml != actualTasksYaml {
+		// Keep git happy with a trailing newline
-		if os.Getenv("HACK_UPDATE_EXPECTED_IN_PLACE") != "" {
+		actual += "\n"
-			t.Logf("HACK_UPDATE_EXPECTED_IN_PLACE: writing expected output %s", tasksYamlPath)
+
-			if err := ioutil.WriteFile(tasksYamlPath, []byte(actualTasksYaml), 0644); err != nil {
+		if err := ioutil.WriteFile(p, []byte(actual), 0644); err != nil {
-				t.Errorf("error writing expected output %s: %v", tasksYamlPath, err)
+			t.Errorf("error writing expected output %s: %v", p, err)
-			}
 		}
 
-		diffString := diff.FormatDiff(expectedTasksYaml, actualTasksYaml)
+		// Keep going so we write all files in a test
-		t.Logf("diff:\n%s\n", diffString)
+		t.Errorf("output did not match expected for %q", p)
-
+		return
-		t.Fatalf("tasks differed from expected for test %q", basedir)
 	}
+
+	diffString := diff.FormatDiff(expected, actual)
+	t.Logf("diff:\n%s\n", diffString)
+
+	abs, err := filepath.Abs(p)
+	if err != nil {
+		t.Errorf("unable to get absolute path for %q: %v", p, err)
+	} else {
+		p = abs
+	}
+
+	t.Logf("to update golden output automatically, run hack/update-expected.sh")
+
+	t.Fatalf("output did not match expected for %q", p)
 }

tests/integration/update_cluster/additional_user-data/cloudformation.json

@@ -865,4 +865,4 @@
       }
     }
   }
-}
+}

tests/integration/update_cluster/mixed_instances/cloudformation.json

@@ -1240,4 +1240,4 @@
       }
     }
   }
-}
+}

tests/integration/update_cluster/mixed_instances_spot/cloudformation.json

@@ -1241,4 +1241,4 @@
       }
     }
   }
-}
+}

upup/pkg/fi/cloudup/BUILD.bazel

@@ -111,7 +111,6 @@ go_test(
         "//pkg/apis/kops/validation:go_default_library",
         "//pkg/assets:go_default_library",
         "//pkg/client/simple/vfsclientset:go_default_library",
-        "//pkg/diff:go_default_library",
         "//pkg/kopscodecs:go_default_library",
         "//pkg/model:go_default_library",
         "//pkg/templates:go_default_library",

upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go

@@ -18,15 +18,12 @@ package cloudup
 
 import (
 	"io/ioutil"
-	"os"
 	"path"
-	"strings"
 	"testing"
 
 	api "k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/assets"
 	"k8s.io/kops/pkg/client/simple/vfsclientset"
-	"k8s.io/kops/pkg/diff"
 	"k8s.io/kops/pkg/kopscodecs"
 	"k8s.io/kops/pkg/model"
 	"k8s.io/kops/pkg/templates"
@@ -121,19 +118,6 @@ func runChannelBuilderTest(t *testing.T, key string) {
 	}
 
 	expectedManifestPath := path.Join(basedir, "manifest.yaml")
-	expectedManifest, err := ioutil.ReadFile(expectedManifestPath)
-	if err != nil {
-		t.Fatalf("error reading file %q: %v", expectedManifestPath, err)
-	}
 
-	if strings.TrimSpace(string(expectedManifest)) != strings.TrimSpace(actualManifest) {
+	testutils.AssertMatchesFile(t, actualManifest, expectedManifestPath)
-		diffString := diff.FormatDiff(string(expectedManifest), actualManifest)
-		t.Logf("diff:\n%s\n", diffString)
-		t.Errorf("manifest differed from expected for test %q", key)
-		if os.Getenv("UPDATE_CHANNEL_BUILDER_TEST_FIXTURES") == "true" {
-			ioutil.WriteFile(expectedManifestPath, []byte(actualManifest), 0755)
-		} else {
-			t.Logf("to update fixtures automatically, run with UPDATE_CHANNEL_BUILDER_TEST_FIXTURES=true")
-		}
-	}
 }