From 599b03af615c5c2d8dccd899f77e858fa3e0f927 Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Wed, 22 Jan 2025 08:42:10 +0100 Subject: [PATCH 1/7] feat(gogenerate): add optional fields in log with omitempty annotation Signed-off-by: T M Rezoan Tamal --- pkg/events/events.go | 2 +- tools/logger/logger.go | 29 +++++++++++++ tools/logger/logger_test.go | 82 +++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 tools/logger/logger_test.go diff --git a/pkg/events/events.go b/pkg/events/events.go index 92252a56..56d03c63 100644 --- a/pkg/events/events.go +++ b/pkg/events/events.go @@ -84,7 +84,7 @@ type HTTPRequest struct { Endpoint string `log:"http.url_details.endpoint"` // Route is the URL path without interpolating the path variables. - Route string `log:"http.route"` + Route string `log:"http.route,omitempty"` } // FillFieldsFromRequest fills in the standard request fields diff --git a/tools/logger/logger.go b/tools/logger/logger.go index d865cd25..a84884cb 100644 --- a/tools/logger/logger.go +++ b/tools/logger/logger.go @@ -54,6 +54,10 @@ if s.{{.name}} != nil { }` ) +const ( + tagOmitEmpty = ",omitempty" +) + func main() { flag.Usage = usage @@ -140,6 +144,9 @@ func processStruct(w io.Writer, s *types.Struct, name string) { write(w, nestedNilableMarshalerFormat, args) case field == ".": write(w, nestedMarshalerFormat, args) + case strings.HasSuffix(field, tagOmitEmpty): + args["key"] = strings.TrimSuffix(field, tagOmitEmpty) + write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args) default: write(w, simpleFieldFormat, args) } @@ -168,3 +175,25 @@ func usage() { fmt.Fprintf(os.Stderr, "Flags:\n") flag.PrintDefaults() } + +func getSimpleOptionalFieldFormat(p types.Type) string { + var defaultValue string + switch p.Underlying().String() { + case "string": + defaultValue = `""` + case "int", "int8", "int16", "int32", "int64", + "uint", "uint8", "uint16", "uint32", "uint64": + defaultValue = "0" + case "float32", "float64": + defaultValue = "0.0" + case "bool": + defaultValue = "false" + default: + defaultValue = "nil" + } + + return fmt.Sprintf(` +if s.{{.name}} != %s { + addField("{{.key}}", s.{{.name}}) +}`, defaultValue) +} diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go new file mode 100644 index 00000000..b3f52ad6 --- /dev/null +++ b/tools/logger/logger_test.go @@ -0,0 +1,82 @@ +package main + +import ( + "go/types" + "testing" +) + +func TestGetSimpleOptionalFieldFormat(t *testing.T) { + newNamedType := func(name string, underlying types.Type) types.Type { + return types.NewNamed( + types.NewTypeName(0, nil, name, nil), + underlying, + nil, + ) + } + + tests := []struct { + name string + typ types.Type + expected string + }{ + { + name: "string type", + typ: types.Typ[types.String], + expected: "\nif s.{{.name}} != \"\" {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "int type", + typ: types.Typ[types.Int], + expected: "\nif s.{{.name}} != 0 {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "float64 type", + typ: types.Typ[types.Float64], + expected: "\nif s.{{.name}} != 0.0 {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "bool type", + typ: types.Typ[types.Bool], + expected: "\nif s.{{.name}} != false {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "custom string type (type Token string)", + typ: newNamedType("Token", types.Typ[types.String]), + expected: "\nif s.{{.name}} != \"\" {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "custom int type (type Digits int)", + typ: newNamedType("Digits", types.Typ[types.Int]), + expected: "\nif s.{{.name}} != 0 {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "custom float type (type Decimal float64)", + typ: newNamedType("Decimal", types.Typ[types.Float64]), + expected: "\nif s.{{.name}} != 0.0 {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "custom bool type (type Truther bool)", + typ: newNamedType("Truther", types.Typ[types.Bool]), + expected: "\nif s.{{.name}} != false {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "pointer type", + typ: types.NewPointer(types.Typ[types.Int]), + expected: "\nif s.{{.name}} != nil {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + { + name: "pointer to custom type", + typ: types.NewPointer(newNamedType("Nullable", types.Typ[types.String])), + expected: "\nif s.{{.name}} != nil {\n\taddField(\"{{.key}}\", s.{{.name}})\n}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := getSimpleOptionalFieldFormat(tt.typ) + if got != tt.expected { + t.Errorf("getSimpleOptionalFieldFormat() =\n%v\nwant:\n%v", got, tt.expected) + } + }) + } +} From ffe256ff48069aaae958aad6e63f62a37bcf11f5 Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Thu, 23 Jan 2025 16:21:50 +0100 Subject: [PATCH 2/7] feat(test): improve anoonation code and add unit tests to cover processStruct() --- tools/logger/generating.md | 37 ++++++++++- tools/logger/logger.go | 20 +++--- tools/logger/logger_test.go | 120 ++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 8 deletions(-) diff --git a/tools/logger/generating.md b/tools/logger/generating.md index acf5e482..6b888995 100644 --- a/tools/logger/generating.md +++ b/tools/logger/generating.md @@ -27,7 +27,7 @@ type OrgInfo struct ``` -With the addition of the annotation (which matches the [standard +With the addition of the log tags `ie. log:"xyz"` for fields (xyz matches the [standard attributes](https://app.datadoghq.com/logs/pipelines/standard-attributes)), we can use [logger](https://github.com/getoutreach/gobox/tree/master/tools/logger) @@ -51,6 +51,41 @@ func (s *OrgInfo) MarshalLog(addField func(key string, value interface{})) { } ``` +### Annotations: +These tags for fields also have limited support for annotations: `ie. log:"xyz,annotation"`. + +Supported annotations: + +- **omitempty**: makes the field optional. + + `omitempty` annotation is available for simple built-in types or + custom types with underlying type being built-in or pointers of any type. + + Example: + ```go + type OrgInfo struct + Org string `log:"or.org.shortname,omitempty"` + Guid *Custom `log:"or.org.guid,omitempty"` + } + ``` + + Generated code: + + ```go + func (s *OrgInfo) MarshalLog(addField func(key string, value interface{})) { + if s == nil { + return + } + if s.Org != "" { + addField("or.org.shortname", s.Org) + } + if s.Guid != nil { + addField("or.org.guid", s.Guid) + } + } + ``` + + ## Using go generate Go `generate` is the standard way to generate pre-build artifacts diff --git a/tools/logger/logger.go b/tools/logger/logger.go index a84884cb..63e81649 100644 --- a/tools/logger/logger.go +++ b/tools/logger/logger.go @@ -46,6 +46,10 @@ func (s *{{ .name }}) MarshalLog(addField func(key string, value interface{})) { addField("{{.key}}", s.{{.name}}.UTC().Format(time.RFC3339Nano))` simpleFieldFormat = ` addField("{{.key}}", s.{{.name}})` + simpleOptionalFieldFormat = ` +if s.{{.name}} != %s { + addField("{{.key}}", s.{{.name}}) +}` nestedMarshalerFormat = ` s.{{.name}}.MarshalLog(addField)` nestedNilableMarshalerFormat = ` @@ -55,7 +59,7 @@ if s.{{.name}} != nil { ) const ( - tagOmitEmpty = ",omitempty" + annotationOmitEmpty = "omitempty" ) func main() { @@ -136,6 +140,12 @@ func processStruct(w io.Writer, s *types.Struct, name string) { write(w, functionHeaderFormat, map[string]string{"name": name}) for kk := 0; kk < s.NumFields(); kk++ { if field, ok := reflect.StructTag(s.Tag(kk)).Lookup("log"); ok { + var annotations string + fieldParts := strings.SplitN(field, ",", 2) + field = fieldParts[0] + if len(fieldParts) > 1 { + annotations = fieldParts[1] + } args := map[string]string{"key": field, "name": s.Field(kk).Name()} switch { case s.Field(kk).Type().String() == "time.Time": @@ -144,8 +154,7 @@ func processStruct(w io.Writer, s *types.Struct, name string) { write(w, nestedNilableMarshalerFormat, args) case field == ".": write(w, nestedMarshalerFormat, args) - case strings.HasSuffix(field, tagOmitEmpty): - args["key"] = strings.TrimSuffix(field, tagOmitEmpty) + case strings.Contains(annotations, annotationOmitEmpty): write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args) default: write(w, simpleFieldFormat, args) @@ -192,8 +201,5 @@ func getSimpleOptionalFieldFormat(p types.Type) string { defaultValue = "nil" } - return fmt.Sprintf(` -if s.{{.name}} != %s { - addField("{{.key}}", s.{{.name}}) -}`, defaultValue) + return fmt.Sprintf(simpleOptionalFieldFormat, defaultValue) } diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go index b3f52ad6..473f7400 100644 --- a/tools/logger/logger_test.go +++ b/tools/logger/logger_test.go @@ -1,7 +1,9 @@ package main import ( + "bytes" "go/types" + "gotest.tools/v3/assert" "testing" ) @@ -80,3 +82,121 @@ func TestGetSimpleOptionalFieldFormat(t *testing.T) { }) } } + +func TestProcessStruct(t *testing.T) { + tests := []struct { + name string + setup func() (*types.Struct, string) + expected string + }{ + { + name: "basic fields", + setup: func() (*types.Struct, string) { + fields := []*types.Var{ + types.NewField(0, nil, "Name", types.Typ[types.String], false), + types.NewField(0, nil, "Age", types.Typ[types.Int], false), + } + tags := []string{ + `log:"name"`, + `log:"age"`, + } + return types.NewStruct(fields, tags), "BasicStruct" + }, + expected: ` +func (s *BasicStruct) MarshalLog(addField func(key string, value interface{})) { + if s == nil { + return + } + +addField("name", s.Name) +addField("age", s.Age) +} +`, + }, + { + name: "time field", + setup: func() (*types.Struct, string) { + pkg := types.NewPackage("time", "time") + timeType := types.NewNamed(types.NewTypeName(0, pkg, "Time", nil), &types.Struct{}, nil) + + fields := []*types.Var{ + types.NewField(0, nil, "CreatedAt", timeType, false), + } + tags := []string{`log:"created_at"`} + return types.NewStruct(fields, tags), "TimeStruct" + }, + expected: ` +func (s *TimeStruct) MarshalLog(addField func(key string, value interface{})) { + if s == nil { + return + } + +addField("created_at", s.CreatedAt.UTC().Format(time.RFC3339Nano)) +} +`, + }, + { + name: "omitempty fields", + setup: func() (*types.Struct, string) { + fields := []*types.Var{ + types.NewField(0, nil, "Name", types.Typ[types.String], false), + types.NewField(0, nil, "AgeP", types.NewPointer(types.Typ[types.Int]), false), + } + tags := []string{ + `log:"name,omitempty"`, + `log:"ageP,omitempty"`, + } + return types.NewStruct(fields, tags), "OmitStruct" + }, + expected: ` +func (s *OmitStruct) MarshalLog(addField func(key string, value interface{})) { + if s == nil { + return + } + +if s.Name != "" { + addField("name", s.Name) +} +if s.AgeP != nil { + addField("ageP", s.AgeP) +} +} +`, + }, + { + name: "nested marshaler", + setup: func() (*types.Struct, string) { + nestedType := types.NewPointer(types.NewNamed( + types.NewTypeName(0, nil, "NestedStruct", nil), + &types.Struct{}, + nil, + )) + fields := []*types.Var{ + types.NewField(0, nil, "Nested", nestedType, false), + } + tags := []string{`log:"."`} + return types.NewStruct(fields, tags), "ParentStruct" + }, + expected: ` +func (s *ParentStruct) MarshalLog(addField func(key string, value interface{})) { + if s == nil { + return + } + +if s.Nested != nil { + s.Nested.MarshalLog(addField) +} +} +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, name := tt.setup() + buf := &bytes.Buffer{} + processStruct(buf, s, name) + assert.Equal(t, tt.expected, buf.String()) + }) + } +} From c2e3208a7ff5e146f8f57d324599698d04f79830 Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Thu, 23 Jan 2025 16:29:46 +0100 Subject: [PATCH 3/7] feat(linter): goimports and nestingReduce: --- tools/logger/logger.go | 43 ++++++++++++++++++++----------------- tools/logger/logger_test.go | 3 ++- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/tools/logger/logger.go b/tools/logger/logger.go index 63e81649..e0c651df 100644 --- a/tools/logger/logger.go +++ b/tools/logger/logger.go @@ -139,26 +139,29 @@ func filterStructs(pkg *packages.Package) ([]string, []*types.Struct) { func processStruct(w io.Writer, s *types.Struct, name string) { write(w, functionHeaderFormat, map[string]string{"name": name}) for kk := 0; kk < s.NumFields(); kk++ { - if field, ok := reflect.StructTag(s.Tag(kk)).Lookup("log"); ok { - var annotations string - fieldParts := strings.SplitN(field, ",", 2) - field = fieldParts[0] - if len(fieldParts) > 1 { - annotations = fieldParts[1] - } - args := map[string]string{"key": field, "name": s.Field(kk).Name()} - switch { - case s.Field(kk).Type().String() == "time.Time": - write(w, timeFieldFormat, args) - case field == "." && isNilable(s.Field(kk).Type()): - write(w, nestedNilableMarshalerFormat, args) - case field == ".": - write(w, nestedMarshalerFormat, args) - case strings.Contains(annotations, annotationOmitEmpty): - write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args) - default: - write(w, simpleFieldFormat, args) - } + field, ok := reflect.StructTag(s.Tag(kk)).Lookup("log") + if !ok { + continue + } + + var annotations string + fieldParts := strings.SplitN(field, ",", 2) + field = fieldParts[0] + if len(fieldParts) > 1 { + annotations = fieldParts[1] + } + args := map[string]string{"key": field, "name": s.Field(kk).Name()} + switch { + case s.Field(kk).Type().String() == "time.Time": + write(w, timeFieldFormat, args) + case field == "." && isNilable(s.Field(kk).Type()): + write(w, nestedNilableMarshalerFormat, args) + case field == ".": + write(w, nestedMarshalerFormat, args) + case strings.Contains(annotations, annotationOmitEmpty): + write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args) + default: + write(w, simpleFieldFormat, args) } } fmt.Fprintf(w, "\n}\n") diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go index 473f7400..6434815a 100644 --- a/tools/logger/logger_test.go +++ b/tools/logger/logger_test.go @@ -3,8 +3,9 @@ package main import ( "bytes" "go/types" - "gotest.tools/v3/assert" "testing" + + "gotest.tools/v3/assert" ) func TestGetSimpleOptionalFieldFormat(t *testing.T) { From 2c2c511ef3a325772205000c061087ac076ab600 Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Thu, 23 Jan 2025 16:42:40 +0100 Subject: [PATCH 4/7] fix(omitempty): use array instead of strings to list annotations --- tools/logger/logger.go | 15 ++++++-- tools/logger/logger_test.go | 71 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/tools/logger/logger.go b/tools/logger/logger.go index e0c651df..6a1c4af7 100644 --- a/tools/logger/logger.go +++ b/tools/logger/logger.go @@ -144,11 +144,11 @@ func processStruct(w io.Writer, s *types.Struct, name string) { continue } - var annotations string + var annotations []string fieldParts := strings.SplitN(field, ",", 2) field = fieldParts[0] if len(fieldParts) > 1 { - annotations = fieldParts[1] + annotations = fieldParts[1:] } args := map[string]string{"key": field, "name": s.Field(kk).Name()} switch { @@ -158,7 +158,7 @@ func processStruct(w io.Writer, s *types.Struct, name string) { write(w, nestedNilableMarshalerFormat, args) case field == ".": write(w, nestedMarshalerFormat, args) - case strings.Contains(annotations, annotationOmitEmpty): + case contains(annotations, annotationOmitEmpty): write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args) default: write(w, simpleFieldFormat, args) @@ -206,3 +206,12 @@ func getSimpleOptionalFieldFormat(p types.Type) string { return fmt.Sprintf(simpleOptionalFieldFormat, defaultValue) } + +func contains[T comparable](slice []T, item T) bool { + for _, s := range slice { + if s == item { + return true + } + } + return false +} diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go index 6434815a..0affc8fa 100644 --- a/tools/logger/logger_test.go +++ b/tools/logger/logger_test.go @@ -201,3 +201,74 @@ if s.Nested != nil { }) } } + +func TestContains(t *testing.T) { + tests := []struct { + name string + slice any + item any + expected bool + }{ + { + name: "string slice with matching item", + slice: []string{"a", "b", "c"}, + item: "b", + expected: true, + }, + { + name: "string slice without matching item", + slice: []string{"a", "b", "c"}, + item: "d", + expected: false, + }, + { + name: "empty string slice", + slice: []string{}, + item: "a", + expected: false, + }, + { + name: "int slice with matching item", + slice: []int{1, 2, 3}, + item: 2, + expected: true, + }, + { + name: "int slice without matching item", + slice: []int{1, 2, 3}, + item: 4, + expected: false, + }, + { + name: "bool slice with matching item", + slice: []bool{true, false}, + item: true, + expected: true, + }, + { + name: "bool slice without matching item", + slice: []bool{false}, + item: true, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + switch slice := tt.slice.(type) { + case []string: + if got := contains(slice, tt.item.(string)); got != tt.expected { + t.Errorf("contains() = %v, want %v", got, tt.expected) + } + case []int: + if got := contains(slice, tt.item.(int)); got != tt.expected { + t.Errorf("contains() = %v, want %v", got, tt.expected) + } + case []bool: + if got := contains(slice, tt.item.(bool)); got != tt.expected { + t.Errorf("contains() = %v, want %v", got, tt.expected) + } + } + }) + } +} From 4091e24ddf7c83cf9052ef5392565ce08431b561 Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Sat, 25 Jan 2025 02:01:43 +0100 Subject: [PATCH 5/7] fix(doc): rename fields and update docs --- tools/logger/generating.md | 52 ++++++++--------- tools/logger/logger.go | 8 +-- tools/logger/logger_test.go | 109 ++++++++++++++++++++++-------------- 3 files changed, 97 insertions(+), 72 deletions(-) diff --git a/tools/logger/generating.md b/tools/logger/generating.md index 6b888995..2dfe0125 100644 --- a/tools/logger/generating.md +++ b/tools/logger/generating.md @@ -7,9 +7,9 @@ Consider a `struct` like so: ```golang -type OrgInfo struct { - Org string - Guid string +type Org struct { + Shortname string + GUID string } ``` @@ -20,14 +20,14 @@ this: ```golang -type OrgInfo struct - Org string `log:"or.org.shortname"` - Guid string `log:"or.org.guid"` +type Org struct + Shortname string `log:"or.org.shortname"` + GUID string `log:"or.org.guid"` } ``` -With the addition of the log tags `ie. log:"xyz"` for fields (xyz matches the [standard +With the addition of the `log` tags for fields (which matches the [standard attributes](https://app.datadoghq.com/logs/pipelines/standard-attributes)), we can use [logger](https://github.com/getoutreach/gobox/tree/master/tools/logger) @@ -35,52 +35,52 @@ to generate the `MarshalLog` method: ```bash -# run from directory where the OrgInfo struct is stored +# run from directory where the Org struct is stored $> go run github.com/getoutreach/gobox/tools/logger -output marshalers.go $> cat marshalers.go // Code generated by "logger -output marshalers.go"; DO NOT EDIT. package log -func (s *OrgInfo) MarshalLog(addField func(key string, value interface{})) { +func (s *Org) MarshalLog(addField func(key string, value interface{})) { if s == nil { return } - addField(or.org.shortname, Org) - addField(or.org.guid, Guid) + addField(or.org.shortname, Shortname) + addField(or.org.guid, GUID) } ``` -### Annotations: -These tags for fields also have limited support for annotations: `ie. log:"xyz,annotation"`. - +### Annotations Supported annotations: -- **omitempty**: makes the field optional. +- `omitempty`: makes the field optional. - `omitempty` annotation is available for simple built-in types or - custom types with underlying type being built-in or pointers of any type. + It is available for basic types, types aliased to a basic type, + and pointers of any type. The annotation parser does **not** + validate if the annotation is applied to supported types. Example: + ```go - type OrgInfo struct - Org string `log:"or.org.shortname,omitempty"` - Guid *Custom `log:"or.org.guid,omitempty"` + type Org struct + Shortname string `log:"or.org.shortname,omitempty"` + GUID *Custom `log:"or.org.guid,omitempty"` } ``` - Generated code: + Generated code (post-formatted with `gofmt`): ```go - func (s *OrgInfo) MarshalLog(addField func(key string, value interface{})) { + func (s *Org) MarshalLog(addField func(key string, value interface{})) { if s == nil { return } - if s.Org != "" { - addField("or.org.shortname", s.Org) + if s.Shortname != "" { + addField("or.org.shortname", s.Shortname) } - if s.Guid != nil { - addField("or.org.guid", s.Guid) + if s.GUID != nil { + addField("or.org.guid", s.GUID) } } ``` diff --git a/tools/logger/logger.go b/tools/logger/logger.go index 6a1c4af7..eff2a5c1 100644 --- a/tools/logger/logger.go +++ b/tools/logger/logger.go @@ -46,7 +46,7 @@ func (s *{{ .name }}) MarshalLog(addField func(key string, value interface{})) { addField("{{.key}}", s.{{.name}}.UTC().Format(time.RFC3339Nano))` simpleFieldFormat = ` addField("{{.key}}", s.{{.name}})` - simpleOptionalFieldFormat = ` + optionalFieldFormat = ` if s.{{.name}} != %s { addField("{{.key}}", s.{{.name}}) }` @@ -159,7 +159,7 @@ func processStruct(w io.Writer, s *types.Struct, name string) { case field == ".": write(w, nestedMarshalerFormat, args) case contains(annotations, annotationOmitEmpty): - write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args) + write(w, getOptionalFieldFormat(s.Field(kk).Type()), args) default: write(w, simpleFieldFormat, args) } @@ -188,7 +188,7 @@ func usage() { flag.PrintDefaults() } -func getSimpleOptionalFieldFormat(p types.Type) string { +func getOptionalFieldFormat(p types.Type) string { var defaultValue string switch p.Underlying().String() { case "string": @@ -204,7 +204,7 @@ func getSimpleOptionalFieldFormat(p types.Type) string { defaultValue = "nil" } - return fmt.Sprintf(simpleOptionalFieldFormat, defaultValue) + return fmt.Sprintf(optionalFieldFormat, defaultValue) } func contains[T comparable](slice []T, item T) bool { diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go index 0affc8fa..e7104be4 100644 --- a/tools/logger/logger_test.go +++ b/tools/logger/logger_test.go @@ -8,7 +8,7 @@ import ( "gotest.tools/v3/assert" ) -func TestGetSimpleOptionalFieldFormat(t *testing.T) { +func TestGetOptionalFieldFormat(t *testing.T) { newNamedType := func(name string, underlying types.Type) types.Type { return types.NewNamed( types.NewTypeName(0, nil, name, nil), @@ -76,9 +76,9 @@ func TestGetSimpleOptionalFieldFormat(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := getSimpleOptionalFieldFormat(tt.typ) + got := getOptionalFieldFormat(tt.typ) if got != tt.expected { - t.Errorf("getSimpleOptionalFieldFormat() =\n%v\nwant:\n%v", got, tt.expected) + t.Errorf("getOptionalFieldFormat() =\n%v\nwant:\n%v", got, tt.expected) } }) } @@ -203,69 +203,94 @@ if s.Nested != nil { } func TestContains(t *testing.T) { + var uninitializedStrings []string + var uninitializedIntegers []int + var uninitializedBooleans []bool tests := []struct { - name string - slice any - item any - expected bool + name string + slice any + item any + underlyingType string + expected bool }{ { - name: "string slice with matching item", - slice: []string{"a", "b", "c"}, - item: "b", - expected: true, + name: "string slice with matching item", + slice: []string{"a", "b", "c"}, + item: "b", + underlyingType: "string", + expected: true, + }, + { + name: "string slice without matching item", + slice: []string{"a", "b", "c"}, + item: "d", + underlyingType: "string", + expected: false, + }, + { + name: "empty string slice", + slice: []string{}, + item: "a", + underlyingType: "string", + expected: false, }, { - name: "string slice without matching item", - slice: []string{"a", "b", "c"}, - item: "d", - expected: false, + name: "uninitialized string slice", + slice: uninitializedStrings, + item: "a", + underlyingType: "string", + expected: false, }, { - name: "empty string slice", - slice: []string{}, - item: "a", - expected: false, + name: "int slice with matching item", + slice: []int{1, 2, 3}, + item: 2, + underlyingType: "int", + expected: true, }, { - name: "int slice with matching item", - slice: []int{1, 2, 3}, - item: 2, - expected: true, + name: "uninitialized string slice", + slice: uninitializedIntegers, + item: 4, + underlyingType: "int", + expected: false, }, { - name: "int slice without matching item", - slice: []int{1, 2, 3}, - item: 4, - expected: false, + name: "bool slice with matching item", + slice: []bool{true, false}, + item: true, + underlyingType: "bool", + expected: true, }, { - name: "bool slice with matching item", - slice: []bool{true, false}, - item: true, - expected: true, + name: "bool slice without matching item", + slice: []bool{false}, + item: true, + underlyingType: "bool", + expected: false, }, { - name: "bool slice without matching item", - slice: []bool{false}, - item: true, - expected: false, + name: "uninitialized bool slice", + slice: uninitializedBooleans, + item: false, + underlyingType: "bool", + expected: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - switch slice := tt.slice.(type) { - case []string: - if got := contains(slice, tt.item.(string)); got != tt.expected { + switch tt.underlyingType { + case "string": + if got := contains(tt.slice.([]string), tt.item.(string)); got != tt.expected { t.Errorf("contains() = %v, want %v", got, tt.expected) } - case []int: - if got := contains(slice, tt.item.(int)); got != tt.expected { + case "int": + if got := contains(tt.slice.([]int), tt.item.(int)); got != tt.expected { t.Errorf("contains() = %v, want %v", got, tt.expected) } - case []bool: - if got := contains(slice, tt.item.(bool)); got != tt.expected { + case "bool": + if got := contains(tt.slice.([]bool), tt.item.(bool)); got != tt.expected { t.Errorf("contains() = %v, want %v", got, tt.expected) } } From b47424356974c6cd147e56efdf6f18bae0f1a6c3 Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Sat, 25 Jan 2025 05:10:21 +0100 Subject: [PATCH 6/7] fix(test): fix worng name for test --- tools/logger/logger_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go index e7104be4..666be191 100644 --- a/tools/logger/logger_test.go +++ b/tools/logger/logger_test.go @@ -249,7 +249,7 @@ func TestContains(t *testing.T) { expected: true, }, { - name: "uninitialized string slice", + name: "uninitialized int slice", slice: uninitializedIntegers, item: 4, underlyingType: "int", From 212a99cead954672f99f1ecb7f05fac92d3f16ec Mon Sep 17 00:00:00 2001 From: T M Rezoan Tamal Date: Fri, 7 Feb 2025 13:38:25 +0100 Subject: [PATCH 7/7] Break down ProcessStruct to individual tests to improve readability --- tools/logger/logger_test.go | 151 ++++++++++++++---------------------- 1 file changed, 57 insertions(+), 94 deletions(-) diff --git a/tools/logger/logger_test.go b/tools/logger/logger_test.go index 666be191..4cadf53c 100644 --- a/tools/logger/logger_test.go +++ b/tools/logger/logger_test.go @@ -84,49 +84,23 @@ func TestGetOptionalFieldFormat(t *testing.T) { } } -func TestProcessStruct(t *testing.T) { - tests := []struct { - name string - setup func() (*types.Struct, string) - expected string - }{ - { - name: "basic fields", - setup: func() (*types.Struct, string) { - fields := []*types.Var{ - types.NewField(0, nil, "Name", types.Typ[types.String], false), - types.NewField(0, nil, "Age", types.Typ[types.Int], false), - } - tags := []string{ - `log:"name"`, - `log:"age"`, - } - return types.NewStruct(fields, tags), "BasicStruct" - }, - expected: ` -func (s *BasicStruct) MarshalLog(addField func(key string, value interface{})) { - if s == nil { - return - } +func TestProcessStructWithTimeField(t *testing.T) { + var buf bytes.Buffer + timeType := types.NewNamed( + types.NewTypeName(0, types.NewPackage("time", "time"), "Time", nil), + types.NewStruct(nil, nil), + nil, + ) -addField("name", s.Name) -addField("age", s.Age) -} -`, - }, - { - name: "time field", - setup: func() (*types.Struct, string) { - pkg := types.NewPackage("time", "time") - timeType := types.NewNamed(types.NewTypeName(0, pkg, "Time", nil), &types.Struct{}, nil) + s := types.NewStruct([]*types.Var{ + types.NewVar(0, nil, "CreatedAt", timeType), + }, []string{ + `log:"created_at"`, + }) - fields := []*types.Var{ - types.NewField(0, nil, "CreatedAt", timeType, false), - } - tags := []string{`log:"created_at"`} - return types.NewStruct(fields, tags), "TimeStruct" - }, - expected: ` + processStruct(&buf, s, "TimeStruct") + + expected := ` func (s *TimeStruct) MarshalLog(addField func(key string, value interface{})) { if s == nil { return @@ -134,72 +108,61 @@ func (s *TimeStruct) MarshalLog(addField func(key string, value interface{})) { addField("created_at", s.CreatedAt.UTC().Format(time.RFC3339Nano)) } -`, - }, - { - name: "omitempty fields", - setup: func() (*types.Struct, string) { - fields := []*types.Var{ - types.NewField(0, nil, "Name", types.Typ[types.String], false), - types.NewField(0, nil, "AgeP", types.NewPointer(types.Typ[types.Int]), false), - } - tags := []string{ - `log:"name,omitempty"`, - `log:"ageP,omitempty"`, - } - return types.NewStruct(fields, tags), "OmitStruct" - }, - expected: ` -func (s *OmitStruct) MarshalLog(addField func(key string, value interface{})) { +` + assert.Equal(t, expected, buf.String()) +} + +func TestProcessStructWithNestedMarshaler(t *testing.T) { + var buf bytes.Buffer + nestedType := types.NewNamed( + types.NewTypeName(0, nil, "NestedStruct", nil), + types.NewStruct(nil, nil), + nil, + ) + s := types.NewStruct([]*types.Var{ + types.NewVar(0, nil, "Child", types.NewPointer(nestedType)), + }, []string{ + `log:"."`, + }) + + processStruct(&buf, s, "ParentStruct") + + expected := ` +func (s *ParentStruct) MarshalLog(addField func(key string, value interface{})) { if s == nil { return } -if s.Name != "" { - addField("name", s.Name) +if s.Child != nil { + s.Child.MarshalLog(addField) } -if s.AgeP != nil { - addField("ageP", s.AgeP) } +` + assert.Equal(t, expected, buf.String()) } -`, - }, - { - name: "nested marshaler", - setup: func() (*types.Struct, string) { - nestedType := types.NewPointer(types.NewNamed( - types.NewTypeName(0, nil, "NestedStruct", nil), - &types.Struct{}, - nil, - )) - fields := []*types.Var{ - types.NewField(0, nil, "Nested", nestedType, false), - } - tags := []string{`log:"."`} - return types.NewStruct(fields, tags), "ParentStruct" - }, - expected: ` -func (s *ParentStruct) MarshalLog(addField func(key string, value interface{})) { + +func TestProcessStructWithOmitemptyField(t *testing.T) { + var buf bytes.Buffer + s := types.NewStruct([]*types.Var{ + types.NewVar(0, nil, "Status", types.Typ[types.String]), + }, []string{ + `log:"status,omitempty"`, + }) + + processStruct(&buf, s, "OptionalStruct") + + expected := ` +func (s *OptionalStruct) MarshalLog(addField func(key string, value interface{})) { if s == nil { return } -if s.Nested != nil { - s.Nested.MarshalLog(addField) +if s.Status != "" { + addField("status", s.Status) } } -`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - s, name := tt.setup() - buf := &bytes.Buffer{} - processStruct(buf, s, name) - assert.Equal(t, tt.expected, buf.String()) - }) - } +` + assert.Equal(t, expected, buf.String()) } func TestContains(t *testing.T) {