ffyaml: interpret null/empty value as empty string instead of error (#83

) * Proposed change to YAML parser functionality, allowing empty values for each of the 6 supported golang datatypes ff lib (string, int, bool, duration, float, slice) and setting them to the 'zero value for that type; reference: #81 Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * cleanup & add new comments Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * format code Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * clarify new code comment Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * added new public function comment Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * Added new unit test for YAML parser Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * format test file Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * Addressed 2 PR comments: formatting fixes Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * PR feedback: removed test function checkParseErr() -> pushed up code into test functions Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * New Updated YAML parser approach to handling blank (empty) YAML file entries -> Always assume that a blank node value is an empty string, and return that as the value; Also assocated Unit Test changes Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * fixed imports Signed-off-by: Aram Openden <Aram.Openden1@ibm.com> * Remove many changes * Test empty YAML nodes for various flag types * Fix tests * Refactor var comparison to t.Error Co-authored-by: Aram Openden <Aram.Openden1@ibm.com>
peterbourgon · Jul 7, 2021 · 17a8312 · 17a8312
1 parent 556df77
commit 17a8312
Show file tree

Hide file tree

Showing 9 changed files with 91 additions and 56 deletions.
diff --git a/ffcli/command_test.go b/ffcli/command_test.go
@@ -133,13 +133,13 @@ func TestCommandRun(t *testing.T) {
 
 			err := root.ParseAndRun(context.Background(), testcase.args)
 			assertNoError(t, err)
-			assertNoError(t, fftest.Compare(&testcase.rootvars, rootvars))
+			fftest.Compare(t, &testcase.rootvars, rootvars)
 			assertBool(t, testcase.rootran, rootran)
 			assertStringSlice(t, testcase.rootargs, rootargs)
-			assertNoError(t, fftest.Compare(&testcase.foovars, foovars))
+			fftest.Compare(t, &testcase.foovars, foovars)
 			assertBool(t, testcase.fooran, fooran)
 			assertStringSlice(t, testcase.fooargs, fooargs)
-			assertNoError(t, fftest.Compare(&testcase.barvars, barvars))
+			fftest.Compare(t, &testcase.barvars, barvars)
 			assertBool(t, testcase.barran, barran)
 			assertStringSlice(t, testcase.barargs, barargs)
 		})

diff --git a/fftest/vars.go b/fftest/vars.go
@@ -3,28 +3,45 @@ package fftest
 import (
 	"errors"
 	"flag"
-	"fmt"
 	"reflect"
 	"strings"
+	"testing"
 	"time"
 )
 
-// Pair returns a predefined flag set, and a predefined set of variables that
-// have been registered into it. Tests can call parse on the flag set with a
-// variety of flags, config files, and env vars, and check the resulting effect
-// on the vars.
+// Pair defines and returns an empty flag set and vars assigned to it.
 func Pair() (*flag.FlagSet, *Vars) {
 	fs := flag.NewFlagSet("fftest", flag.ContinueOnError)
+	vars := DefaultVars(fs)
+	return fs, vars
+}
 
+// DefaultVars registers a predefined set of variables to the flag set.
+// Tests can call parse on the flag set with a variety of flags, config files,
+// and env vars, and check the resulting effect on the vars.
+func DefaultVars(fs *flag.FlagSet) *Vars {
 	var v Vars
 	fs.StringVar(&v.S, "s", "", "string")
 	fs.IntVar(&v.I, "i", 0, "int")
 	fs.Float64Var(&v.F, "f", 0., "float64")
 	fs.BoolVar(&v.B, "b", false, "bool")
 	fs.DurationVar(&v.D, "d", 0*time.Second, "time.Duration")
 	fs.Var(&v.X, "x", "collection of strings (repeatable)")
+	return &v
+}
 
-	return fs, &v
+// NonzeroDefaultVars is like DefaultVars, but provides each primitive flag with
+// a nonzero default value. This is useful for tests that explicitly provide a
+// zero value for the type.
+func NonzeroDefaultVars(fs *flag.FlagSet) *Vars {
+	var v Vars
+	fs.StringVar(&v.S, "s", "foo", "string")
+	fs.IntVar(&v.I, "i", 123, "int")
+	fs.Float64Var(&v.F, "f", 9.99, "float64")
+	fs.BoolVar(&v.B, "b", true, "bool")
+	fs.DurationVar(&v.D, "d", 3*time.Hour, "time.Duration")
+	fs.Var(&v.X, "x", "collection of strings (repeatable)")
+	return &v
 }
 
 // Vars are a common set of variables used for testing.
@@ -51,52 +68,51 @@ type Vars struct {
 }
 
 // Compare one set of vars with another
-// and return an error on any difference.
-func Compare(want, have *Vars) error {
+// and t.Error on any difference.
+func Compare(t *testing.T, want, have *Vars) {
+	t.Helper()
+
 	if want.WantParseErrorIs != nil || want.WantParseErrorString != "" {
 		if want.WantParseErrorIs != nil && have.ParseError == nil {
-			return fmt.Errorf("want error (%v), have none", want.WantParseErrorIs)
+			t.Errorf("want error (%v), have none", want.WantParseErrorIs)
 		}
-
 		if want.WantParseErrorString != "" && have.ParseError == nil {
-			return fmt.Errorf("want error (%q), have none", want.WantParseErrorString)
+			t.Errorf("want error (%q), have none", want.WantParseErrorString)
 		}
-
 		if want.WantParseErrorIs == nil && want.WantParseErrorString == "" && have.ParseError != nil {
-			return fmt.Errorf("want clean parse, have error (%v)", have.ParseError)
+			t.Errorf("want clean parse, have error (%v)", have.ParseError)
 		}
-
 		if want.WantParseErrorIs != nil && have.ParseError != nil && !errors.Is(have.ParseError, want.WantParseErrorIs) {
-			return fmt.Errorf("want wrapped error (%#+v), have error (%#+v)", want.WantParseErrorIs, have.ParseError)
+			t.Errorf("want wrapped error (%#+v), have error (%#+v)", want.WantParseErrorIs, have.ParseError)
 		}
-
 		if want.WantParseErrorString != "" && have.ParseError != nil && !strings.Contains(have.ParseError.Error(), want.WantParseErrorString) {
-			return fmt.Errorf("want error string (%q), have error (%v)", want.WantParseErrorString, have.ParseError)
+			t.Errorf("want error string (%q), have error (%v)", want.WantParseErrorString, have.ParseError)
 		}
+		return
+	}
 
-		return nil
+	if have.ParseError != nil {
+		t.Errorf("error: %v", have.ParseError)
 	}
 
 	if want.S != have.S {
-		return fmt.Errorf("var S: want %q, have %q", want.S, have.S)
+		t.Errorf("var S: want %q, have %q", want.S, have.S)
 	}
 	if want.I != have.I {
-		return fmt.Errorf("var I: want %d, have %d", want.I, have.I)
+		t.Errorf("var I: want %d, have %d", want.I, have.I)
 	}
 	if want.F != have.F {
-		return fmt.Errorf("var F: want %f, have %f", want.F, have.F)
+		t.Errorf("var F: want %f, have %f", want.F, have.F)
 	}
 	if want.B != have.B {
-		return fmt.Errorf("var B: want %v, have %v", want.B, have.B)
+		t.Errorf("var B: want %v, have %v", want.B, have.B)
 	}
 	if want.D != have.D {
-		return fmt.Errorf("var D: want %s, have %s", want.D, have.D)
+		t.Errorf("var D: want %s, have %s", want.D, have.D)
 	}
 	if !reflect.DeepEqual(want.X, have.X) {
-		return fmt.Errorf("var X: want %v, have %v", want.X, have.X)
+		t.Errorf("var X: want %v, have %v", want.X, have.X)
 	}
-
-	return nil
 }
 
 // StringSlice is a flag.Value that collects each Set string

diff --git a/fftoml/fftoml_test.go b/fftoml/fftoml_test.go
@@ -48,9 +48,7 @@ func TestParser(t *testing.T) {
 				ff.WithConfigFile(testcase.file),
 				ff.WithConfigFileParser(fftoml.Parser),
 			)
-			if err := fftest.Compare(&testcase.want, vars); err != nil {
-				t.Fatal(err)
-			}
+			fftest.Compare(t, &testcase.want, vars)
 		})
 	}
 }

diff --git a/ffyaml/ffyaml.go b/ffyaml/ffyaml.go
@@ -1,4 +1,4 @@
-// Package ffyaml provides a YAML config file paser.
+// Package ffyaml provides a YAML config file parser.
 package ffyaml
 
 import (
@@ -68,6 +68,8 @@ func valToStr(val interface{}) (string, error) {
 		return strconv.FormatInt(v, 10), nil
 	case float64:
 		return strconv.FormatFloat(v, 'g', -1, 64), nil
+	case nil:
+		return "", nil
 	default:
 		return "", ff.StringConversionError{Value: val}
 	}

diff --git a/ffyaml/ffyaml_test.go b/ffyaml/ffyaml_test.go
@@ -1,6 +1,8 @@
 package ffyaml_test
 
 import (
+	"flag"
+	"os"
 	"testing"
 	"time"
 
@@ -13,13 +15,15 @@ func TestParser(t *testing.T) {
 	t.Parallel()
 
 	for _, testcase := range []struct {
+		vars func(*flag.FlagSet) *fftest.Vars
 		name string
 		file string
+		miss bool // AllowMissingConfigFiles
 		want fftest.Vars
 	}{
 		{
-			name: "empty input",
-			file: "testdata/empty_input.yaml",
+			name: "empty",
+			file: "testdata/empty.yaml",
 			want: fftest.Vars{},
 		},
 		{
@@ -33,9 +37,16 @@ func TestParser(t *testing.T) {
 			want: fftest.Vars{WantParseErrorString: "found character that cannot start any token"},
 		},
 		{
-			name: "no value",
-			file: "testdata/no_value.yaml",
-			want: fftest.Vars{WantParseErrorIs: ff.StringConversionError{}},
+			vars: fftest.NonzeroDefaultVars,
+			name: "no value for s",
+			file: "testdata/no_value_s.yaml",
+			want: fftest.Vars{S: "", I: 123, F: 9.99, B: true, D: 3 * time.Hour},
+		},
+		{
+			vars: fftest.NonzeroDefaultVars,
+			name: "no value for i",
+			file: "testdata/no_value_i.yaml",
+			want: fftest.Vars{WantParseErrorString: "parse error"},
 		},
 		{
 			name: "basic arrays",
@@ -57,17 +68,31 @@ func TestParser(t *testing.T) {
 			file: "testdata/unquoted_string_array.yaml",
 			want: fftest.Vars{X: []string{"one", "two", "three"}},
 		},
+		{
+			name: "missing config file allowed",
+			file: "testdata/this_file_does_not_exist.yaml",
+			miss: true,
+			want: fftest.Vars{},
+		},
+		{
+			name: "missing config file not allowed",
+			file: "testdata/this_file_does_not_exist.yaml",
+			miss: false,
+			want: fftest.Vars{WantParseErrorIs: os.ErrNotExist},
+		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
-			fs, vars := fftest.Pair()
+			if testcase.vars == nil {
+				testcase.vars = fftest.DefaultVars
+			}
+			fs := flag.NewFlagSet("fftest", flag.ContinueOnError)
+			vars := testcase.vars(fs)
 			vars.ParseError = ff.Parse(fs, []string{},
 				ff.WithConfigFile(testcase.file),
 				ff.WithConfigFileParser(ffyaml.Parser),
-				ff.WithAllowMissingConfigFile(true),
+				ff.WithAllowMissingConfigFile(testcase.miss),
 			)
-			if err := fftest.Compare(&testcase.want, vars); err != nil {
-				t.Fatal(err)
-			}
+			fftest.Compare(t, &testcase.want, vars)
 		})
 	}
 }
diff --git a/ffyaml/testdata/no_value_i.yaml b/ffyaml/testdata/no_value_i.yaml
@@ -0,0 +1,2 @@
+s: woozlewozzle
+i: 
diff --git a/ffyaml/testdata/no_value.yaml → ffyaml/testdata/no_value_s.yaml b/ffyaml/testdata/no_value.yaml → ffyaml/testdata/no_value_s.yaml
@@ -1,2 +1,2 @@
-i: 123
 s:
+i: 123
diff --git a/json_test.go b/json_test.go
@@ -49,9 +49,7 @@ func TestJSONParser(t *testing.T) {
 				ff.WithConfigFile(testcase.file),
 				ff.WithConfigFileParser(ff.JSONParser),
 			)
-			if err := fftest.Compare(&testcase.want, vars); err != nil {
-				t.Fatal(err)
-			}
+			fftest.Compare(t, &testcase.want, vars)
 		})
 	}
 }
diff --git a/parse_test.go b/parse_test.go
@@ -152,9 +152,7 @@ func TestParseBasics(t *testing.T) {
 
 			fs, vars := fftest.Pair()
 			vars.ParseError = ff.Parse(fs, testcase.args, testcase.opts...)
-			if err := fftest.Compare(&testcase.want, vars); err != nil {
-				t.Fatal(err)
-			}
+			fftest.Compare(t, &testcase.want, vars)
 		})
 	}
 }
@@ -204,9 +202,7 @@ func TestParseIssue16(t *testing.T) {
 			)
 
 			want := fftest.Vars{S: testcase.want}
-			if err := fftest.Compare(&want, vars); err != nil {
-				t.Fatal(err)
-			}
+			fftest.Compare(t, &want, vars)
 		})
 	}
 }
@@ -251,9 +247,7 @@ func TestParseConfigFile(t *testing.T) {
 			vars.ParseError = ff.Parse(fs, []string{}, options...)
 
 			want := fftest.Vars{WantParseErrorIs: testcase.parseError}
-			if err := fftest.Compare(&want, vars); err != nil {
-				t.Fatal(err)
-			}
+			fftest.Compare(t, &want, vars)
 		})
 	}
 }