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

Add options to customize Fatal behaviour for better testability #861

Merged
merged 5 commits into from Sep 1, 2020

Conversation

prashantv
Copy link
Collaborator

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.

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.
@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #861 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
+ Coverage   98.35%   98.37%   +0.01%     
==========================================
  Files          43       43              
  Lines        2378     2393      +15     
==========================================
+ Hits         2339     2354      +15     
  Misses         32       32              
  Partials        7        7              
Impacted Files Coverage Δ
logger.go 95.31% <100.00%> (+0.11%) ⬆️
options.go 100.00% <100.00%> (ø)
zapcore/entry.go 94.62% <100.00%> (+0.11%) ⬆️
zapcore/field.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2657839...5de1194. Read the comment docs.

zapcore/entry.go Outdated Show resolved Hide resolved
logger.go Outdated Show resolved Hide resolved
logger.go Outdated Show resolved Hide resolved
@prashantv prashantv merged commit 217b2cb into master Sep 1, 2020
@prashantv prashantv deleted the fatal branch September 1, 2020 20:30
cgxxv pushed a commit to cgxxv/zap that referenced this pull request 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.
sywhang pushed a commit that referenced this pull request Feb 20, 2024
Add a `WithPanicHook` logger option that allows callers to specify
custom behavior besides panicking on Panic/DPanic logs. This is similar
to what we already have with the WithFatal hook implemented in
#861.

This will make it possible to unit test Panic log cases like the one we
had with our periodic runner which was impossible because of
unrecoverable panics in another go routine.

Added unit tests and they pass.

```
$ make test
?       go.uber.org/zap/internal        [no test files]
?       go.uber.org/zap/internal/bufferpool     [no test files]
ok      go.uber.org/zap (cached)
?       go.uber.org/zap/internal/readme [no test files]
ok      go.uber.org/zap/buffer  (cached)
ok      go.uber.org/zap/internal/color  (cached)
ok      go.uber.org/zap/internal/exit   (cached)
ok      go.uber.org/zap/internal/pool   (cached)
ok      go.uber.org/zap/internal/stacktrace     (cached)
ok      go.uber.org/zap/internal/ztest  (cached)
ok      go.uber.org/zap/zapcore (cached)
ok      go.uber.org/zap/zapgrpc (cached)
ok      go.uber.org/zap/zapio   (cached)
ok      go.uber.org/zap/zaptest (cached)
ok      go.uber.org/zap/zaptest/observer        (cached)
ok      go.uber.org/zap/exp/zapfield    (cached)
ok      go.uber.org/zap/exp/zapslog     (cached)
ok      go.uber.org/zap/benchmarks      (cached) [no tests to run]
ok      go.uber.org/zap/zapgrpc/internal/test   (cached)
```

Closes #1415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make it easy to test Fatal logs without requiring indirection at the call site
2 participants