Skip to content
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

Open
earthboundkid opened this issue May 2, 2023 · 51 comments
Open

proposal: testing: Add T.Output() etc #59928

earthboundkid opened this issue May 2, 2023 · 51 comments
Labels
Milestone

Comments

@earthboundkid
Copy link
Contributor

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:

Alas, given the available functionality on testing.T (i.e. the Helper method), and how slog is implemented, there's no way to have the correct callsite printed.

It seems like this needs to be done on the Go side to fix the callsite.

@gopherbot gopherbot added this to the Proposal milestone May 2, 2023
@earthboundkid
Copy link
Contributor Author

Expanding on this little, there are two cases of logging during testing:

  1. Sometimes you want to examine the log and ensure that some info is in the log.
  2. Sometimes you just want to provide debug information.

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.

@seankhliao
Copy link
Member

See also previous discussion

#56345 (comment)
#56345 (comment)

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented May 3, 2023

Sometimes you just want to provide debug information.

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 testing observes stack metadata reports lines related to the structure and control flow of running tests. The way that slog observes stack metadata reports where code has decided to emit a slog.Record. We might want both kinds of observations, for example we might have a discrete chunk of test logic where the code under test emits a number of slog.Records worth looking at - we'd want a line annotation for the test logic, and one for each Record.

I had some results decorating a testing.TB with Handler methods and a buffer that would replay slog.Logger calls when t.Error was hit, this kind of thing (https://go.dev/play/p/mPkMxYq2CK__z?v=gotip) ... this approach dictates a particular way of collating slog output via testing.TB hooks, which doesn't feel quite right, however.

I do think other approaches might need a way to access the result of testing.common.frameSkip.

@bitfield
Copy link

bitfield commented May 3, 2023

In a test, you often want to mock out the logger

Speak for yourself...

@rsc
Copy link
Contributor

rsc commented Jun 28, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@dnephin
Copy link
Contributor

dnephin commented Jun 29, 2023

Does the file:line annotation from t.Log help in debugging the test failure? The file:line of the caller should already be available by setting slog.HandlerOptions.AddSource=true.

In my experience adding the file:line annotation from t.Log to log output generally makes the test harder to debug because log output and the failure messages from t.Error/t.Fatal look identical. This is particularly problematic when logs come from a background goroutine and the failure message appears in the middle of the logging output.

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 test2json when tests use t.Parallel. The obvious disadvantage is that log output is no longer streamed.

Maybe t.Slog() could omit the leading file:line annotation? Or alternatively a t.Output() that returns an io.Writer that prints without the file:line annotation could be passed to a slog.Logger?

@Merovius
Copy link
Contributor

Merovius commented Jun 29, 2023

@dnephin

In my experience adding the file:line annotation from t.Log to log output generally makes the test harder to debug because log output and the failure messages from t.Error/t.Fatal look identical.

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 t.Error calls. So you end up with a slodge of logging output, then a list of tests runs and failures.

If the goal is to only display log lines when a test fails

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 -run), you have no idea what log output happens before and which happens after the failure.

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.

Maybe t.Slog() could omit the leading file:line annotation? Or alternatively a t.Output() that returns an io.Writer that prints without the file:line annotation could be passed to a slog.Logger?

I think both of these would work for me.

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Jun 29, 2023

#52751 (comment) would make more third-party (or similarly, deliberately decoupled) solutions possible, as the top post here discusses.

@dnephin
Copy link
Contributor

dnephin commented Jun 30, 2023

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

Looking at the output of https://go.dev/play/p/8s2T3VcEi7C, go test, go test -v, and go test -json all make it clear where the output comes from, for both stdout and t.Log. Stdout interleaved with t.Log is difficult to read, but having worked on projects that use t.Log for logger output, I would argue the file:line annotation from t.Log only makes the cause of the failure harder to find.

Looking at that ouptut I realize that stdout is also hidden by go test when the test passes, so that also can't be the goal of this proposal.

The goal is ... also to correctly correlate and interleave it with other test output.

That is already possible today, so that must not be the goal of this proposal.

@Merovius
Copy link
Contributor

@dnephin

For me at least, not when I run it locally.

2023-06-30-061159_264x330_scrot

@dnephin
Copy link
Contributor

dnephin commented Jun 30, 2023

#52751 (comment) would make more third-party solutions possible, as the top post here discusses.

What does #52751 provide that isn't already possible with slog.HandlerOptions.AddSource=true ?

A library like https://github.com/neilotoole/slogt could implement a handler that:

  • formats the source attr without the directory (see this example).
  • formats the record as ex: main.go:102: time=2023-06-30T00:12:06.256-04:00 level=INFO log output
  • print it with t.Log

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 log.Info, that's already available from slog.HandlerOptions.AddSource=true.

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 t.Log output, but without the standard file:line annotation. I believe at that time (go1.18) the stdout from tests behaved differently (was not hidden when the test passed, and was grouped separately from t.Log output).

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 t.Print or t.Output returning io.Writer might be more reliable.

@dnephin
Copy link
Contributor

dnephin commented Jun 30, 2023

@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.

@Merovius
Copy link
Contributor

go version go1.21rc2 linux/amd64. It's been like this ~forever, for me.

@Merovius
Copy link
Contributor

Merovius commented Jun 30, 2023

TBF, with -v it interleaves the output correctly. I don't think it used to (again, for me, as far as I remember, all that jazz). So maybe that makes me happy enough.

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Jun 30, 2023

What does #52751 provide that isn't already possible with slog.HandlerOptions.AddSource=true ?

While AddSource captures file:line data wherever code under test has chosen to log, #52751 would offer a way to get the file:line of test inputs or other arbitrary points of interest that occur in the testing suite. This information could then be slog.Attr-ized and emitted in slog records (or even baked into handlers).

I think using hooks in testing to trigger emitting logs from code under test is usually plausible, but there are some limitations to this approach. First, the output is no longer consistently structured. Second, it doesn't cover some testing arrangements. For example, if I have routines for test setup and teardown, there might be interesting activity captured in logs but this doesn't neatly line up with the hooks testing provides. It's not that #52751 solves these issues exactly, but more flexibility in obtaining accurate file:line information from the testing suite would be useful; the information would be conspicuously missing otherwise.

@jba
Copy link
Contributor

jba commented Jul 20, 2023

Let me try to summarize.

The request seems to be for a slog.Logger whose output behaves like that of t.Log: it is interleaved properly with the other output from the testing package, and without -v it appears only for failed tests.

The request isn't merely for structured logging that behaves like t.Log, like you could get with something like t.Info(msg, "k1", v1, "k2", v2). I take it no one wants that. For one thing, I don't see why anyone would prefer structured logs to formatted logs for test output, which should be designed for maximum human readability. For another thing, you could get that output with t.Logf("msg=%q k1=%v k2=%v", msg, v1, v2) if you really wanted it. The solution must actually expose a slog.Logger, so it can be injected into the code under test.

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 testing. The slog package already allows you to include, exclude or reformat the source line of the logging call.

So if a slog.Logger with interleaved output is what's wanted, then something like https://go.dev/play/p/nnu-8RLql0X, written without any new API, should almost do it. It will produce interleaved and indented output when run with -v on the current (1.21 prerelease) version of the testing package:

=== RUN   TestTestLogger
    level=INFO msg="info 1"
    t1_test.go:48: t.log
    level=INFO msg="info 2"
    t1_test.go:50: t.log
    level=INFO msg="info 3"
    ...

The -json flag also results in properly interleaved output. Without any flags, the output isn't interleaved, and all of it appears, for passing tests as well as failing ones.

So does that make everyone, as @Merovius says, "happy enough"?

@rsc
Copy link
Contributor

rsc commented Jul 20, 2023

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

type testWriter struct {
   t *testing.T
}
func (w *testWriter) Write(b []byte) (int, error) {
    t.Logf("%s", b)
    return len(b), nil
}

and then use slog.NewTextHandler(&testWriter{t}, nil)

Perhaps all that is needed is some kind of t.LogWriter() io.Writer method, which could be used with log.New, slog.NewTextHandler, and slog.NewJSONHandler.

@jba
Copy link
Contributor

jba commented Jul 20, 2023

t.LogWriter() was suggested upthread as t.Output(). That does seem the simplest and most general idea. Is there a good reason to put Log in the name?

@rsc
Copy link
Contributor

rsc commented Jul 20, 2023

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.

@jba
Copy link
Contributor

jba commented Jul 20, 2023

Name bikeshed: t.Output or t.Writer?

@rsc
Copy link
Contributor

rsc commented Jul 20, 2023

t.Output sounds fine to me.

@jba
Copy link
Contributor

jba commented Jul 20, 2023

Here's a problem: When you use t.Logf or fmt.Fprintf(t.Output(), ...), you'll always get the right output for the test you're in. But if you write

func Test(t *testing.T) {
    logger := slog.New(slog.NewTextHandler(t.Output(), nil))
    ...
}

then all uses of logger, even in subtests, will write to the output for whatever t was when the logger was constructed. Even without -v, you'll see the output for all subtests, both succeeding and failing ones, grouped with the parent test.

I don't know how annoying or error-prone that's going to be.

@rsc
Copy link
Contributor

rsc commented Jul 20, 2023

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.

@AndrewHarrisSPU
Copy link

all uses of logger, even in subtests, will write to the output for whatever t was when the logger was constructed.

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.

So does that make everyone, as @Merovius says, "happy enough"?

FWIW, I'd mention Kubernetes' ktesting as another interesting exploration of how to integrate testing and logging (pre-slog). I think the approach of testing.T.Output as well as #52751 is enough to make some third-party extension like this a lot more reasonable, so of course everyone's happy :)

@jba
Copy link
Contributor

jba commented Jul 21, 2023

The current proposal is to add the methods

package testing
func (*T) Output() io.Writer
func (*B) Output() io.Writer
func (*F) Output() io.Writer

Writes to tbf.Output() will behave like tbf.Log:

  • They will appear along with other output for tbf's test/benchmark/fuzz.
  • Lines will be indented.
  • They will be displayed only for failing tests, unless the -v or -json flags are provided, in which case they will always be displayed.

@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
Copy link
Contributor

Will you also add a new method to the testing.TB interface?

@earthboundkid earthboundkid changed the title proposal: testing: Add log/slog support proposal: testing: Add T.Output() etc Jul 21, 2023
@earthboundkid
Copy link
Contributor Author

I think as long as Output doesn't add line numbers, it's good.

@jba
Copy link
Contributor

jba commented Jul 21, 2023

@ChrisHines I don't see how we could add a method to TB without breaking backwards compatibility.

@ChrisHines
Copy link
Contributor

@jba The authors of testing.TB carved it out of the backwards compatibility contract, see: https://cs.opensource.google/go/go/+/refs/tags/go1.20.6:src/testing/testing.go;drc=38cfb3be9d486833456276777155980d1ec0823e;l=880

	// A private method to prevent users implementing the
	// interface and so future additions to it will not
	// violate Go 1 compatibility.
	private()

As far as I know the only way to implement testing.TB outside of the testing package is to embed it in another type, but in that scenario adding a new method to TB also adds the method to any type that embeds it. :)

@ChrisHines
Copy link
Contributor

ChrisHines commented Jul 21, 2023

To answer the unasked question "why should we add Output" to testing.TB? ...

The practice of writing a logger adapter to forward messages to t.Log is pretty common (I think). It's certainly not new, I've been doing it for years. Our internal logging package (which is basically a blending of uber/zap and go-kit/log) provides just such a wrapper that looks essentially like:

func TestLogger(t testing.TB) Logger {
    f := func(p []byte) (int, error) {
        t.Logf("%s", p)
        return len(p), nil
    }
    testLogger := NewLogger(writerfunc(f))
    return DuplicateKeyDetector(testLogger, t)
}

type writerfunc func([]byte) (int, error)

func (f writerfunc) Write(p []byte) (n int, err error) { return f(p) }

func DuplicateKeyDetector(next Logger, t testing.TB) Logger { ... }

With the current proposal we could delete the local f function and writerFunc type and just pass t.Output directly to NewLogger. If the method is added to testing.TB this would not require a local interface declaration and type assertion.

Also of note: DuplicateKeyDetector fails any test for code that tries to log a record with conflicting log keys. The test code for DuplicateKeyDetector mocks testing.TB as seen below. This code would not be broken by adding a new method to testing.TB.

type mockTBInterface struct {
	ErrorStrings []string
	testing.TB
}

func (m *mockTBInterface) Errorf(format string, args ...any) {
	m.ErrorStrings = append(m.ErrorStrings, fmt.Sprintf(format, args...))
}

@riannucci
Copy link

This may be a similar/overlapping proposal with #43936 where slog output on testing was suggested as a possible way to accomplish test metadata output.

@jba
Copy link
Contributor

jba commented Jul 21, 2023

@ChrisHines I did not know about the exception for testing.TB. I guess we would add TB.Output then.

@rsc
Copy link
Contributor

rsc commented Jul 25, 2023

#43936 asks for the ability to add new JSON directly to the test2json output.
Instead of new API in testing it seems like some connection to slog would make more sense.
But that connection would look differently from just using TB.Output, so maybe we should rethink this part.

@earthboundkid
Copy link
Contributor Author

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.

@earthboundkid
Copy link
Contributor Author

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".

@jba
Copy link
Contributor

jba commented Jul 26, 2023

Here's an alternative design that aims to satisfy #43936 as well as this issue.

If t is a testing.T (or B or F), then t.Slog() returns a *slog.Logger. The logger's handler cannot be modified. Its output is coordinated with the rest of the test output. It has no minimum level—all log events are output. It uses the TextHandler format normally, or JSON if the -json flag is present, in which case it appears like

{"action": "log", "record": {"message": "debugging  info", "level":"INFO", "requestID": 123}}

It is now easy to annotate tests with metadata as #43936 asks for, with code like

t.Slog().Info("debugging info",  "requestID", 123)

To satisfy the needs of this proposal, the logger returned by t.Slog() can be passed directly to the code under test. Or the handler can be wrapped to set a minimum level, modify the record, or whatever else is desired, by writing

logger := slog.New(newWrappingHandler(t.Slog().Handler()))

@dnephin
Copy link
Contributor

dnephin commented Jul 26, 2023

Trying to satisfy both cases likely makes it not ideal for either.

As noted in #43936 (comment) the testing package doesn't emit JSON directly, so t.Slog() would not be able to emit JSON. It would have to emit some logging format that can be parsed by cmd/test2json. Can the TextHandler output be easily and unambiguously parsed into JSON? How would these lines be distinguished from the existing text output that uses === RUN, etc for framing of events?

A tool attempting to use this new action=log output for metadata would need to be concerned about every possible attribute used by any library that emits logs to slog.Logger. At the very least anyone trying to use it for metadata would have to construct long unique keys to ensure that some library doesn't use the same attribute key. A t.Metadata wouldn't have these problems because only the test cases and maybe a few testing helpers would have access to t.Metadata.
Edit: I guess this is already true for any library using slog, and a convention of a test.metadata prefix for keys would be easy enough.

Many of the tools that use the -json output today (IDEs, CI systems, etc) use the run/pass/fail actions to learn about the tests, but will generally print the TestEvent.Output field of action=output events directly to the user. Today logs from a test run would be included in action=output events. When a user switches to t.Slog() the logs will be output from an action=log event. At the very least the TestEvent.Output field of these events should contain the text output of the log. Even then, if the tool that's consuming these events is strict about the actions, there's a reasonable chance it will discard these new events with action=log instead of treating them like action=output. That seems like it could be considered a breaking change. An action=metadata doesn't have this problem because test metadata isn't an important part of the test output. A system reading the test2json output only needs to be concerned about the metadata it knows how to consume, it wouldn't need to be updated to maintain correct behaviour.

Using t.Slog to emit metadata is not different from what is possible today. A test author can already emit JSON and the system that wants to read that metadata can decode the JSON from the Output field.

Both #43936 and this proposal are similar in that they deal with structured data, but otherwise the use cases are quite different.

@riannucci
Copy link

riannucci commented Jul 26, 2023

So perhaps the dichotomy could be summarized as:

  1. There is a need to be able to associate Stdout (and Stderr) of the code under test with the test case itself... short of backwards incompatible changes to how os.Stdout/Stderr work, the next best thing would be to expose output object(s) directly on testing.TB which can be passed to the code under test. The intent would be to allow this output from the code under test to display with the actual test output, ideally without breaking existing tools reading this via test2json (and also having a chance to work correctly with parallel tests!)
  2. There is a need to be able to have tests emit Metadata to a compatible reader; ideally this has a different "channel" than raw output to allow out of band communication between the test and some test observer.

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

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Jul 26, 2023

I'm more optimistic about t.Slog when thinking about a tool like jq reprocessing output. I also wonder if the TestEvent struct defined in cmd/internal/test2json is like helper-aware file:line stuff - is it unwise to expose these details? TestEvent could be in testing, along with a way to define a callback for when a TestEvent is generated.

@earthboundkid
Copy link
Contributor Author

2. There is a need to be able to have tests emit Metadata to a compatible reader; ideally this has a different "channel" than raw output to allow out of band communication between the test and some test observer.

That suggests calling it TB.StdOut() and TB.StdErr().

@rsc
Copy link
Contributor

rsc commented Aug 2, 2023

If we add these it should definitely not be spelled with different casing than the top-level os variables (Stdout and Stderr).

@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

I don't think we understand what is needed here yet. If people are happy with t.Output, they can do that easily themselves:

type testWriter struct {
   t *testing.T
}
func (w *testWriter) Write(b []byte) (int, error) {
    t.Helper()
    t.Logf("%s", b)
    return len(b), nil
}
func TestOutput(t *testing.T) { return &testWriter{t} }

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

// Slog returns a structured logger that writes to the test case output, like t.Log does.
// When used with go test -json, any log messages are printed in JSON form.
// A TestEvent corresponding to a log message has OutputType "slog", and the Output field contains the JSON text.
func (*T) Slog() *slog.Logger

I'm just brainstorming here. Happy to hear other suggestions.

@jba
Copy link
Contributor

jba commented Nov 2, 2023

If people are happy with t.Output, they can do that easily themselves...

People have done something like that, but the source location is wrong.

@dnephin
Copy link
Contributor

dnephin commented Nov 2, 2023

If people are happy with t.Output, they can do that easily themselves

That example is not how t.Output was proposed to work.

The motivation for t.Output is to have output that behaves like t.Log without the file:line prefix that is added by t.Log. There are a few cases where the prefixed file:line is undesirable.

  1. it makes the actual test failure more difficult to find when all of the log output lines look the same as the test failure messages (because they all have file:line prefix)
  2. when using slog, you only care about the file:line where the log was emitted, which slog already provides with AddSource=true. The prefixed file:line of some random test helper, an unrelated line where the logger happened to be created in a test, or an internal stdlib package is not helpful.

Someone could use slog with os.Stdout, the problem with that approach is shown here: #59928 (comment). go test will print that output instead of hiding it.

The "behaves like t.Log" means that go test should hide the output in passing tests, instead of showing it. That would allow someone to trivially create an slog logger using t.Output , while still giving them control over other logger settings.

// A TestEvent corresponding to a log message has OutputType "slog", and the Output field contains the JSON text

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.

@jba
Copy link
Contributor

jba commented Dec 26, 2023

The suggestion is that t.Slog returns a logger with a special slog.Handler that behaves like t.Log. That raises some questions.

  • What is the minimum level of the handler? The default handlers use Info, but we are logging failed tests, so perhaps Debug makes more sense. But then logs at lower levels wouldn't appear, and they might be helpful.

  • One aspect of slog's flexibility is that an application can provide a handler that processes some values specially. For example, an application might log values of type func() T, and its handler might call those functions just before it outputs the log event. That sort of lazy evaluation is not part of slog, so special functionality like this wouldn't be available to the testing handler.

One advantage of t.Output is that it's just an io.Writer, so the application could use its own handler with it. But that has its problems too. Perhaps the application handler doesn't take an io.Writer, or maybe it outputs a binary format.

I don't think there's a perfect answer, but if we go with t.Slog then the user can always wrap the testing handler with their own:

type myTestingHandler struct {
   h slog.Handler
}

func newTestingLogger(t *testing.T) *slog.Logger {
    return slog.New(&myTestingHandler{h: t.Slog.Handler()})
}

// Enabled enforces a minimum level of Debug-10. It does not call h.h.Enabled,
// so whatever minimum level the testing handler uses is ignored.
func (h *myTestingHandler) Enabled(ctx context.Context, level slog.Level) bool {...}

// Handle replaces attribute values of type `func() T`  with the result of invoking the function,
// then calls h.h.Handle.
func (h *myTestingHandler) Handle(ctx context.Context, r slog.Record) error {...}

Another possibility is that t.Slog could take a slog.HandlerOptions, which would make it easy to set a minimum level and perform some special actions, including the lazy evaluation I described above.

@jba
Copy link
Contributor

jba commented Dec 27, 2023

@dnephin, if I understand you right, you are criticizing Russ's claim that people can write t.Output themselves. We agree they cannot because of the source location.

But do you have a problem with Russ's actual suggestion, which aligns with the top post, to have a t.Slog method that behaves like t.Log in the relevant ways (no output for passing tests, etc.)?

I don't think it's critical that the -json flag results in JSON output, but it could make life easier for the consumer of the output. Instead of a line like

{"Time":"...","Action":"slog","Package":"p","Test":"t","Output":"msg=hello k1=1 k2=2"}

where you'd have to parse the output string to recover the key-value pairs, there would be

{"Time":"...","Action":"slog","Package":"p","Test":"t","Output":{"msg":"hello","k1":1,"k2":2}}

which is JSON all the way down.

@AndrewHarrisSPU
Copy link

Another possibility is that t.Slog could take a slog.HandlerOptions, which would make it easy to set a minimum level and perform some special actions, including the lazy evaluation I described above.

Is there another possibility, something like passing t.Slog a slog Handler, and generating log lines for significant testing events from the t (or derived t, etc.)?

Currently testing.TB implementations are a bit privileged - source location in relation to testing.TB.Helper is the obvious one. Resolving this would require greater access to some the internal implementation, or testing.TB implementations could learn to generate slog logging lines. The latter seems messy but finite - I think the destination would be another testing type along the lines of T or B that also knows how to slog.

@dnephin
Copy link
Contributor

dnephin commented Dec 31, 2023

@jba to be honest, I'm a bit confused about why we went back to talking about t.Slog. I reread this entire comment history in an attempt to understand. In June through August it seems like everyone was happy with testing.T.Output() io.Writer. What changed?

But do you have a problem with Russ's actual suggestion, which aligns with the top post, to have a t.Slog method that behaves like t.Log in the relevant ways (no output for passing tests, etc.)?

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:

  1. Sometimes you want to examine the log and ensure that some info is in the log.

I believe everyone agrees that this case is already handled by bytes.Buffer. I think this is relevant to the proposal because some tests should explicitly not use an API like t.Slog, and should always use something like:

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 t.Slog would make this pattern (using a bytes.Buffer) more difficult to discover.

  1. Sometimes you just want to provide debug information. This [proposal] is just about [this case].

The original problem was the lack of relevant file:line information, but I think I showed in #59928 (comment) that the problem isn't a lack of file:line info (that's already available from slog.HandlerOptions.AddSource=true). The problem is actually that t.Log adds an unnecessary and misleading file:line for an irrelevant source location.

testing.T.Output() io.Writer seems like the obvious solution to not only this problem, but a number of other problems I've had in the past when trying to output more detailed failure messages. I believe that was confirmed in #59928 (comment) - "I think as long as Output doesn't add line numbers, it's good."

Perhaps the application handler doesn't take an io.Writer, or maybe it outputs a binary format.

It seems like it should always be possible to accept an io.Writer. If the application accepts a higher level interface like slog.Handler or slog.Logger you can easily construct those from an io.Writer. I don't understand why someone would print a binary format to the test output. I'd expect that to go to a bytes.Buffer, or to a file.

I think you've already found many of the problems with func (*T) Slog() *slog.Logger. A func (*T) Slog() *slog.Handler was proposed in the second post. I think that's slightly better, but I don't understand why that would be preferred over t.Output given that both would require a line of code to construct the actual logger. I'd like to explore that a bit more, but first a response to another point.


I don't think it's critical that the -json flag results in JSON output, but it could make life easier for the consumer of the output.

{"Time":"...","Action":"slog","Package":"p","Test":"t","Output":{"msg":"hello","k1":1,"k2":2}}

I didn't realize this originally, but I think this has the same problem as I described in #59928 (comment).

Many of the tools that use the -json output today (IDEs, CI systems, etc) use the run/pass/fail actions to learn about the tests, but will generally print the TestEvent.Output field of action=output events directly to the user.

Structured JSON output in the Output field breaks the most common use case for test2json output. Any tools reading this now have to convert the JSON back into the text format (whatever was parsed by test2json).

In other words, I don't think the logger should write JSON in the Output field, as #59928 (comment) says, whatever is reading the Output can handle decoding the log. That shouldn't be a problem.

go test and go test -v should output a text format, so either way something needs to convert the text format into JSON, but I don't think we have seen any use cases for JSON in the Output field.

@dnephin
Copy link
Contributor

dnephin commented Dec 31, 2023

If I understand correctly there are a few options for something like t.Slog:

Option 1

func (*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 2

To 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 go test and go test -v output look like for this logger? Is is equivalent to:

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 T add much.

Or would it be more like this?

source.go:512: level=INFO msg="something happened" key=value

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 test2json output, but I think the problem remains for the standard text format.

Option 3

My 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()))

SlogHandlerOptions does not really need to be in the testing package. It could be copied around initially. It should be under 10 lines.

@earthboundkid
Copy link
Contributor Author

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.

serprex added a commit to PeerDB-io/peerdb that referenced this issue Jan 31, 2024
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
serprex added a commit to PeerDB-io/peerdb that referenced this issue Jan 31, 2024
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
serprex added a commit to PeerDB-io/peerdb that referenced this issue Jan 31, 2024
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
pankaj-peerdb pushed a commit to PeerDB-io/peerdb that referenced this issue Feb 6, 2024
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
@stevenh
Copy link
Contributor

stevenh commented Feb 17, 2024

t.Output gives much more flexibility for other use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Active
Development

No branches or pull requests