-
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 |
This comment has been minimized.
This comment has been minimized.
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? |
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
|
Thanks for the summary, @dnephin.
With I can see a few advantages to built-in support of The Writer returned by |
That would require somehow exposing |
I do hesitate to add this since I note that this proposal already has at least a couple counterproposals in the comments, but getting log output integrated nicely with tests has been a pain point for me for a long time and so I'm pretty invested in something happening here. I'd personally find it useful to be able to attribute a log line to some specific part of a test, since in the system I maintain in my day job our tests often call a variety of functions that all generate quite a large amount of log information. Therefore while I do like the When considering the discussion above, I had the following idea... The I imagine a new method on A new package-level function in package testing
func (c *T) LocationContext(ctx context.Context) context.Context
func (c *B) LocationContext(ctx context.Context) context.Context
func LocationFromContext(ctx context.Context) (TestLocation, bool)
type TestLocation struct {
Filename string
Line int
}
func (l TestLocation) String() string {
// (returns the same filename:line format that t.Log would normally use as a prefix)
} When a test calls a function that takes a func TestExample(t *testing.T) {
ctx := context.Background()
// ...
got, err := ThingBeingTested(t.LocationContext(ctx))
// ...
} A hypothetical This exploits the ability for a Whether the standard library should then provide this hypothetical Footnotes
|
@apparentlymart, I think your You could get very close to what you want today, by writing a |
Fair enough! I'd considered what I proposed as filling the gap left by the Since this issue presumably already has an audience of people who are invested in some sort of integration between |
If I understand correctly, the current proposal includes indenting the output, but what are the indentation rules? fmt.Fprint(t.Output(), "foo")
t.Log("bar") is the output
or
I recently had another use for t.Output: I wanted to run x/tools/go/analysis passes over the module's source code, and report the lines of the analysis result, not the testing setup (related #71478) |
@seankhliao I would have That is, So the output would be
However, |
I like that general idea of That seems more aligned with likely author intent -- at least for me, I always intend for a test log item to stand alone relative to anything else I might be emitting -- and would encourage the system to produce more intelligible output even if, for example, a quirk of the test code or the code under test causes there to be an incomplete line left in the buffer just before a I can't personally think of any strong justification for starting a line with a write to |
@apparentlymart That works for me. It's not that |
I assume use of |
Yes, it should. |
Change https://go.dev/cl/646956 mentions this issue: |
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: