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 log.Logger adapter to testing.T #22513

Closed
SamWhited opened this issue Oct 31, 2017 · 8 comments
Closed

proposal: testing: add log.Logger adapter to testing.T #22513

SamWhited opened this issue Oct 31, 2017 · 8 comments
Labels
Milestone

Comments

@SamWhited
Copy link
Member

@SamWhited SamWhited commented Oct 31, 2017

When working on applications I often find myself copy pasting some boilerplate similar to the following into my tests:

func testLogger(t *testing.T) *log.Logger {
	return log.New(testWriter{t}, "test", log.LstdFlags)
}

type testWriter struct {
	t *testing.T
}

func (tw testWriter) Write(p []byte) (n int, err error) {
	tw.t.Log(string(p))
	return len(p), nil
}

then when I test functions in my application I can pass in the testing logger and not end up with lots of logging unless the tests failed (in which case the logs show up in a more-or-less sane way and I only see logs generated by the specific tests that failed).

I would like to propose adding this to the testing package as a method on T:

// Logger returns a new logger that logs output using c.Print.
func (c *T) Logger() *log.Logger {}

Alternatively, making *T a writer would require no new dependencies and be more generic but would require a bit more boiler plate for the test writer:

// Write satisfies the io.Writer interface for T that "writes" to t.Print.
// Write always returns len(p), nil.
func (c *T) Write(p []byte) (n int, err error) {}

If accepted, this adds one method to type T which would need to be covered under the compatibility promise.

@gopherbot gopherbot added this to the Proposal milestone Oct 31, 2017
@gopherbot gopherbot added the Proposal label Oct 31, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2017

Does someone have a list of libraries etc that take a *log.Logger? It would be more compelling if there were many such packages that this would help.

As an aside, it would be nice if testing.T and log.Logger both implemented some common interface that code could be written to use, so that you could pass t instead of a logger and not need this adapter. But testing.T calls the logging Logf and log.Logger calls it Printf. (They both agree on a Fatalf.)

@rsc rsc changed the title proposal: add testing.T / log.Logger adapter to the testing package proposal: testing: add log.Logger adapter to testing.T Nov 6, 2017
@SamWhited

This comment has been minimized.

Copy link
Member Author

@SamWhited SamWhited commented Nov 6, 2017

Does someone have a list of libraries etc that take a *log.Logger? It would be more compelling if there were many such packages that this would help.

I am not very skilled at BigTable but maybe someone with more monthly minutes than I could run something over the GitHub dataset to find out (or come up with a query that I can run once). I know @campoy isn't on the team anymore, but I remember him being good at this, maybe he'd be willing to advise on the best way to do this and I can try to run the query?

In the standard library there are at least:

  • "net/http".Server.ErrorLogger
  • "net/http/httputil".ReverseProxy.ErrorLogger
  • "net/http/cgi".Handler.Logger

As an aside, it would be nice if testing.T and log.Logger both implemented some common interface that code could be written to use, so that you could pass t instead of a logger and not need this adapter. But testing.T calls the logging Logf and log.Logger calls it Printf. (They both agree on a Fatalf.)

Agreed. Having testing.T also have a Printf method might not be the end of the world, although it does feel redundant even if we were to say that the old one is deprecated. This wouldn't work for the cases in the standard library that already take a log.Logger though.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 13, 2017

Adding a new dependency into testing is a big step, one that we're not ready for. In particular there are a few things about log.Logger that we'd change if we could, and maybe one would be arranging that it had a useful method in common with testing.T, so that an interface would be usable here. Maybe there should even be an io.Logfer or something like that, but there is not today (and as noted above testing.T and log.Logger have no useful methods in common).

@rsc rsc closed this Nov 13, 2017
@SamWhited

This comment has been minimized.

Copy link
Member Author

@SamWhited SamWhited commented Nov 13, 2017

What about making testing.T an io.Writer which would allow it to be passed in when constructing a logger?

@ags799

This comment has been minimized.

Copy link

@ags799 ags799 commented Apr 19, 2018

Hey @SamWhited 😊. I like your idea about an io.Writer for testing.T. For example, when I test command-line applications, I would like to be able to set os.exec.Cmd.Stdout to a test logger.

@phinicota

This comment has been minimized.

Copy link

@phinicota phinicota commented Aug 18, 2018

@SamWhited @ags799 just came in looking exactly for that!

edit: this might help: https://golang.org/pkg/testing/iotest/#NewWriteLogger

poy added a commit to poy/go that referenced this issue Nov 13, 2018
T and B implement io.Writer so that each can be used to create a logger.

Fixes golang#22513
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 13, 2018

Change https://golang.org/cl/149379 mentions this issue: testing: add Write method to T and B

mmcloughlin added a commit to mmcloughlin/avo that referenced this issue Jan 6, 2019
Provides *log.Logger and io.Writer that will log to a test object.

See golang/go#22513
Helped with #34
@SamWhited

This comment has been minimized.

Copy link
Member Author

@SamWhited SamWhited commented May 29, 2019

For anyone finding this proposal via internet search in the future, I got tired of constantly copy/pasting something like this into every project I start and threw up a package here:

https://godoc.org/code.soquee.net/testlog

Unfortunately, it requires maintaining another external dependency for something that's relatively simple, so it may still be best to copy paste, but since I personally was doing it heavily and kept having to copy/paste minor fixes and changes around I decided it was best for me to have an external dependency instead. Hopefully this is useful for someone else visiting this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.