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

Make it easy to test Fatal logs without requiring indirection at the call site #846

Closed
prashantv opened this issue Jul 3, 2020 · 2 comments · Fixed by #861
Closed

Make it easy to test Fatal logs without requiring indirection at the call site #846

prashantv opened this issue Jul 3, 2020 · 2 comments · Fixed by #861
Assignees

Comments

@prashantv
Copy link
Collaborator

Related to #494

There's no clean way to test code that uses Fatal,

Rather than exposing the internal/exit package, we could expose an option that overrides the exit function used by zap.

@prashantv prashantv self-assigned this Jul 3, 2020
@abhinav
Copy link
Collaborator

abhinav commented Jul 15, 2020

The expectation in code that calls log.Fatal is that the line following
log.Fatal will never be executed.

log.Fatal("foo")
panic("not executed")

If we allow users to override the behavior arbitrarily, there's no guarantee
that their logic satisfies this guarantee.

Perhaps a better choice is to allow choosing between pre-defined behaviors.
For example, choosing between exiting the process, panicking, or exiting the
current goroutine.

zap.OnFatal(zap.PanicOnFatal)
zap.OnFatal(zap.ExitProcessOnFatal)
zap.OnFatal(zap.ExitGoroutineOnFatal)
// naming TBD

For ExitGoroutineOnFatal, log.Fatal will call runtime.Goexit(). For
PanicOnFatal, log.Fatal will panic.

This makes testing log.Fatal possible because one could do the following with
PanicOnFatal.

log = log.WithOptions(zap.OnFatal(zap.PanicOnFatal))

var panicked interface{}
func() {
  defer func() {
    panicked = recover()
  }()
  
  log.Fatal("foo")
  t.Error("log.Fatal did not exit")
}()

Or the following with ExitGoroutineOnFatal.

log = log.WithOptions(zap.OnFatal(zap.ExitGoroutineOnFatal))

done := make(chan struct{})
go func() {
  defer close(done)
  
  log.Fatal("foo")
  t.Error("log.Fatal did not exit")
}()
<-done

@prashantv
Copy link
Collaborator Author

prashantv commented Jul 16, 2020

That seems reasonable, and would be enough for the test-cases I'm looking at.

There's some cases cases where the test can't easily set up a defer (e.g., in one case, there's a Fatal inside of a time.AfterFunc). However, the test can either use an observer logger and wait till a fatal log is logged, and can even set up a logging hook to be notified.

Looking at the code for how the fatal is plumbed through, I think the pre-defined exit/panic behaviour would be easier to plumb through to the CheckedEntry as well.

prashantv added a commit that referenced this issue Sep 1, 2020
Fixes #846.

There's currently no easy way to test Fatal from outside of zap, as it
triggers an os.Exit(1). Add options to customize the behaviour (allowing
for panic, or Goexit), which can be used with `recover()` in tests.
prashantv added a commit that referenced this issue Sep 1, 2020
Fixes #846.

There's currently no easy way to test Fatal from outside of zap, as it
triggers an os.Exit(1). Add options to customize the behaviour (allowing
for panic, or Goexit), which can be used with `recover()` in tests.
cgxxv pushed a commit to cgxxv/zap that referenced this issue Mar 25, 2022
…-go#861)

Fixes uber-go#846.

There's currently no easy way to test Fatal from outside of zap, as it
triggers an os.Exit(1). Add options to customize the behaviour (allowing
for panic, or Goexit), which can be used with `recover()` in tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants