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 <[email protected]>

* 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 <[email protected]>

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/<name>/service-account.json\"", data.ServiceAccount)
+		logFatalf("Service account path %q is expected to be \"/etc/<name>/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 {

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)

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.

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
+}

tools/config-generator/udpaterelease.go

@@ -208,15 +208,15 @@ 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
 }
 
 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])
 }

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
 }