testing: de-globalize discard loggers

[jmank88]

What does this do / why do we need it?

This change replaces global discard Logger variables with per-test instances.

We've developed a habit of using consolidated global vars to share discardLoggers between tests (log.New(ioutil.Discard, "", 0)). This seems logical since calls are effectively no-ops, but the internals of the Logger are still locking to serialize writes to ioutil.Discard, which creates unnecessary coupling and synchronization between tests that could otherwise be completely independent and/or parallel.

[sdboyer]

let's have a helper func to do it, just so that the intent and pattern is entirely clear. makes it easy, too - just turn var discardLogger into func discardLogger() *log.Logger

[jmank88]

let's have a helper func to do it, just so that the intent and pattern is entirely clear. makes it easy, too - just turn var discardLogger into func discardLogger() *log.Logger

Done

[carolynvs]

What do you think about moving this to internal/test and exporting? That way it doesn't need to be defined.

[sdboyer]

I'm kinda meh about that - tbh I was kinda meh about my original suggestion, but the idea of exporting it just makes it seem definitively unwise 😛

[carolynvs]

I don't care much about if we do it or not but out of curiosity what's wrong/unwise with exporting test.DiscardLogger() in our internal test package?

[sdboyer]

it's a really subjective thing, which is why i sorta hesitate about it in the first place, but to my mind, exporting jumps the line between an innocuous helper and a recommended pattern.

[ibrasho]

LGTM