Repository: hamba/avro
Pull request #305 (2023-09-06 21:23:24)

chore: codebase cleanup for avrogen and avrosv
This PR mainly resolves technical debt left over in avrosv after the improvements in avrogen.

Two chore commits are included:
* chore: rename and rearrange `io.Writer` arguments in avrogen
* chore: improve code and tests in avrosv after avrogen

The first one is to cleanup the ugly `out`/`dumpout` argument names into clearer meanings and logical order.
The second one mostly brings up all the recent unit tests improvements from avrogen into avrosv.

I also added an extra bad schema test case to the avrosv stdout unit test.


Patch:
diff --git a/cmd/avrogen/main.go b/cmd/avrogen/main.go
index 243ef97..c89f2d9 100644
--- a/cmd/avrogen/main.go
+++ b/cmd/avrogen/main.go
@@ -24,13 +24,13 @@ type config struct {
 }
 
 func main() {
-	os.Exit(realMain(os.Args, os.Stderr, os.Stdout))
+	os.Exit(realMain(os.Args, os.Stdout, os.Stderr))
 }
 
-func realMain(args []string, out, dumpout io.Writer) int {
+func realMain(args []string, stdout, stderr io.Writer) int {
 	var cfg config
 	flgs := flag.NewFlagSet("avrogen", flag.ExitOnError)
-	flgs.SetOutput(out)
+	flgs.SetOutput(stderr)
 	flgs.StringVar(&cfg.Pkg, "pkg", "", "The package name of the output file.")
 	flgs.StringVar(&cfg.Out, "o", "", "The output file path to write to instead of stdout.")
 	flgs.StringVar(&cfg.Tags, "tags", "", "The additional field tags <tag-name>:{snake|camel|upper-camel|kebab}>[,...]")
@@ -38,8 +38,8 @@ func realMain(args []string, out, dumpout io.Writer) int {
 	flgs.BoolVar(&cfg.Encoders, "encoders", false, "Generate encoders for the structs.")
 	flgs.StringVar(&cfg.Initialisms, "initialisms", "", "Custom initialisms <VAL>[,...] for struct and field names.")
 	flgs.Usage = func() {
-		_, _ = fmt.Fprintln(out, "Usage: avrogen [options] schemas")
-		_, _ = fmt.Fprintln(out, "Options:")
+		_, _ = fmt.Fprintln(stderr, "Usage: avrogen [options] schemas")
+		_, _ = fmt.Fprintln(stderr, "Options:")
 		flgs.PrintDefaults()
 	}
 	if err := flgs.Parse(args[1:]); err != nil {
@@ -47,19 +47,19 @@ func realMain(args []string, out, dumpout io.Writer) int {
 	}
 
 	if err := validateOpts(flgs.NArg(), cfg); err != nil {
-		_, _ = fmt.Fprintln(out, "Error: "+err.Error())
+		_, _ = fmt.Fprintln(stderr, "Error: "+err.Error())
 		return 1
 	}
 
 	tags, err := parseTags(cfg.Tags)
 	if err != nil {
-		_, _ = fmt.Fprintln(out, "Error: "+err.Error())
+		_, _ = fmt.Fprintln(stderr, "Error: "+err.Error())
 		return 1
 	}
 
 	initialisms, err := parseInitialisms(cfg.Initialisms)
 	if err != nil {
-		_, _ = fmt.Fprintln(out, "Error: "+err.Error())
+		_, _ = fmt.Fprintln(stderr, "Error: "+err.Error())
 		return 1
 	}
 
@@ -72,7 +72,7 @@ func realMain(args []string, out, dumpout io.Writer) int {
 	for _, file := range flgs.Args() {
 		schema, err := avro.ParseFiles(filepath.Clean(file))
 		if err != nil {
-			_, _ = fmt.Fprintf(out, "Error: %v\n", err)
+			_, _ = fmt.Fprintf(stderr, "Error: %v\n", err)
 			return 2
 		}
 		g.Parse(schema)
@@ -80,20 +80,20 @@ func realMain(args []string, out, dumpout io.Writer) int {
 
 	var buf bytes.Buffer
 	if err = g.Write(&buf); err != nil {
-		_, _ = fmt.Fprintf(out, "Error: could not generate code: %v\n", err)
+		_, _ = fmt.Fprintf(stderr, "Error: could not generate code: %v\n", err)
 		return 3
 	}
 	formatted, err := format.Source(buf.Bytes())
 	if err != nil {
-		_, _ = fmt.Fprintf(out, "Error: could not format code: %v\n", err)
+		_, _ = fmt.Fprintf(stderr, "Error: could not format code: %v\n", err)
 		return 3
 	}
 
-	writer := dumpout
+	writer := stdout
 	if cfg.Out != "" {
 		file, err := os.Create(cfg.Out)
 		if err != nil {
-			_, _ = fmt.Fprintf(out, "Error: could not create output file: %v\n", err)
+			_, _ = fmt.Fprintf(stderr, "Error: could not create output file: %v\n", err)
 			return 4
 		}
 		defer func() { _ = file.Close() }()
@@ -102,7 +102,7 @@ func realMain(args []string, out, dumpout io.Writer) int {
 	}
 
 	if _, err := writer.Write(formatted); err != nil {
-		_, _ = fmt.Fprintf(out, "Error: could not write code: %v\n", err)
+		_, _ = fmt.Fprintf(stderr, "Error: could not write code: %v\n", err)
 		return 4
 	}
 
diff --git a/cmd/avrosv/main.go b/cmd/avrosv/main.go
index 26f737b..ec6964a 100644
--- a/cmd/avrosv/main.go
+++ b/cmd/avrosv/main.go
@@ -14,36 +14,36 @@ type config struct {
 }
 
 func main() {
-	os.Exit(realMain(os.Args, os.Stderr, os.Stdout))
+	os.Exit(realMain(os.Args, os.Stdout, os.Stderr))
 }
 
-func realMain(args []string, out, dumpout io.Writer) int {
+func realMain(args []string, stdout, stderr io.Writer) int {
 	var cfg config
 	flgs := flag.NewFlagSet("avrosv", flag.ExitOnError)
-	flgs.SetOutput(out)
+	flgs.SetOutput(stderr)
 	flgs.BoolVar(&cfg.Verbose, "v", false, "Verbose output (dump final parsed schema).")
 	flgs.Usage = func() {
-		_, _ = fmt.Fprintln(out, "Usage: avrosv [options] schemas")
-		_, _ = fmt.Fprintln(out, "Options:")
+		_, _ = fmt.Fprintln(stderr, "Usage: avrosv [options] schemas")
+		_, _ = fmt.Fprintln(stderr, "Options:")
 		flgs.PrintDefaults()
-		_, _ = fmt.Fprintln(out, "\nSchemas are processed in the order they appear.")
+		_, _ = fmt.Fprintln(stderr, "\nSchemas are processed in the order they appear.")
 	}
 	if err := flgs.Parse(args[1:]); err != nil {
 		return 1
 	}
 	if flgs.NArg() < 1 {
-		_, _ = fmt.Fprintln(out, "Error: at least one schema is required")
+		_, _ = fmt.Fprintln(stderr, "Error: at least one schema is required")
 		return 1
 	}
 
 	schema, err := avro.ParseFiles(flgs.Args()...)
 	if err != nil {
-		_, _ = fmt.Fprintf(out, "Error: %v\n", err)
+		_, _ = fmt.Fprintf(stderr, "Error: %v\n", err)
 		return 2
 	}
 
 	if cfg.Verbose {
-		fmt.Fprintln(dumpout, schema)
+		fmt.Fprintln(stdout, schema)
 	}
 
 	return 0


Test patch:
diff --git a/cmd/avrogen/main_test.go b/cmd/avrogen/main_test.go
index 1317122..97eb97d 100644
--- a/cmd/avrogen/main_test.go
+++ b/cmd/avrogen/main_test.go
@@ -66,7 +66,7 @@ func TestAvroGen_GeneratesSchemaStdout(t *testing.T) {
 	var buf bytes.Buffer
 
 	args := []string{"avrogen", "-pkg", "testpkg", "testdata/schema.avsc"}
-	gotCode := realMain(args, io.Discard, &buf)
+	gotCode := realMain(args, &buf, io.Discard)
 	require.Equal(t, 0, gotCode)
 
 	want, err := os.ReadFile("testdata/golden.go")
diff --git a/cmd/avrosv/main_test.go b/cmd/avrosv/main_test.go
index 7b47cef..3d85a06 100644
--- a/cmd/avrosv/main_test.go
+++ b/cmd/avrosv/main_test.go
@@ -11,24 +11,24 @@ import (
 
 func TestAvroSv_RequiredFlags(t *testing.T) {
 	tests := []struct {
-		name    string
-		args    []string
-		wantErr bool
+		name         string
+		args         []string
+		wantExitCode int
 	}{
 		{
-			name:    "validates no schema is set",
-			args:    []string{"avrosv"},
-			wantErr: true,
+			name:         "validates no schema is set",
+			args:         []string{"avrosv"},
+			wantExitCode: 1,
 		},
 		{
-			name:    "validates single schema is set",
-			args:    []string{"avrosv", "some/file"},
-			wantErr: true,
+			name:         "validates single schema is set",
+			args:         []string{"avrosv", "some/file"},
+			wantExitCode: 2,
 		},
 		{
-			name:    "validates multiple schemas are set",
-			args:    []string{"avrosv", "some/file", "some/other"},
-			wantErr: true,
+			name:         "validates multiple schemas are set",
+			args:         []string{"avrosv", "some/file", "some/other"},
+			wantExitCode: 2,
 		},
 	}
 
@@ -37,46 +37,41 @@ func TestAvroSv_RequiredFlags(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			got := realMain(test.args, io.Discard, io.Discard)
 
-			if !test.wantErr {
-				assert.Equal(t, 0, got)
-				return
-			}
-
-			assert.NotEqual(t, 0, got)
+			assert.Equal(t, test.wantExitCode, got)
 		})
 	}
 }
 
 func TestAvroSv_ValidatesSchema(t *testing.T) {
 	tests := []struct {
-		name    string
-		args    []string
-		wantErr bool
+		name         string
+		args         []string
+		wantExitCode int
 	}{
 		{
-			name:    "validates a simple schema",
-			args:    []string{"avrosv", "testdata/schema.avsc"},
-			wantErr: false,
+			name:         "validates a simple schema",
+			args:         []string{"avrosv", "testdata/schema.avsc"},
+			wantExitCode: 0,
 		},
 		{
-			name:    "does not validate a bad schema",
-			args:    []string{"avrosv", "testdata/bad-schema.avsc"},
-			wantErr: true,
+			name:         "does not validate a bad schema",
+			args:         []string{"avrosv", "testdata/bad-schema.avsc"},
+			wantExitCode: 2,
 		},
 		{
-			name:    "does not validate a schema with a bad default",
-			args:    []string{"avrosv", "testdata/bad-default-schema.avsc"},
-			wantErr: true,
+			name:         "does not validate a schema with a bad default",
+			args:         []string{"avrosv", "testdata/bad-default-schema.avsc"},
+			wantExitCode: 2,
 		},
 		{
-			name:    "does not validate a schema with a reference to a missing schema",
-			args:    []string{"avrosv", "testdata/withref-schema.avsc"},
-			wantErr: true,
+			name:         "does not validate a schema with a reference to a missing schema",
+			args:         []string{"avrosv", "testdata/withref-schema.avsc"},
+			wantExitCode: 2,
 		},
 		{
-			name:    "validates a schema with a reference to an existing schema",
-			args:    []string{"avrosv", "testdata/schema.avsc", "testdata/withref-schema.avsc"},
-			wantErr: false,
+			name:         "validates a schema with a reference to an existing schema",
+			args:         []string{"avrosv", "testdata/schema.avsc", "testdata/withref-schema.avsc"},
+			wantExitCode: 0,
 		},
 	}
 
@@ -86,45 +81,49 @@ func TestAvroSv_ValidatesSchema(t *testing.T) {
 			avro.DefaultSchemaCache = &avro.SchemaCache{} // reset the schema cache
 			got := realMain(test.args, io.Discard, io.Discard)
 
-			if !test.wantErr {
-				assert.Equal(t, 0, got)
-				return
-			}
-
-			assert.NotEqual(t, 0, got)
+			assert.Equal(t, test.wantExitCode, got)
 		})
 	}
 }
 
 func TestAvroSv_Verbose(t *testing.T) {
 	tests := []struct {
-		name    string
-		args    []string
-		out     string
-		wantErr bool
+		name         string
+		args         []string
+		wantStdout   string
+		wantExitCode int
 	}{
 		{
-			name:    "dumps a simple schema",
-			args:    []string{"avrosv", "-v", "testdata/schema.avsc"},
-			out:     "{\"name\":\"test\",\"type\":\"record\",\"fields\":[{\"name\":\"someString\",\"type\":\"string\"}]}\n",
-			wantErr: false,
+			name:         "dumps a simple schema",
+			args:         []string{"avrosv", "-v", "testdata/schema.avsc"},
+			wantStdout:   "{\"name\":\"test\",\"type\":\"record\",\"fields\":[{\"name\":\"someString\",\"type\":\"string\"}]}\n",
+			wantExitCode: 0,
+		},
+		{
+			name:         "dumps a schema with a reference to an existing schema",
+			args:         []string{"avrosv", "-v", "testdata/schema.avsc", "testdata/withref-schema.avsc"},
+			wantStdout:   "{\"name\":\"testref\",\"type\":\"record\",\"fields\":[{\"name\":\"someref\",\"type\":{\"name\":\"test\",\"type\":\"record\",\"fields\":[{\"name\":\"someString\",\"type\":\"string\"}]}}]}\n",
+			wantExitCode: 0,
 		},
 		{
-			name:    "dumps a schema with a reference to an existing schema",
-			args:    []string{"avrosv", "-v", "testdata/schema.avsc", "testdata/withref-schema.avsc"},
-			out:     "{\"name\":\"testref\",\"type\":\"record\",\"fields\":[{\"name\":\"someref\",\"type\":{\"name\":\"test\",\"type\":\"record\",\"fields\":[{\"name\":\"someString\",\"type\":\"string\"}]}}]}\n",
-			wantErr: false,
+			name:         "does not dump any schema when the schema file is invalid",
+			args:         []string{"avrosv", "-v", "testdata/bad-schema.avsc"},
+			wantStdout:   "",
+			wantExitCode: 2,
 		},
 	}
 
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
+			var buf bytes.Buffer
+
 			avro.DefaultSchemaCache = &avro.SchemaCache{} // reset the schema cache
-			w := &bytes.Buffer{}
-			got := realMain(test.args, io.Discard, w)
-			assert.Equal(t, 0, got)
-			assert.Equal(t, test.out, w.String())
+
+			got := realMain(test.args, &buf, io.Discard)
+
+			assert.Equal(t, test.wantStdout, buf.String())
+			assert.Equal(t, test.wantExitCode, got)
 		})
 	}
 }