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

Jump to bottom

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
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@SamWhited
Copy link
Member

SamWhited commented Oct 31, 2017 •
edited

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

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
Copy link
Member Author

SamWhited commented Nov 6, 2017 •
edited

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

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 as completed Nov 13, 2017
@SamWhited
Copy link
Member Author

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

@ags799
Copy link

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

phinicota commented Aug 18, 2018 •
edited

@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
@poy
testing: add Write method to T and B
2fea5c0 
T and B implement io.Writer so that each can be used to create a logger.

Fixes golang#22513
@poy poy mentioned this issue Nov 13, 2018
Closed
@gopherbot
Copy link

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

@SamWhited SamWhited mentioned this issue Nov 14, 2018
Closed
mmcloughlin added a commit to mmcloughlin/avo that referenced this issue Jan 6, 2019
@mmcloughlin
internal/test: test Logger helper
b6576fe 
Provides *log.Logger and io.Writer that will log to a test object.

See golang/go#22513
Helped with #34
@SamWhited
Copy link
Member Author

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.

@golang golang locked and limited conversation to collaborators May 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

Successfully merging a pull request may close this issue.

testing: add Write method to T and B poy/go
5 participants
@rsc @SamWhited @ags799 @phinicota @gopherbot