From c00f02b369a1fce9398dd8f2af686c34b9d56d83 Mon Sep 17 00:00:00 2001 From: Joshua Bone Date: Thu, 17 Sep 2020 11:56:42 -0700 Subject: [PATCH] Mock log.Fatalf calls and add test suite (#2437) * Add unit test for outputConfig * Fix PR comments * Use outputter struct with count instead of separate global vars * Add unit test for outputConfig * Fix PR comments * Use outputter struct with count instead of separate global vars * Fix linter comment * Update tools/config-generator/testgrid_config.go Co-authored-by: Matt Moore * Fix PR comments * address PR comments * Make log.Fatalf function replaceable for testing * add unit tests for utils * Unit tests now call SetupForTesting() Co-authored-by: Matt Moore --- tools/config-generator/main.go | 28 +- tools/config-generator/main_test.go | 6 +- tools/config-generator/periodic_config.go | 6 +- tools/config-generator/testutils_test.go | 13 + tools/config-generator/udpaterelease.go | 4 +- tools/config-generator/utils.go | 13 +- tools/config-generator/utils_test.go | 333 ++++++++++++++++++++++ 7 files changed, 374 insertions(+), 29 deletions(-) create mode 100644 tools/config-generator/utils_test.go diff --git a/tools/config-generator/main.go b/tools/config-generator/main.go index b0f36c9731f..c98fd2332a4 100644 --- a/tools/config-generator/main.go +++ b/tools/config-generator/main.go @@ -60,6 +60,8 @@ var ( nonPathAliasRepos = sets.NewString("knative/docs") ) +type logFatalfFunc func(string, ...interface{}) + // repositoryData contains basic data about each Knative repository. type repositoryData struct { Name string @@ -159,6 +161,7 @@ var ( // TODO: these should be CapsCase // ... until they are not global output outputter + logFatalf logFatalfFunc prowHost string testGridHost string gubernatorHost string @@ -211,7 +214,7 @@ func readTemplate(fp string) string { _, f, _, _ := runtime.Caller(0) content, err := ioutil.ReadFile(path.Join(path.Dir(f), templateDir, fp)) if err != nil { - log.Fatalf("Failed read file '%s': '%v'", fp, err) + logFatalf("Failed read file '%s': '%v'", fp, err) } templatesCache[fp] = string(content) } @@ -319,7 +322,7 @@ func configureServiceAccountForJob(data *baseProwJobTemplateData) { } p := strings.Split(data.ServiceAccount, "/") if len(p) != 4 || p[0] != "" || p[1] != "etc" || p[3] != "service-account.json" { - log.Fatalf("Service account path %q is expected to be \"/etc//service-account.json\"", data.ServiceAccount) + logFatalf("Service account path %q is expected to be \"/etc//service-account.json\"", data.ServiceAccount) } name := p[2] addVolumeToJob(data, "/etc/"+name, name, true, nil) @@ -330,7 +333,7 @@ func addExtraEnvVarsToJob(envVars []string, data *baseProwJobTemplateData) { for _, env := range envVars { pair := strings.SplitN(env, "=", 2) if len(pair) != 2 { - log.Fatalf("Environment variable %q is expected to be \"key=value\"", env) + logFatalf("Environment variable %q is expected to be \"key=value\"", env) } data.addEnvToJob(pair[0], pair[1]) } @@ -415,7 +418,7 @@ func parseBasicJobConfigOverrides(data *baseProwJobTemplateData, config yaml.Map case nil: // already processed continue default: - log.Fatalf("Unknown entry %q for job", item.Key) + logFatalf("Unknown entry %q for job", item.Key) } // Knock-out the item, signalling it was already parsed. config[i] = yaml.MapItem{} @@ -525,7 +528,7 @@ func executeTemplate(name, templ string, data interface{}) { } t := template.Must(template.New(name).Funcs(funcMap).Delims("[[", "]]").Parse(templ)) if err := t.Execute(&res, data); err != nil { - log.Fatalf("Error in template %s: %v", name, err) + logFatalf("Error in template %s: %v", name, err) } for _, line := range strings.Split(res.String(), "\n") { output.outputConfig(line) @@ -551,7 +554,7 @@ func parseJob(config yaml.MapSlice, jobName string) yaml.MapSlice { } } - log.Fatalf("The metadata misses %s configuration, cannot continue.", jobName) + logFatalf("The metadata misses %s configuration, cannot continue.", jobName) return nil } @@ -683,7 +686,7 @@ func setOutput(fileName string) { } configFile, err := os.OpenFile(fileName, os.O_RDWR|os.O_CREATE, 0666) if err != nil { - log.Fatalf("Cannot create the configuration file %q: %v", fileName, err) + logFatalf("Cannot create the configuration file %q: %v", fileName, err) } configFile.Truncate(0) configFile.Seek(0, 0) @@ -692,6 +695,7 @@ func setOutput(fileName string) { // main is the script entry point. func main() { + logFatalf = log.Fatalf // Parse flags and sanity check them. prowJobsConfigOutput := "" testgridConfigOutput := "" @@ -736,19 +740,19 @@ func main() { if upgradeReleaseBranches { gc, err := ghutil.NewGithubClient(githubTokenPath) if err != nil { - log.Fatalf("Failed creating github client from %q: %v", githubTokenPath, err) + logFatalf("Failed creating github client from %q: %v", githubTokenPath, err) } if err := upgradeReleaseBranchesTemplate(name, gc); err != nil { - log.Fatalf("Failed upgrade based on release branch: '%v'", err) + logFatalf("Failed upgrade based on release branch: '%v'", err) } } content, err := ioutil.ReadFile(name) if err != nil { - log.Fatalf("Cannot read file %q: %v", name, err) + logFatalf("Cannot read file %q: %v", name, err) } if err = yaml.Unmarshal(content, &config); err != nil { - log.Fatalf("Cannot parse config %q: %v", name, err) + logFatalf("Cannot parse config %q: %v", name, err) } prowConfigData := getProwConfigData(config) @@ -778,7 +782,7 @@ func main() { // config object is modified when we generate prow config, so we'll need to reload it here if err = yaml.Unmarshal(content, &config); err != nil { - log.Fatalf("Cannot parse config %q: %v", name, err) + logFatalf("Cannot parse config %q: %v", name, err) } // Generate Testgrid config. if *generateTestgridConfig { diff --git a/tools/config-generator/main_test.go b/tools/config-generator/main_test.go index 63779c24e68..20cba576cde 100644 --- a/tools/config-generator/main_test.go +++ b/tools/config-generator/main_test.go @@ -14,17 +14,13 @@ limitations under the License. package main import ( - "os" "testing" "github.com/google/go-cmp/cmp" ) -func TestMain(m *testing.M) { - ResetOutput() // Redirect output prior to each test. - os.Exit(m.Run()) -} func TestOutputConfig(t *testing.T) { + SetupForTesting() output.outputConfig("") if diff := cmp.Diff(GetOutput(), ""); diff != "" { t.Errorf("Incorrect output for empty string: (-got +want)\n%s", diff) diff --git a/tools/config-generator/periodic_config.go b/tools/config-generator/periodic_config.go index 04a553d6e0b..d73a56ceee2 100644 --- a/tools/config-generator/periodic_config.go +++ b/tools/config-generator/periodic_config.go @@ -229,13 +229,13 @@ func generatePeriodic(title string, repoName string, periodicConfig yaml.MapSlic } // Ensure required data exist. if data.CronString == "" { - log.Fatalf("Job %q is missing cron string", data.PeriodicJobName) + logFatalf("Job %q is missing cron string", data.PeriodicJobName) } if len(data.Base.Args) == 0 && data.Base.Command == "" { - log.Fatalf("Job %q is missing command", data.PeriodicJobName) + logFatalf("Job %q is missing command", data.PeriodicJobName) } if jobType == "branch-ci" && data.Base.RepoBranch == "" { - log.Fatalf("%q jobs are intended to be used on release branches", jobType) + logFatalf("%q jobs are intended to be used on release branches", jobType) } // Generate config itself. diff --git a/tools/config-generator/testutils_test.go b/tools/config-generator/testutils_test.go index c8cb31b9404..02e6d42bcd9 100644 --- a/tools/config-generator/testutils_test.go +++ b/tools/config-generator/testutils_test.go @@ -19,6 +19,13 @@ import ( var outputBuffer bytes.Buffer +// logFatalCalls tracks the number of logFatalf calls that occurred within a test +var logFatalCalls int + +func logFatalfMock(format string, v ...interface{}) { + logFatalCalls++ +} + func ResetOutput() { outputBuffer = bytes.Buffer{} output = newOutputter(&outputBuffer) @@ -27,3 +34,9 @@ func ResetOutput() { func GetOutput() string { return outputBuffer.String() } + +func SetupForTesting() { + ResetOutput() // Redirect output prior to each test. + logFatalf = logFatalfMock + logFatalCalls = 0 +} diff --git a/tools/config-generator/udpaterelease.go b/tools/config-generator/udpaterelease.go index 29147e4f39e..5ec607eb6e4 100644 --- a/tools/config-generator/udpaterelease.go +++ b/tools/config-generator/udpaterelease.go @@ -208,7 +208,7 @@ func versionComp(v1, v2 string) int { func mustInt(s string) int { r, err := strconv.Atoi(s) if err != nil { - log.Fatalf("Failed to parse int %q: %v", s, err) + logFatalf("Failed to parse int %q: %v", s, err) } return r } @@ -216,7 +216,7 @@ func mustInt(s string) int { func majorMinor(s string) (int, int) { parts := strings.Split(s, ".") if len(parts) != 2 { - log.Fatalf("Version string has to be in the form of [MAJOR].[MINOR]: %q", s) + logFatalf("Version string has to be in the form of [MAJOR].[MINOR]: %q", s) } return mustInt(parts[0]), mustInt(parts[1]) } diff --git a/tools/config-generator/utils.go b/tools/config-generator/utils.go index 03158e1b483..cfe2e42a358 100644 --- a/tools/config-generator/utils.go +++ b/tools/config-generator/utils.go @@ -17,7 +17,6 @@ limitations under the License. package main import ( - "log" "sort" "strconv" "strings" @@ -33,12 +32,12 @@ func getString(s interface{}) string { if len(values) == 1 { return values[0] } - log.Fatalf("Entry %v is not a string or string array of size 1", s) + logFatalf("Entry %v is not a string or string array of size 1", s) } if str, ok := s.(string); ok { return str } - log.Fatalf("Entry %v is not a string", s) + logFatalf("Entry %v is not a string", s) return "" } @@ -47,7 +46,7 @@ func getInt(s interface{}) int { if value, ok := s.(int); ok { return value } - log.Fatalf("Entry %v is not an integer", s) + logFatalf("Entry %v is not an integer", s) return 0 } @@ -56,7 +55,7 @@ func getBool(s interface{}) bool { if value, ok := s.(bool); ok { return value } - log.Fatalf("Entry %v is not a boolean", s) + logFatalf("Entry %v is not a boolean", s) return false } @@ -65,7 +64,7 @@ func getInterfaceArray(s interface{}) []interface{} { if interfaceArray, ok := s.([]interface{}); ok { return interfaceArray } - log.Fatalf("Entry %v is not an interface array", s) + logFatalf("Entry %v is not an interface array", s) return nil } @@ -84,7 +83,7 @@ func getMapSlice(m interface{}) yaml.MapSlice { if mm, ok := m.(yaml.MapSlice); ok { return mm } - log.Fatalf("Entry %v is not a yaml.MapSlice", m) + logFatalf("Entry %v is not a yaml.MapSlice", m) return nil } diff --git a/tools/config-generator/utils_test.go b/tools/config-generator/utils_test.go new file mode 100644 index 00000000000..021617f30df --- /dev/null +++ b/tools/config-generator/utils_test.go @@ -0,0 +1,333 @@ +/* +Copyright 2020 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 main + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "gopkg.in/yaml.v2" +) + +func TestGetString(t *testing.T) { + SetupForTesting() + var in interface{} = "abcdefg" + out := getString(in) + if diff := cmp.Diff(out, "abcdefg"); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } + if logFatalCalls != 0 { + t.Fatalf("logFatal was called for %v", in) + } + + out = getString(42) + if logFatalCalls != 1 { + t.Fatalf("logFatal was not called for %v", in) + } +} + +func TestGetInt(t *testing.T) { + SetupForTesting() + var in interface{} = 123 + out := getInt(in) + if logFatalCalls != 0 { + t.Fatalf("logFatal was called for %v", in) + } + if out != 123 { + t.Fatalf("Expected 123, got %v", out) + } + + getInt("abc") + if logFatalCalls == 0 { + t.Fatalf("Expected logFatal to be called") + } +} + +func TestGetBool(t *testing.T) { + SetupForTesting() + var in interface{} = true + out := getBool(in) + if logFatalCalls != 0 { + t.Fatalf("logFatal was called for %v", in) + } + if !out { + t.Fatalf("Expected true, got %v", out) + } + + getBool(123) + if logFatalCalls == 0 { + t.Fatalf("Expected logFatal to be called") + } +} + +func TestGetInterfaceArray(t *testing.T) { + SetupForTesting() + in1 := []interface{}{"foo", "bar", "baz"} + out1 := getInterfaceArray(in1) + if fmt.Sprint(in1) != fmt.Sprint(out1) { + t.Fatalf("Did not get same interface slice back.") + } + if logFatalCalls != 0 { + t.Fatalf("Interface slice caused logFatal call") + } + + in2 := []string{"foo", "bar", "baz"} + getInterfaceArray(in2) + if logFatalCalls != 1 { + t.Fatalf("Non interface slice should have caused logFatal call") + } +} + +func TestGetStringArray(t *testing.T) { + SetupForTesting() + in := []interface{}{"foo", "bar", "baz"} + out := getStringArray(in) + if logFatalCalls != 0 { + t.Fatalf("Input %v should not have caused logFatal call.", in) + } + if fmt.Sprint(out) != fmt.Sprint(in) { + t.Fatalf("Expected input %v and output %v to have identical string output.", in, out) + } +} + +func TestGetMapSlice(t *testing.T) { + SetupForTesting() + var in interface{} = yaml.MapSlice{ + yaml.MapItem{Key: "abc", Value: 123}, + yaml.MapItem{Key: "def", Value: 456}, + } + out := getMapSlice(in) + if logFatalCalls != 0 { + t.Fatalf("Input %v should not have caused logFatal call.", in) + } + if fmt.Sprint(out) != fmt.Sprint(in) { + t.Fatalf("Expected input %v and output %v to have identical string output.", in, out) + } +} + +func TestAppendIfUnique(t *testing.T) { + SetupForTesting() + arr := []string{"foo", "bar"} + arr = appendIfUnique(arr, "foo") + if len(arr) != 2 { + t.Fatalf("Expected length 2 but was %v", len(arr)) + } + arr = appendIfUnique(arr, "baz") + if arr[2] != "baz" { + t.Fatalf("Expected 'baz' to be appended but wasn't.") + } +} + +func TestIsNum(t *testing.T) { + SetupForTesting() + nums := []string{"-123456.789", "-123", "0", "0.0", ".0", "123", "123456.789"} + for _, n := range nums { + if !isNum(n) { + t.Fatalf("Input %v should be a num, but wasn't.", n) + } + } + notNums := []string{"", ".", "abc", "123 "} + for _, n := range notNums { + if isNum(n) { + t.Fatalf("Input %v should not be a num, but was identified as one.", n) + } + } +} + +func TestQuote(t *testing.T) { + SetupForTesting() + tests := []struct { + in string + expectQuotes bool + }{ + {"foo bar baz", true}, + {"", true}, + {"\"foo bar\"", false}, + {"'foo bar'", false}, + {"123", false}, + {"abc:def", true}, // Not recognized as a key value pair without space after colon + {"abc: def", false}, + {"abc:", false}, + } + for _, test := range tests { + out := quote(test.in) + quoted := "\"" + test.in + "\"" + if test.expectQuotes && out != "\""+test.in+"\"" { + t.Fatalf("Expected %v, got %v", quoted, out) + } + if !test.expectQuotes && test.in != out { + t.Fatalf("Expected %v, got %v", test.in, out) + } + } +} + +func TestIndentBase(t *testing.T) { + SetupForTesting() + tests := []struct { + input []string + indentation int + prefix string + indentFirstLine bool + expected string + }{ + { + input: []string{"foo", "bar", "baz"}, + indentation: 2, + prefix: "", + indentFirstLine: false, + expected: fmt.Sprintf("%q\n %q\n %q\n", "foo", "bar", "baz"), + }, + { + input: []string{"foo", "bar", "baz"}, + indentation: 0, + prefix: "", + indentFirstLine: false, + expected: fmt.Sprintf("%q\n%q\n%q\n", "foo", "bar", "baz"), + }, + { + input: []string{"foo", "bar", "baz"}, + indentation: 2, + prefix: "", + indentFirstLine: true, + expected: fmt.Sprintf(" %q\n %q\n %q\n", "foo", "bar", "baz"), + }, + { + input: []string{"foo", "bar", "baz"}, + indentation: 2, + prefix: "__", + indentFirstLine: false, + expected: fmt.Sprintf("__%q\n __%q\n __%q\n", "foo", "bar", "baz"), + }, + } + for _, test := range tests { + out := indentBase( + test.indentation, + test.prefix, + test.indentFirstLine, + test.input) + if diff := cmp.Diff(out, test.expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } + } +} + +func TestIndentArray(t *testing.T) { + SetupForTesting() + input := []string{"'foo'", "42", "key: value", "bar"} + indentation := 2 + expected := "- 'foo'\n - 42\n - key: value\n - \"bar\"\n" + + if diff := cmp.Diff(indentArray(indentation, input), expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } +} + +func TestIndentKeys(t *testing.T) { + SetupForTesting() + input := []string{"abc: def", "foo: bar"} + indentation := 2 + expected := "abc: def\n foo: bar\n" + + if diff := cmp.Diff(indentKeys(indentation, input), expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } +} + +func TestIndentSectionBase(t *testing.T) { + SetupForTesting() + indentation := 2 + title := "foo" + prefix := "__" + input := []string{"abc: def", "bar", "42"} + expected := "foo:\n __abc: def\n __\"bar\"\n __42\n" + + out := indentSectionBase(indentation, title, prefix, input) + if diff := cmp.Diff(out, expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } + + out = indentSectionBase(indentation, title, prefix, []string{}) + if diff := cmp.Diff(out, ""); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } +} + +func TestIndentArraySection(t *testing.T) { + SetupForTesting() + indentation := 2 + title := "foo" + input := []string{"abc: def", "bar", "42"} + expected := "foo:\n - abc: def\n - \"bar\"\n - 42\n" + + out := indentArraySection(indentation, title, input) + if diff := cmp.Diff(out, expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } + + out = indentArraySection(indentation, title, []string{}) + if diff := cmp.Diff(out, ""); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } +} + +func TestIndentSection(t *testing.T) { + SetupForTesting() + indentation := 2 + title := "foo" + input := []string{"abc: def", "bar: baz", "magic_num: 42"} + expected := "foo:\n abc: def\n bar: baz\n magic_num: 42\n" + + out := indentSection(indentation, title, input) + if diff := cmp.Diff(out, expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } + + out = indentSection(indentation, title, []string{}) + if diff := cmp.Diff(out, ""); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } +} + +func TestIndentMap(t *testing.T) { + SetupForTesting() + indentation := 2 + input := map[string]string{ + "foo": "bar", + "abc": "def", + "num": "42", + } + expected := "abc: \"def\"\n foo: \"bar\"\n num: 42\n" + + out := indentMap(indentation, input) + if diff := cmp.Diff(out, expected); diff != "" { + t.Fatalf("Unexpected output (-got +want):\n%s", diff) + } +} + +func TestStrExists(t *testing.T) { + SetupForTesting() + sArray := []string{"foo", "bar", "baz"} + + if strExists(sArray, "abc") { + t.Fatalf("String abc should not exist in %v", sArray) + } + + if !strExists(sArray, "bar") { + t.Fatalf("String bar should exist in %v", sArray) + } +}