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

* cleanup & add new comments

Signed-off-by: Aram Openden <[email protected]>

* format code

Signed-off-by: Aram Openden <[email protected]>

* clarify new code comment

Signed-off-by: Aram Openden <[email protected]>

* added new public function comment

Signed-off-by: Aram Openden <[email protected]>

* Added new unit test for YAML parser

Signed-off-by: Aram Openden <[email protected]>

* format test file

Signed-off-by: Aram Openden <[email protected]>

* Addressed 2 PR comments: formatting fixes

Signed-off-by: Aram Openden <[email protected]>

* PR feedback: removed test function checkParseErr() -> pushed up code into test functions

Signed-off-by: Aram Openden <[email protected]>

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

* fixed imports

Signed-off-by: Aram Openden <[email protected]>

* Remove many changes

* Test empty YAML nodes for various flag types

* Fix tests

* Refactor var comparison to t.Error

Co-authored-by: Aram Openden <[email protected]>

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

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

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

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

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

ffyaml/testdata/no_value_i.yaml

@@ -0,0 +1,2 @@
+s: woozlewozzle
+i: 

ffyaml/testdata/no_value.yaml → ffyaml/testdata/no_value_s.yaml

@@ -1,2 +1,2 @@
-i: 123
 s:
+i: 123

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

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