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 NewTestingLogger for usage in tests #28
Conversation
There are many ways to compose the Go kit logger interface with other standard packages and components. In fact, package log was designed specifically so that these kinds of value-add capabilities could be "layered on" to loggers without requiring additions or modifications to the package itself. I'd say this is a good example of something that you can and should implement yourself in your consumer application/library. In fact, I frequently write something quite similar to your test logger: an implementation of the log.Logger interface that converts logger.Log events to tb.Logf calls: type logWriter struct {
tb testing.TB
}
func (w *logWriter) Write(p []byte) (int, error) {
w.tb.Log(strings.TrimSpace(string(p)))
return len(p), nil
}
func newTestLogger(tb testing.TB) log.Logger {
return log.NewLogfmtLogger(&logWriter{tb})
} |
Thanks for the quick reply. I agree with your comment and I think such a function would be a nice addition to this library:
So to sum up, adding such a function does not add much complexity to this library, encourages developers to use it in there tests and prevents having them re-implementing it (which can go wrong). |
I have also written variations on this theme with different bells and whistles depending on the situation. My initial thoughts are that I agree the idea is useful, but I'm not sure the code needs to be in this module because there may be too many small variations that people want. The variations may swamp the simplicity over time and that would be a shame. My vote would be to post the implementation to a discussion thread in this repo. Maybe others will share their tweaks and variations. If, over time, a common core becomes obvious we could add an implementation into the module itself. If we do add it in the future, my instinct is to add it as a separate |
Thanks for the feedback! I agree that we can't provide a solution for every single problem. However I think that proposing a canonical approach is greatly beneficial: my code is 20 lines long, if they really want a variation they can copy/paste and adapt them to the situation.
What would you want? for me the version posted has enough (as mentioned, it is limited regarding
I think that this PR can be considered a discussion thread, but feel free to open a thread a ping me :)
For better discoverability I think the main package would make more sense (however if the goal is to provide many variation, a dedicated package would make sense - I would rather have one accessible convenient function than many variations) |
I appreciate the effort! Your suggestion is certainly useful. But the bar for expanding the surface area of the log module is very very high. And, as @ChrisHines notes, there are many solutions to this problem, none of which IMO are sufficiently general to be the "canonical" implementation. So I'll close this PR, but, again, thanks for the thought and discussion. |
Having access to the log can be quite useful in tests. However it is even nicer if the log is displayed only for failing tests, as well as the location where the call was made.
This PR add a
NewTestingLogger
function which supports such usecase, by callingt.Helper
andt.Log
with the logfmt-formatted string.It uses
logfmt.MarshalKeyvals
directly, since I don't think that performance is so critical in tests.