Skip to content

Conversation

NastyaAR
Copy link

This problem occured due to parallel reading of os.Stderr (logger.Initialize ->logger.InitializeWithEnv -> zap.Config.Build) and writing to it when intercepting stderr in TestFindClientConfigs.

Changes

  • added InitializeTest() for creating zaptest/observer in-memory logs
  • fixed TestFindClientConfigs()

 * new function for initialize test logger

Signed-off-by: NastyaAR <[email protected]>
Copy link
Member

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @NastyaAR!
The overall approach looks good, I've left a few comments inline.

return unstructuredLogs
}

func InitializeTest() *observer.ObservedLogs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only used in config_test.go let's make it a test helper in that file.


logger := zap.New(core)
zap.ReplaceGlobals(logger)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a cleanup step here that restores the global logger

  	originalLogger := zap.L()
  	zap.ReplaceGlobals(testLogger)

  	t.Cleanup(func() {
  		zap.ReplaceGlobals(originalLogger)
  	})

}

func TestFindClientConfigs(t *testing.T) {
t.Parallel()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot run this test in parallel with others because it replaces the global logger. If multiple tests do this, it's not clear which logger will be used.

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

Successfully merging this pull request may close these issues.

2 participants