Make DPanic fatal in zaptest #1274
Conversation
The intention of DPanic is to catch errors that should never happen. Unless you added zaptest.WrapOptions(zap.Development()), they were previously silently ignored. The downside of this change, however, that it's impossible to _disable_ development mode for zaptest logger.
|
|
Codecov Report
@@ Coverage Diff @@
## master #1274 +/- ##
==========================================
- Coverage 98.08% 97.93% -0.16%
==========================================
Files 50 50
Lines 3240 3242 +2
==========================================
- Hits 3178 3175 -3
- Misses 53 57 +4
- Partials 9 10 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks! I agree that the zaptest logger should panic/run in development mode. This was an oversight. For opting out, we could maybe add a zaptest.LoggerOption that allows setting it to false?
zaptest.NewLogger(t) // will panic
zaptest.NewLogger(t, zaptest.Development(false)) // won't panicThere's a question of whether this is a breaking change, though.
Based on the intent of this API, I would consider this more a bugfix than a breaking change, but I'd like to hear thoughts on this from other maintainers.
|
IMO this is a breaking change in behavior; but since this will only affect tests I think it might be worth considering. Adding an option for opting out of the panic behavior would save many people from suddenly seeing a bunch of their tests break so I'm +1 on that suggestion. |
The intention of
DPanicis to catch errors that should never happen. Unless you addedzaptest.WrapOptions(zap.Development()), they were previously silently ignored.The downside of this change, however, that it's impossible to disable development mode for zaptest logger.