Extend WithLazy() method to SugaredLogger #1378
Conversation
|
|
sugar_test.go
Outdated
| }, | ||
| }, | ||
| { | ||
| name: "lazy with captures arguments at time of With or Logging", |
There was a problem hiding this comment.
I suspect that this may be a typo in the original zap.Logger PR, but I think the test case name should be "lazy with captures arguments at time of Logging"
There was a problem hiding this comment.
Yeah, I think so, fixed
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
https://github.com/uber-go/zap/pull/1319/files also has a few more scenarios for the capture tests. While you are testing some aspect of chained With and WithLazy, I think it makes sense to also test interleaved calls of With then WithLazy and vis-versa as well.
There was a problem hiding this comment.
I was actually thinking about including those interleaved With and WithLazy tests at first. But then I thought, since lazyWithCore already covers what we need, maybe it’s better not to repeat the same stuff? I just focused on showing how With and WithLazy are different, especially with how WithLazy uses that extra core.
I'm totally fine with adding more tests if it really helps. What do you think?
There was a problem hiding this comment.
I agree that there would be testing overlap between the two loggers.
I think it would be nice for zap to set up testing in a way that would be easy to verify sugared and non sugared behavior through the same process where they should be similar. However, that doesn't need to be part of this change.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1378 +/- ##
==========================================
+ Coverage 98.28% 98.42% +0.14%
==========================================
Files 53 53
Lines 3493 3495 +2
==========================================
+ Hits 3433 3440 +7
+ Misses 50 46 -4
+ Partials 10 9 -1 ☔ View full report in Codecov by Sentry. |
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
I agree that there would be testing overlap between the two loggers.
I think it would be nice for zap to set up testing in a way that would be easy to verify sugared and non sugared behavior through the same process where they should be similar. However, that doesn't need to be part of this change.
Hey there! Resolves #1377
Notes:
With()method to work with bothWith()andWithLazy()With()andWithLazy(), I added a test similar to this one. I kept only the test cases that clearly show the distinctionsLooking forward to your thoughts!