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

Specify a default LoggerProvider for tests. #39

Open
jamesross3 opened this issue Mar 22, 2019 · 3 comments
Open

Specify a default LoggerProvider for tests. #39

jamesross3 opened this issue Mar 22, 2019 · 3 comments

Comments

@jamesross3
Copy link
Contributor

jamesross3 commented Mar 22, 2019

Log output can be helpful in diagnosing test failures, but witchcraft-go-logging requires a LoggerProvider to be set to actually emit useful logs. One workaround to this is including either of the methods specified below to set a LoggerProvider within a test package:

func TestMain(m *testing.M) {
    wlog.SetDefaultLoggerProvider(wlogzap.LoggerProvider())
    os.Exit(m.Run())
}

func init() {
    wlog.SetDefaultLoggerProvider(wlogzap.LoggerProvider())
}

This is cumbersome, since every test package would require setting this. Furthermore, the go community seems to have inconsistencies regarding test package naming - for a package foo, tests can be written either in foo or foo_test. I believe this means that the init/TestMain method approach above could actually clobber another init method's SetDefaultLoggerProvider call (not sure what guarantees go makes about ordering init methods across packages).

Proposed solution:
Add an init method to witchcraft-go-logging that checks if we're running in a test and, if this is the case, set some LoggerProvider:

func init() {
  executable, err := os.Executable()
  if err != nil {
    // log or just return
  }
  // see https://golang.org/cmd/go/#hdr-Testing_flags
  if strings.HasSuffix(executable, ".test") {
    wlog.SetDefaultLoggerProvider(loggerProviderBackedByStdLibraryLogger)
  }
}

@bmoylan @nmiyake thoughts? I am happy to contribute this as well as a stdlog LoggerProvider implementation.

@nmiyake
Copy link
Contributor

nmiyake commented Mar 28, 2019

Hey @jamesross3 , sorry for the late reply on this.

I'm against this for a few reasons:

  1. "Regular" code should have any special-case logic that changes logic based on whether or not it is being run as part of a test. Even the task of determining whether or not the code is running as part of a test is fairly non-standard/weird (as evidenced by your sample code)
  2. Tests should generally not need logger output. Ideally, a successful test should run and complete without printing anything to stdout/stderr etc. If a test wants to assert something about the log content, then the test itself should be responsible for setting up its logger accordingly

It is a bit unfortunate that the "default" for tests will print the warning and also print output to stdout rather than not printing at all, but I think that's the best trade-off to make at the moment. Also, although not ideal, the TestMain approach does allow this to be controlled at the test package level as well.

@bmoylan
Copy link
Contributor

bmoylan commented Mar 28, 2019

Tests should generally not need logger output.

Given that logging is intended to assist debugging, I think having log output available for test failures is actually quite important.

@nmiyake
Copy link
Contributor

nmiyake commented Mar 28, 2019

I think this is pretty context-dependent and kind of gets into the spectrum of unit vs. integration testing etc.

Ideally, most "unit" tests should be quick to run and verify, and if they fail it should make the reason quite clear. If a failure could use more context from debugging, then I would model "turning on logging" as similar to adding Printf statements to debug: something that should be done locally when debugging/diagnosing, but not run continuously in the course of regular tests.

There may certainly be tests that are an exception to this (a particularly flaky test that is hard to track down, an integration test where you want the full regular lifecycle including logging, etc.), but I do believe that these should generally be exceptions rather than the rule.

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

No branches or pull requests

3 participants