-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
proposal: testing: Add T.Output() etc #59928
Comments
Expanding on this little, there are two cases of logging during testing:
This is just about case 2. For that reason, I thinking testing.Slog() should return a slog.Handler that is essentially a slog.TextHandler, except it always logs to the testing.Log stream. For case 1, people can just log to a bytes.Buffer and poke around as usual, or maybe there can be a log/slog/testslog that has a handler that just appends slog.Records to a big slice you can look at after the test. |
See also previous discussion |
Yeah, I agree having something like "if this test fails, show me some logs" is a productive idea and different from testing against the contents of logged output. The twist seems like - the way that I had some results decorating a I do think other approaches might need a way to access the result of |
Speak for yourself... |
This proposal has been added to the active column of the proposals project |
Does the In my experience adding the If the goal is to only display log lines when a test fails, I've seen this approach (https://go.dev/play/p/WXHWdRW8s4Z) in a few places: func testLogger(t *testing.T) *slog.Logger {
t.Helper()
buf := new(bytes.Buffer)
opts := &slog.HandlerOptions{AddSource: true}
log := slog.New(slog.NewTextHandler(buf, opts))
t.Cleanup(func() {
if !t.Failed() {
return
}
t.Helper()
t.Log("log output\n", buf.String())
})
return log
} This approach makes the failure message and log lines much more distinct. It also addresses some of the challenges in Maybe |
I'd be totally happy if the output would made to be look different. But I don't think that is possible with the current API. If you log to stdout/stderr, the output looks different, but is then also not correlated with the actual test and interleaved with its
Even if you only get the output of failed tests, that still means if you have more than one of those you have no idea what log output belongs where. And even if you only have one failing test (e.g. by re-running it with So, no. The goal is not only to limit log output to failures - it's also to correctly correlate and interleave it with other test output.
I think both of these would work for me. |
#52751 (comment) would make more third-party (or similarly, deliberately decoupled) solutions possible, as the top post here discusses. |
Looking at the output of https://go.dev/play/p/8s2T3VcEi7C, Looking at that ouptut I realize that stdout is also hidden by
That is already possible today, so that must not be the goal of this proposal. |
For me at least, not when I run it locally. |
What does #52751 provide that isn't already possible with A library like https://github.com/neilotoole/slogt could implement a handler that:
This would match exactly the output proposed in #52751. The problem faced by https://github.com/neilotoole/slogt must not be retrieving the line that called In #52751 (comment) I suggested what was missing was a way to print test output that was indented and displayed in the same place as Looking at the output today (https://go.dev/play/p/ES9_Y5BC5kj), it seems those problems are fixed. It should already be possible to implement a logger that has the behaviour described in https://github.com/neilotoole/slogt#deficiency by writing the log output with: fmt.Printf(" %v: %v\n", filepath.Base(source), logMsg) I believe the formatting of test output has changed a few times. It seems possible it could change again in the future. A |
@Merovius what version of Go? Is that from go1.18? The output you shared is what I remember as well, but I can no longer reproduce with the two latest versions of Go. |
|
TBF, with |
While I think using hooks in |
Let me try to summarize. The request seems to be for a The request isn't merely for structured logging that behaves like The discussion about source locations (file:line information) doesn't seem relevant to this proposal. We already have an accepted proposal, #52751, that adds general source location marking to So if a
The So does that make everyone, as @Merovius says, "happy enough"? |
Writing to os.Stdout does not play well with (the lack of) -v, nor does it play well with parallel tests. That said, I think instead it would be fine to use something like
and then use Perhaps all that is needed is some kind of |
|
If it's a general writer we should probably line-buffer up to some max like 4k to handle Write callers that - unlike the log packages - do not guarantee one line per write. |
Name bikeshed: |
t.Output sounds fine to me. |
Here's a problem: When you use
then all uses of I don't know how annoying or error-prone that's going to be. |
I agree that if you write code to send the log messages to the wrong test, they show up in the wrong test. We can't fix every possible mistake. Writing to os.Stdout definitely does the wrong thing with parallel subtests though, and we can fix that. |
Seems like it's a moving target - while this is probably the wrong thing for tables of unit tests, this seems like the right thing for wider integration tests.
FWIW, I'd mention Kubernetes' ktesting as another interesting exploration of how to integrate |
The current proposal is to add the methods
Writes to
@carlmjohnson, this is your issue. If you're okay with this change, could you modify the issue title, and edit your top post to include the above or link to this comment? |
@ChrisHines I did not know about the exception for |
#43936 asks for the ability to add new JSON directly to the test2json output. |
I'm not sure it needs to look different than TB.Output. A JSON log analyze ought to be able to read JSON nested inside of JSON. |
Maybe to help the log analyzers, add a new action for TB.Output. Something like {
"Time":"2023-07-25T16:54:03.554841-04:00",
"Action":"log",
"Package":"example.com/foo",
"Output":"{\"nested\": true}"
} Instead of "Action": "output". |
Here's an alternative design that aims to satisfy #43936 as well as this issue. If
It is now easy to annotate tests with metadata as #43936 asks for, with code like
To satisfy the needs of this proposal, the logger returned by
|
Trying to satisfy both cases likely makes it not ideal for either. As noted in #43936 (comment) the
Many of the tools that use the Using Both #43936 and this proposal are similar in that they deal with structured data, but otherwise the use cases are quite different. |
So perhaps the dichotomy could be summarized as:
Slog was mentioned in both places... but I think that @dnephin is right here - fundamentally the requirements are different. The intent is to have separate channels for these different types of data. And it's not clear that code under test should have the ability to emit Metadata for the test incidentally by using slog (I would argue that it should not; test Metadata should require using the testing.TB directly because the code should be aware its talking to a test observer... though I'm willing to be wrong here!). So consider my earlier comment a (likely) red herring :) Edit: typo |
I'm more optimistic about |
That suggests calling it |
If we add these it should definitely not be spelled with different casing than the top-level os variables (Stdout and Stderr). |
I don't think we understand what is needed here yet. If people are happy with t.Output, they can do that easily themselves:
Or you could write a slightly longer one that only writes whole lines. It seems hard to believe that we need to add new API just for that. In these meandering discussions, it can be good to return to the top comment, which asked specifically for slog+testing integration. The benefits of that would be that slog's file:line could replace what testing would use, and go test -json could embed slog's JSON value into the event or at least flag it as JSON. Maybe I'm the only one, but these seem like reasonable reasons to do a direct testing+slog integration, in which case the API would be something like
I'm just brainstorming here. Happy to hear other suggestions. |
People have done something like that, but the source location is wrong. |
That example is not how The motivation for
Someone could use The "behaves like
What's the use case for "output as json". I thought that part of the discussion was an attempt to satisfy #43936. The concensus seemed to be that these were actually different use cases and that we shouldn't try to solve both together. |
The suggestion is that
One advantage of I don't think there's a perfect answer, but if we go with
Another possibility is that |
@dnephin, if I understand you right, you are criticizing Russ's claim that people can write But do you have a problem with Russ's actual suggestion, which aligns with the top post, to have a I don't think it's critical that the
where you'd have to parse the output string to recover the key-value pairs, there would be
which is JSON all the way down. |
Is there another possibility, something like passing Currently |
@jba to be honest, I'm a bit confused about why we went back to talking about
First I'd like to challenge the idea that this aligns with the top post. To summarize, the top 2 posts said this about using a logger in tests:
I believe everyone agrees that this case is already handled by buf := new(bytes.Buffer)
logger := slog.New(slog.NewJSONHandler(buf, nil)) // or maybe a custom handler to capture the attributes I think the existence of a
The original problem was the lack of relevant
It seems like it should always be possible to accept an I think you've already found many of the problems with
I didn't realize this originally, but I think this has the same problem as I described in #59928 (comment).
Structured JSON output in the In other words, I don't think the logger should write JSON in the
|
If I understand correctly there are a few options for something like Option 1func (*T) Slog() *slog.Logger (Ignoring the part about "When used with go test -json, any log messages are printed in JSON form.", since I've made an argument against that in the second part of #59928 (comment)). I think #59928 (comment) already calls out the problems with this approach. I'll add that In general I'd probably want to hide the timestamp from this output, so I'd want something like ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr {
// Remove time from the output for predictable test output.
if a.Key == slog.TimeKey {
return slog.Attr{}
}
... But that may not be appropriate in all cases. The lack of extensibility makes this not a good option. A variation on this option from the second post: func (*T) SlogHandler() *slog.Handler I think this has the same problems. Option 2To address those problems the methods could accept handler options. func (*T) Slog(handlerOpts slog.HandlerOptions) *slog.Logger
...
func (*T) SlogHandler(handlerOpts slog.HandlerOptions) *slog.Handler What does the opts := &slog.HandlerOptions{AddSource: true, Level: slog.LevelDebug}
logger := slog.New(slog.NewTextHandler(t.Output(), opts)) If that's the output format it doesn't seem like these new methods on Or would it be more like this?
I mentioned in #59928 (comment), in my experience this makes test failures more difficult to debug because the log output looks too similar to the failure message. This problem is considerably worse when logs are output from a background goroutine, because the failure is not at the end of the output, but somewhere in the middle of many log lines, and they all have a similar prefix. #62728 helps address that problem for Option 3My proposal is this: // Output returns a writter that writes to the test case output, like t.Log does, without any leading
// file:line annotations.
func (*T) Output() *io.Writer
// SlogHandlerOptions returns options appropriate for constructing an slog.Handler. The default
// level is set to slog.LevelDebug, AddSource = true, and slog.TimeKey will be omitted from the
// attributes.
func SlogHandlerOptions() *slog.HandlerOptions This approach makes the common case fairly easy: logger := slog.New(slog.NewTextHandler(t.Output(), testing.SlogHandlerOptions())) while still making it possible to customize the logger: opts := testing.SlogHandlerOptions()
opts.Level = slog.LevelTrace
logger := slog.New(slog.NewTextHandler(t.Output(), opts)) and allowing the "capture logs for comparison" case to look similar: buf := new(bytes.Buffer)
logger := slog.New(slog.NewJSONHandler(buf, testing.SlogHandlerOptions()))
|
I agree with @dnephin. The only thing I’ll add is that besides bytes.Buffer it might be nice to have a slice of slog records to use for test log verification, but that should be a separate proposal. I think t.Output solves this case and leaves things open for whatever future log stuff comes up. |
Logging to os.Stdout mixes up logging in test output, wrap t.Log in an io.Writer interface to avoid that This is not ideal, there are proposals to address this in go: 1. golang/go#22513 2. golang/go#59928
Logging to os.Stdout mixes up logging in test output, wrap t.Log in an io.Writer interface to avoid that This is not ideal, there are proposals to address this in go: 1. golang/go#22513 2. golang/go#59928
Logging to os.Stdout mixes up logging in test output, wrap t.Log in an io.Writer interface to avoid that This is not ideal, there are proposals to address this in go: 1. golang/go#22513 2. golang/go#59928
Logging to os.Stdout mixes up logging in test output, wrap t.Log in an io.Writer interface to avoid that This is not ideal, there are proposals to address this in go: 1. golang/go#22513 2. golang/go#59928
|
In a test, you often want to mock out the logger. It would be nice to be able to call t.Slog() and get a log/slog logger that send output to t.Log() with the correct caller information.
See https://github.com/neilotoole/slogt for an example of a third party library providing this functionality, but note that it cannot provide correct caller information:
It seems like this needs to be done on the Go side to fix the callsite.
The text was updated successfully, but these errors were encountered: