From cb3eb9295a5ad85387b60a782bd8126aa546e6ed Mon Sep 17 00:00:00 2001 From: Josh Kline Date: Sat, 3 Feb 2018 21:14:22 -0800 Subject: [PATCH 1/3] Use t.Run for some encoder tests Wrap each encoder test case with t.Run for improved test output and ability to selectively run specific sub tests. --- zapcore/json_encoder_impl_test.go | 26 ++++++++++++++----------- zapcore/memory_encoder_test.go | 32 ++++++++++++++++--------------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go index a2dd72286..340004f11 100644 --- a/zapcore/json_encoder_impl_test.go +++ b/zapcore/json_encoder_impl_test.go @@ -223,7 +223,9 @@ func TestJSONEncoderObjectFields(t *testing.T) { } for _, tt := range tests { - assertOutput(t, tt.desc, tt.expected, tt.f) + t.Run(tt.desc, func(t *testing.T) { + assertOutput(t, tt.desc, tt.expected, tt.f) + }) } } @@ -314,16 +316,18 @@ func TestJSONEncoderArrays(t *testing.T) { } for _, tt := range tests { - f := func(enc Encoder) error { - return enc.AddArray("array", ArrayMarshalerFunc(func(arr ArrayEncoder) error { - tt.f(arr) - tt.f(arr) - return nil - })) - } - assertOutput(t, tt.desc, `"array":`+tt.expected, func(enc Encoder) { - err := f(enc) - assert.NoError(t, err, "Unexpected error adding array to JSON encoder.") + t.Run(tt.desc, func(t *testing.T) { + f := func(enc Encoder) error { + return enc.AddArray("array", ArrayMarshalerFunc(func(arr ArrayEncoder) error { + tt.f(arr) + tt.f(arr) + return nil + })) + } + assertOutput(t, tt.desc, `"array":`+tt.expected, func(enc Encoder) { + err := f(enc) + assert.NoError(t, err, "Unexpected error adding array to JSON encoder.") + }) }) } } diff --git a/zapcore/memory_encoder_test.go b/zapcore/memory_encoder_test.go index e63454a3e..4523a0e11 100644 --- a/zapcore/memory_encoder_test.go +++ b/zapcore/memory_encoder_test.go @@ -25,6 +25,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestMapObjectEncoderAdd(t *testing.T) { @@ -204,9 +205,11 @@ func TestMapObjectEncoderAdd(t *testing.T) { } for _, tt := range tests { - enc := NewMapObjectEncoder() - tt.f(enc) - assert.Equal(t, tt.expected, enc.Fields["k"], "Unexpected encoder output.") + t.Run(tt.desc, func(t *testing.T) { + enc := NewMapObjectEncoder() + tt.f(enc) + assert.Equal(t, tt.expected, enc.Fields["k"], "Unexpected encoder output.") + }) } } func TestSliceArrayEncoderAppend(t *testing.T) { @@ -255,19 +258,18 @@ func TestSliceArrayEncoderAppend(t *testing.T) { } for _, tt := range tests { - enc := NewMapObjectEncoder() - assert.NoError(t, enc.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error { - tt.f(arr) - tt.f(arr) - return nil - })), "Expected AddArray to succeed.") + t.Run(tt.desc, func(t *testing.T) { + enc := NewMapObjectEncoder() + assert.NoError(t, enc.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error { + tt.f(arr) + tt.f(arr) + return nil + })), "Expected AddArray to succeed.") - arr, ok := enc.Fields["k"].([]interface{}) - if !ok { - t.Errorf("Test case %s didn't encode an array.", tt.desc) - continue - } - assert.Equal(t, []interface{}{tt.expected, tt.expected}, arr, "Unexpected encoder output.") + arr, ok := enc.Fields["k"].([]interface{}) + require.True(t, ok, "Test case %s didn't encode an array.", tt.desc) + assert.Equal(t, []interface{}{tt.expected, tt.expected}, arr, "Unexpected encoder output.") + }) } } From 367a5f76c8a392b89355668bdd2563e31931c8ef Mon Sep 17 00:00:00 2001 From: Josh Kline Date: Sat, 3 Feb 2018 12:22:36 -0800 Subject: [PATCH 2/3] Demonstrate jsonEncoder and MapObjectEncoder differences Add test cases to demonstrate https://github.com/uber-go/zap/issues/554, how jsonEncoder and MapObjectEncoder handle namespaces differently. Specifically: When foo.MarshalLogObject uses OpenNamespace, jsonEncoder.AddObject does not close the namespace. --- zapcore/json_encoder_impl_test.go | 44 +++++++++++++++++++++ zapcore/memory_encoder_test.go | 66 +++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go index 340004f11..72a2de328 100644 --- a/zapcore/json_encoder_impl_test.go +++ b/zapcore/json_encoder_impl_test.go @@ -220,6 +220,22 @@ func TestJSONEncoderObjectFields(t *testing.T) { e.OpenNamespace("innermost") }, }, + { + desc: "object (no nested namespace)", + expected: `"obj":{"obj-out":"obj-outside-namespace"},"not-obj":"should-be-outside-obj"`, + f: func(e Encoder) { + e.AddObject("obj", maybeNamespace{false}) + e.AddString("not-obj", "should-be-outside-obj") + }, + }, + { + desc: "object (with nested namespace)", + expected: `"obj":{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"not-obj":"should-be-outside-obj"`, + f: func(e Encoder) { + e.AddObject("obj", maybeNamespace{true}) + e.AddString("not-obj", "should-be-outside-obj") + }, + }, } for _, tt := range tests { @@ -313,6 +329,22 @@ func TestJSONEncoderArrays(t *testing.T) { ) }, }, + { + desc: "object (no nested namespace) then string", + expected: `[{"obj-out":"obj-outside-namespace"},"should-be-outside-obj",{"obj-out":"obj-outside-namespace"},"should-be-outside-obj"]`, + f: func(arr ArrayEncoder) { + arr.AppendObject(maybeNamespace{false}) + arr.AppendString("should-be-outside-obj") + }, + }, + { + desc: "object (with nested namespace) then string", + expected: `[{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj",{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj"]`, + f: func(arr ArrayEncoder) { + arr.AppendObject(maybeNamespace{true}) + arr.AppendString("should-be-outside-obj") + }, + }, } for _, tt := range tests { @@ -400,6 +432,18 @@ func (l loggable) MarshalLogArray(enc ArrayEncoder) error { return nil } +// maybeNamespace is an ObjectMarhsaler that sometimes opens a namespace +type maybeNamespace struct{ bool } + +func (m maybeNamespace) MarshalLogObject(enc ObjectEncoder) error { + enc.AddString("obj-out", "obj-outside-namespace") + if m.bool { + enc.OpenNamespace("obj-namespace") + enc.AddString("obj-in", "obj-inside-namespace") + } + return nil +} + type noJSON struct{} func (nj noJSON) MarshalJSON() ([]byte, error) { diff --git a/zapcore/memory_encoder_test.go b/zapcore/memory_encoder_test.go index 4523a0e11..bb647001b 100644 --- a/zapcore/memory_encoder_test.go +++ b/zapcore/memory_encoder_test.go @@ -202,6 +202,37 @@ func TestMapObjectEncoderAdd(t *testing.T) { }, }, }, + { + desc: "object (no nested namespace) then string", + f: func(e ObjectEncoder) { + e.OpenNamespace("k") + e.AddObject("obj", maybeNamespace{false}) + e.AddString("not-obj", "should-be-outside-obj") + }, + expected: map[string]interface{}{ + "obj": map[string]interface{}{ + "obj-out": "obj-outside-namespace", + }, + "not-obj": "should-be-outside-obj", + }, + }, + { + desc: "object (with nested namespace) then string", + f: func(e ObjectEncoder) { + e.OpenNamespace("k") + e.AddObject("obj", maybeNamespace{true}) + e.AddString("not-obj", "should-be-outside-obj") + }, + expected: map[string]interface{}{ + "obj": map[string]interface{}{ + "obj-out": "obj-outside-namespace", + "obj-namespace": map[string]interface{}{ + "obj-in": "obj-inside-namespace", + }, + }, + "not-obj": "should-be-outside-obj", + }, + }, } for _, tt := range tests { @@ -255,6 +286,41 @@ func TestSliceArrayEncoderAppend(t *testing.T) { }, expected: []interface{}{true, false}, }, + { + desc: "object (no nested namespace) then string", + f: func(e ArrayEncoder) { + e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + inner.AppendObject(maybeNamespace{false}) + inner.AppendString("should-be-outside-obj") + return nil + })) + }, + expected: []interface{}{ + map[string]interface{}{ + "obj-out": "obj-outside-namespace", + }, + "should-be-outside-obj", + }, + }, + { + desc: "object (with nested namespace) then string", + f: func(e ArrayEncoder) { + e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + inner.AppendObject(maybeNamespace{true}) + inner.AppendString("should-be-outside-obj") + return nil + })) + }, + expected: []interface{}{ + map[string]interface{}{ + "obj-out": "obj-outside-namespace", + "obj-namespace": map[string]interface{}{ + "obj-in": "obj-inside-namespace", + }, + }, + "should-be-outside-obj", + }, + }, } for _, tt := range tests { From 9cc8d3d25e8f175d6b1e4b14ad500be8ebe6e0a0 Mon Sep 17 00:00:00 2001 From: Josh Kline Date: Sat, 3 Feb 2018 22:09:46 -0800 Subject: [PATCH 3/3] Fix type in comment --- zapcore/json_encoder_impl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go index 72a2de328..5e7ecc6ae 100644 --- a/zapcore/json_encoder_impl_test.go +++ b/zapcore/json_encoder_impl_test.go @@ -432,7 +432,7 @@ func (l loggable) MarshalLogArray(enc ArrayEncoder) error { return nil } -// maybeNamespace is an ObjectMarhsaler that sometimes opens a namespace +// maybeNamespace is an ObjectMarshaler that sometimes opens a namespace type maybeNamespace struct{ bool } func (m maybeNamespace) MarshalLogObject(enc ObjectEncoder) error {