From ee180f341a74b25fa8d3aecaf661183876834347 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Thu, 2 Apr 2020 23:23:30 -0700 Subject: [PATCH 1/2] Fix IncreaseLevel being reset after With Fixes #810 We missed implementing the increase-level logic in With and since Core was embedded, we wended up using With from the original core. To avoid this sort of issue, avoid embedding and implement all Core methods explicitly. This lets us consider the behaviour of each method explicitly. --- increase_level_test.go | 13 +++++++++++-- zapcore/increase_level.go | 17 ++++++++++++++--- zapcore/increase_level_test.go | 11 +++++++++++ 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/increase_level_test.go b/increase_level_test.go index d3e111464..56a23873c 100644 --- a/increase_level_test.go +++ b/increase_level_test.go @@ -29,10 +29,13 @@ import ( "go.uber.org/zap/zaptest/observer" ) -func newLoggedEntry(level zapcore.Level, msg string) observer.LoggedEntry { +func newLoggedEntry(level zapcore.Level, msg string, fields ...zapcore.Field) observer.LoggedEntry { + if len(fields) == 0 { + fields = []zapcore.Field{} + } return observer.LoggedEntry{ Entry: zapcore.Entry{Level: level, Message: msg}, - Context: []zapcore.Field{}, + Context: fields, } } @@ -75,9 +78,15 @@ func TestIncreaseLevel(t *testing.T) { errorLogger.Warn("ignored warn log") errorLogger.Error("increase level error log") + withFields := errorLogger.With(String("k", "v")) + withFields.Debug("ignored debug log with fields") + withFields.Warn("ignored warn log with fields") + withFields.Error("increase level error log with fields") + assert.Equal(t, []observer.LoggedEntry{ newLoggedEntry(WarnLevel, "original warn log"), newLoggedEntry(ErrorLevel, "increase level error log"), + newLoggedEntry(ErrorLevel, "increase level error log with fields", String("k", "v")), }, logs.AllUntimed(), "unexpected logs") assert.Empty(t, errorOut.String(), "expect no error output") diff --git a/zapcore/increase_level.go b/zapcore/increase_level.go index a42135c15..5a1749261 100644 --- a/zapcore/increase_level.go +++ b/zapcore/increase_level.go @@ -23,8 +23,7 @@ package zapcore import "fmt" type levelFilterCore struct { - Core - + core Core level LevelEnabler } @@ -46,10 +45,22 @@ func (c *levelFilterCore) Enabled(lvl Level) bool { return c.level.Enabled(lvl) } +func (c *levelFilterCore) With(fields []Field) Core { + return &levelFilterCore{c.core.With(fields), c.level} +} + func (c *levelFilterCore) Check(ent Entry, ce *CheckedEntry) *CheckedEntry { if !c.Enabled(ent.Level) { return ce } - return c.Core.Check(ent, ce) + return c.core.Check(ent, ce) +} + +func (c *levelFilterCore) Write(ent Entry, fields []Field) error { + return c.core.Write(ent, fields) +} + +func (c *levelFilterCore) Sync() error { + return c.core.Sync() } diff --git a/zapcore/increase_level_test.go b/zapcore/increase_level_test.go index 8b8613375..29fca418e 100644 --- a/zapcore/increase_level_test.go +++ b/zapcore/increase_level_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" . "go.uber.org/zap/zapcore" "go.uber.org/zap/zaptest/observer" ) @@ -35,6 +36,7 @@ func TestIncreaseLevel(t *testing.T) { coreLevel Level increaseLevel Level wantErr bool + with []Field }{ { coreLevel: InfoLevel, @@ -49,6 +51,11 @@ func TestIncreaseLevel(t *testing.T) { coreLevel: InfoLevel, increaseLevel: ErrorLevel, }, + { + coreLevel: InfoLevel, + increaseLevel: ErrorLevel, + with: []Field{zap.String("k", "v")}, + }, { coreLevel: ErrorLevel, increaseLevel: DebugLevel, @@ -82,6 +89,10 @@ func TestIncreaseLevel(t *testing.T) { return } + if len(tt.with) > 0 { + filteredLogger = filteredLogger.With(tt.with) + } + require.NoError(t, err) for l := DebugLevel; l <= FatalLevel; l++ { From 3ede7e813c22634f3ae73ae62f06ed7af949e6d6 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Sat, 11 Apr 2020 15:25:26 -0700 Subject: [PATCH 2/2] Add coverage for Write + Sync --- zapcore/increase_level_test.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/zapcore/increase_level_test.go b/zapcore/increase_level_test.go index 29fca418e..acb8700f7 100644 --- a/zapcore/increase_level_test.go +++ b/zapcore/increase_level_test.go @@ -80,7 +80,7 @@ func TestIncreaseLevel(t *testing.T) { for _, tt := range tests { msg := fmt.Sprintf("increase %v to %v", tt.coreLevel, tt.increaseLevel) t.Run(msg, func(t *testing.T) { - logger, _ := observer.New(tt.coreLevel) + logger, logs := observer.New(tt.coreLevel) filteredLogger, err := NewIncreaseLevelCore(logger, tt.increaseLevel) if tt.wantErr { @@ -97,15 +97,25 @@ func TestIncreaseLevel(t *testing.T) { for l := DebugLevel; l <= FatalLevel; l++ { enabled := filteredLogger.Enabled(l) - ce := filteredLogger.Check(Entry{Level: l}, nil) + entry := Entry{Level: l} + ce := filteredLogger.Check(entry, nil) + ce.Write() + entries := logs.TakeAll() if l >= tt.increaseLevel { assert.True(t, enabled, "expect %v to be enabled", l) assert.NotNil(t, ce, "expect non-nil Check") + assert.NotEmpty(t, entries, "Expect log to be written") } else { assert.False(t, enabled, "expect %v to be disabled", l) assert.Nil(t, ce, "expect nil Check") + assert.Empty(t, entries, "No logs should have been written") } + + // Write should always log the entry as per the Core interface + require.NoError(t, filteredLogger.Write(entry, nil), "Write failed") + require.NoError(t, filteredLogger.Sync(), "Sync failed") + assert.NotEmpty(t, logs.TakeAll(), "Write should always log") } }) }