Use a test-logger in tests rather than stdout #3976
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meiliang86
reviewed
Feb 10, 2021
common/log/loggerimpl/logger.go
Outdated
| @@ -52,11 +53,7 @@ func NewNopLogger() log.Logger { | |||
|
|
|||
| // NewDevelopmentForTest is a helper to create new development logger in unit test | |||
| func NewDevelopmentForTest(s suite.Suite) log.Logger { | |||
There was a problem hiding this comment.
nit: change name to NewLoggerForTest?
| @@ -51,7 +52,9 @@ func TestWorkflowTestSuite(t *testing.T) { | |||
| } | |||
|
|
|||
| func (s *workflowTestSuite) SetupTest() { | |||
| s.SetLogger(zaptest.NewLogger(s.T())) | |||
There was a problem hiding this comment.
Any reason we are not changing the default here?
There was a problem hiding this comment.
I think that's the right choice, yeah. Among possibly other locations - any calls to zap.New... in pretty much any go code anywhere are immediately suspect IMO.
There was a problem hiding this comment.
hm. unfortunately adding it there is somewhere between "not trivial" and "requires breaking changes". there doesn't appear to be any *testing.T available at that point, and if I start changing code to require it, I eventually end up at public APIs.
still very much worthwhile, but tbh I think it's worth tackling separately.
github-actions
bot
force-pushed
the
logs
branch
from
ab71f97
to
2ffb303
Compare
meiliang86
approved these changes
Feb 17, 2021
Currently, on failure, e.g. matchingEngine_test.go tests spew out *ridiculous* amounts of text. This happens because Go cannot tell which logs are related to the failed test(s), so it does what it can: show all stdout. Unfortunately, stdout blends logs from parallel tests, and it's hard or impossible to find the ones relevant to the failure simply by reading. So: stop doing that. Zap has a `zaptest` package for this purpose. I'm not sure how many other places may need to be changed, but this helps the ones I'm looking at at the moment.
github-actions
bot
force-pushed
the
logs
branch
from
d874b23
to
f31b7cb
Compare
yux0
pushed a commit
to yux0/cadence
that referenced
this pull request
May 4, 2021
Currently, on failure, e.g. matchingEngine_test.go tests spew out *ridiculous* amounts of text. This happens because Go cannot tell which logs are related to the failed test(s), so it does what it can: show all stdout. Unfortunately, stdout blends logs from parallel tests, and it's hard or impossible to find the ones relevant to the failure simply by reading. So: stop doing that. Zap has a `zaptest` package for this purpose. I'm not sure how many other places may need to be changed, but this helps the ones I'm looking at at the moment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, on failure, e.g.
matchingEngine_test.gotests spew out ridiculous amounts of text.This happens because Go cannot tell which logs are related to the failed test(s), so it does what it can: show all stdout.
Unfortunately, stdout blends logs from parallel tests (I don't believe it's even line-synchronized), and it's hard or impossible to find the ones relevant to the failure simply by reading.
So: stop doing that. Zap has a
zaptestpackage for this purpose, we just have to use it.For tests using the workflow suite, there's an annoying call-order dependency that means we have to
suite.SetLoggerbeforesuite.NewTestWorkflowEnvironmentor it's ignored... but that's not actually hard to deal with. I just wish we (and our users) didn't have to.I'm not sure how many other places may need to be changed, but this helps the ones I'm looking at at the moment.