Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Logger.Log specification to "should not actually panic or exit" #168

Merged
merged 5 commits into from
Nov 10, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Shift Logger.Log contract to "should not actually exit or panic"
  • Loading branch information
Joshua T Corbin committed Nov 9, 2016
commit 5ad23b91173ab968b9cd55b86aa23916778917d5
14 changes: 6 additions & 8 deletions logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ type Logger interface {

// Log a message at the given level. Messages include any context that's
// accumulated on the logger, as well as any fields added at the log site.
//
// Calling Panic should panic(), calling Fatal() should terminate the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "...should panic() and calling Fatal()..."

// process, but calling Log(PanicLevel, ...) or Log(FatalLevel, ...) should
// not. It may not be possible for compatibility wrappers to comply with
// this last part (e.g. the bark wrapper).
Log(Level, string, ...Field)
Debug(string, ...Field)
Info(string, ...Field)
Expand Down Expand Up @@ -99,14 +104,7 @@ func (log *logger) Check(lvl Level, msg string) *CheckedMessage {
}

func (log *logger) Log(lvl Level, msg string, fields ...Field) {
switch lvl {
case PanicLevel:
log.Panic(msg, fields...)
case FatalLevel:
log.Fatal(msg, fields...)
default:
log.log(lvl, msg, fields)
}
log.log(lvl, msg, fields)
}

func (log *logger) Debug(msg string, fields ...Field) {
Expand Down
32 changes: 29 additions & 3 deletions logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,15 +186,41 @@ func TestJSONLoggerLog(t *testing.T) {
})

withJSONLogger(t, nil, func(logger Logger, buf *testBuffer) {
assert.Panics(t, func() { logger.Log(PanicLevel, "foo") }, "Expected logging at Panic level to panic.")
assert.NotPanics(t, func() { logger.Log(PanicLevel, "foo") }, "Expected logging at Panic level to not panic.")
assert.Equal(t, `{"level":"panic","msg":"foo"}`, buf.Stripped(), "Unexpected output from panic-level Log.")
})

stub := stubExit()
defer stub.Unstub()
withJSONLogger(t, nil, func(logger Logger, buf *testBuffer) {
assert.Panics(t, func() { logger.Panic("bar") }, "Expected logger.Panic to panic.")
assert.Equal(t, `{"level":"panic","msg":"bar"}`, buf.Stripped(), "Unexpected output from panic-level Log.")
})

withJSONLogger(t, nil, func(logger Logger, buf *testBuffer) {
assert.Panics(t, func() { logger.Check(PanicLevel, "baz").Write() }, "Expected checked logging at Panic level to panic.")
assert.Equal(t, `{"level":"panic","msg":"baz"}`, buf.Stripped(), "Unexpected output from panic-level Log.")
})

withJSONLogger(t, nil, func(logger Logger, buf *testBuffer) {
stub := stubExit()
defer stub.Unstub()
logger.Log(FatalLevel, "foo")
assert.Equal(t, `{"level":"fatal","msg":"foo"}`, buf.Stripped(), "Unexpected output from fatal-level Log.")
stub.AssertNoExit(t)
})

withJSONLogger(t, nil, func(logger Logger, buf *testBuffer) {
stub := stubExit()
defer stub.Unstub()
logger.Check(FatalLevel, "bar").Write()
assert.Equal(t, `{"level":"fatal","msg":"bar"}`, buf.Stripped(), "Unexpected output from fatal-level Log.")
stub.AssertStatus(t, 1)
})

withJSONLogger(t, nil, func(logger Logger, buf *testBuffer) {
stub := stubExit()
defer stub.Unstub()
logger.Fatal("baz")
assert.Equal(t, `{"level":"fatal","msg":"baz"}`, buf.Stripped(), "Unexpected output from fatal-level Log.")
stub.AssertStatus(t, 1)
})
}
Expand Down
2 changes: 2 additions & 0 deletions zbark/debark.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ func (z *zapper) DFatal(msg string, fields ...zap.Field) {
}

func (z *zapper) Log(l zap.Level, msg string, fields ...zap.Field) {
// NOTE: logging at panic and fatal level actually panic and exit the
// process, meaning that bark loggers cannot compose well.
switch l {
case zap.PanicLevel, zap.FatalLevel:
default:
Expand Down