-
Notifications
You must be signed in to change notification settings - Fork 48
feat: add newzaplogger and rework logptest #335
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
Conversation
properly use zaptest logger instead of rewrapping the zap core this should make full use of zaptest logger and only log if a test fails
Co-authored-by: Khushi Jain <[email protected]>
Co-authored-by: Khushi Jain <[email protected]>
💚 Build Succeeded
History
|
belimawr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The only thing I'd add is to better document the modified/added functions to describe the behaviour of this log in a test scenario. Explaining if/when it will send logs to stdout/err, etc.
NewTestingLogger returns a testing suitable logp.Logger.
Does not explain what is the behaviour of 'suitable testing logger'.
mauri870
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The new constructor that also returns an observer is a very welcome addition!
I'm conflicted about this, it's an implementation but I have no strong opinion, happy to add it if there's consensus |
What does this PR do?
properly use zaptest logger instead of rewrapping the zap core
Why is it important?
this should make full use of zaptest logger and only log if a test fails
Checklist
Author's Checklist
Related issues