From 26878fc9f4af057cb5a1090d845b8b9685f5dc84 Mon Sep 17 00:00:00 2001 From: Martin Ostrowski Date: Fri, 12 Jul 2019 14:17:59 -0700 Subject: [PATCH] Cleanup and unit tests for util package (#73) * Cleanup and unit tests for util package * Review comments --- pkg/util/common.go | 14 ----- pkg/util/path.go | 13 +++-- pkg/util/reflect.go | 20 +------ pkg/util/reflect_test.go | 81 ++++++++++++++++++++++++++++ pkg/validate/validate_values.go | 11 ++-- pkg/validate/validate_values_test.go | 2 +- 6 files changed, 94 insertions(+), 47 deletions(-) diff --git a/pkg/util/common.go b/pkg/util/common.go index a5c2c187b9a..165cf8997a0 100644 --- a/pkg/util/common.go +++ b/pkg/util/common.go @@ -16,8 +16,6 @@ package util import ( "strings" - - "gopkg.in/yaml.v2" ) const ( @@ -25,18 +23,6 @@ const ( LocalFilePrefix = "file:///" ) -// Tree is a tree. -type Tree map[string]interface{} - -// String implements the Stringer interface method. -func (t Tree) String() string { - y, err := yaml.Marshal(t) - if err != nil { - return err.Error() - } - return string(y) -} - // IsFilePath reports whether the given URL is a local file path. func IsFilePath(path string) bool { return strings.HasPrefix(path, LocalFilePrefix) diff --git a/pkg/util/path.go b/pkg/util/path.go index 97c41c42af2..3a4a463685d 100644 --- a/pkg/util/path.go +++ b/pkg/util/path.go @@ -22,12 +22,11 @@ import ( ) const ( - kvSeparatorRune = ':' - // PathSeparator is the separator between path elements. PathSeparator = "." // KVSeparator is the separator between the key and value in a key/value path element, - KVSeparator = string(kvSeparatorRune) + KVSeparator = string(kvSeparatorRune) + kvSeparatorRune = ':' ) var ( @@ -67,10 +66,6 @@ func ToYAMLPathString(path string) string { return p.String() } -func firstCharToLowerCase(s string) string { - return strings.ToLower(s[0:1]) + s[1:] -} - // IsValidPathElement reports whether pe is a valid path element. func IsValidPathElement(pe string) bool { return ValidKeyRegex.MatchString(pe) @@ -148,3 +143,7 @@ func splitEscaped(s string, r rune) []string { out = append(out, s[prevIdx:]) return out } + +func firstCharToLowerCase(s string) string { + return strings.ToLower(s[0:1]) + s[1:] +} diff --git a/pkg/util/reflect.go b/pkg/util/reflect.go index 1ede805d5b8..fbe08d2e67a 100644 --- a/pkg/util/reflect.go +++ b/pkg/util/reflect.go @@ -209,21 +209,6 @@ func IsEmptyString(value interface{}) bool { return false } -// AppendToSlicePtr inserts value into parent which must be a slice ptr. -func AppendToSlicePtr(parentSlice interface{}, value interface{}) error { - dbgPrint("AppendToSlicePtr slice=\n%s\nvalue=\n%v", pretty.Sprint(parentSlice), value) - pv := reflect.ValueOf(parentSlice) - v := reflect.ValueOf(value) - - if !IsSliceInterfacePtr(parentSlice) { - return fmt.Errorf("appendToSlicePtr parent type is %T, must be *[]interface{}", parentSlice) - } - - pv.Elem().Set(reflect.Append(pv.Elem(), v)) - - return nil -} - // DeleteFromSlicePtr deletes an entry at index from the parent, which must be a slice ptr. func DeleteFromSlicePtr(parentSlice interface{}, index int) error { dbgPrint("DeleteFromSlicePtr index=%d, slice=\n%s", index, pretty.Sprint(parentSlice)) @@ -238,8 +223,7 @@ func DeleteFromSlicePtr(parentSlice interface{}, index int) error { pvv = pvv.Elem() } - ns := reflect.AppendSlice(pvv.Slice(0, index), pvv.Slice(index+1, pvv.Len())) - pv.Elem().Set(ns) + pv.Elem().Set(reflect.AppendSlice(pvv.Slice(0, index), pvv.Slice(index+1, pvv.Len()))) return nil } @@ -291,7 +275,7 @@ func InsertIntoMap(parentMap interface{}, key interface{}, value interface{}) er // ToIntValue returns 0, false if val is not a number type, otherwise it returns the int value of val. func ToIntValue(val interface{}) (int, bool) { if IsValueNil(val) { - return 0, false + return 0, true } v := reflect.ValueOf(val) switch v.Kind() { diff --git a/pkg/util/reflect_test.go b/pkg/util/reflect_test.go index 2e47d452d69..d69df63603c 100644 --- a/pkg/util/reflect_test.go +++ b/pkg/util/reflect_test.go @@ -304,6 +304,42 @@ func isInListOfInterface(lv []interface{}, v interface{}) bool { return false } +func TestDeleteFromSlicePtr(t *testing.T) { + parentSlice := []int{42, 43, 44, 45} + var parentSliceI interface{} = parentSlice + if err := DeleteFromSlicePtr(&parentSliceI, 1); err != nil { + t.Fatalf("got error: %s, want error: nil", err) + } + wantSlice := []int{42, 44, 45} + if got, want := parentSliceI, wantSlice; !reflect.DeepEqual(got, want) { + t.Errorf("got:\n%v\nwant:\n%v\n", got, want) + } + + badParent := struct{}{} + wantErr := `deleteFromSlicePtr parent type is *struct {}, must be *[]interface{}` + if got, want := errToString(DeleteFromSlicePtr(&badParent, 1)), wantErr; got != want { + t.Fatalf("got error: %s, want error: %s", got, want) + } +} + +func TestUpdateSlicePtr(t *testing.T) { + parentSlice := []int{42, 43, 44, 45} + var parentSliceI interface{} = parentSlice + if err := UpdateSlicePtr(&parentSliceI, 1, 42); err != nil { + t.Fatalf("got error: %s, want error: nil", err) + } + wantSlice := []int{42, 42, 44, 45} + if got, want := parentSliceI, wantSlice; !reflect.DeepEqual(got, want) { + t.Errorf("got:\n%v\nwant:\n%v\n", got, want) + } + + badParent := struct{}{} + wantErr := `updateSlicePtr parent type is *struct {}, must be *[]interface{}` + if got, want := errToString(UpdateSlicePtr(&badParent, 1, 42)), wantErr; got != want { + t.Fatalf("got error: %s, want error: %s", got, want) + } +} + func TestInsertIntoMap(t *testing.T) { parentMap := map[int]string{42: "forty two", 43: "forty three"} key := 44 @@ -322,3 +358,48 @@ func TestInsertIntoMap(t *testing.T) { t.Fatalf("got error: %s, want error: %s", got, want) } } + +func TestToIntValue(t *testing.T) { + tests := []struct { + desc string + in interface{} + wantOk bool + want int + }{ + { + desc: "nil success", + in: nil, + wantOk: true, + want: 0, + }, + { + desc: "int8 success", + in: int8(-42), + wantOk: true, + want: -42, + }, + { + desc: "uint32 success", + in: uint32(42), + wantOk: true, + want: 42, + }, + { + desc: "bed type fail", + in: "42", + wantOk: false, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + got, ok := ToIntValue(tt.in) + if ok != tt.wantOk { + t.Errorf("%s: gotOk %v, wantOk %v", tt.desc, ok, tt.wantOk) + } + if got != tt.want { + t.Errorf("%s: got %v, want %v", tt.desc, got, tt.want) + } + }) + } +} diff --git a/pkg/validate/validate_values.go b/pkg/validate/validate_values.go index 02858c08914..50c610744e0 100644 --- a/pkg/validate/validate_values.go +++ b/pkg/validate/validate_values.go @@ -29,7 +29,7 @@ var ( ) // CheckValues validates the values in the given tree, which follows the Istio values.yaml schema. -func CheckValues(root util.Tree) util.Errors { +func CheckValues(root map[string]interface{}) util.Errors { return validateValues(defaultValuesValidations, root, nil) } @@ -41,13 +41,10 @@ func validateValues(validations map[string]ValidatorFunc, node interface{}, path errs = util.AppendErrs(errs, vf(path, node)) } - nn, ok := node.(util.Tree) + nn, ok := node.(map[string]interface{}) if !ok { - nn, ok = node.(map[string]interface{}) - if !ok { - // Leaf, nothing more to recurse. - return - } + // Leaf, nothing more to recurse. + return errs } for k, v := range nn { errs = util.AppendErrs(errs, validateValues(validations, v, append(path, k))) diff --git a/pkg/validate/validate_values_test.go b/pkg/validate/validate_values_test.go index 206114f5468..ccaea62255c 100644 --- a/pkg/validate/validate_values_test.go +++ b/pkg/validate/validate_values_test.go @@ -98,7 +98,7 @@ global: for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - root := util.Tree{} + root := make(map[string]interface{}) err := yaml.Unmarshal([]byte(tt.yamlStr), &root) if err != nil { t.Fatalf("yaml.Unmarshal(%s): got error %s", tt.desc, err)