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

Make zaptest.NewTestingWriter public #1399

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

dimmo
Copy link
Contributor

@dimmo dimmo commented Dec 30, 2023

Add more flexibility in configuring zap logger for tests.

The default zapcore.Core, which is created in zaptest.NewLogger() may not be suitable for all use-cases.

func NewLogger(t TestingT, opts ...LoggerOption) *zap.Logger {
...
	return zap.New(
		zapcore.NewCore(
			zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()),
			writer,
			cfg.Level,
		),
		zapOptions...,
	)

E.g., we may need custom encoder or encoder config.

This PR allows us to do such customization:

writer := zaptest.NewTestingWriter(t)
core := zapcore.NewCore(encoder, writer, level)
logger := zap.New(core, zap.AddCaller())

@CLAassistant
Copy link

CLAassistant commented Dec 30, 2023

CLA assistant check
All committers have signed the CLA.

@r-hang
Copy link
Contributor

r-hang commented Jan 2, 2024

As an alternative to making newTestingWriter public would it help to configure zaptest.NewLogger?

func TestConfigure(t *testing.T) {
	opt := zap.WrapCore(func(zapcore.Core) zapcore.Core {
		return zapcore.NewNopCore()
	})
	logger := zaptest.NewLogger(t, zaptest.WrapOptions(opt))
	logger.Info("a")
}

@dimmo
Copy link
Contributor Author

dimmo commented Jan 9, 2024

@r-hang , thanks for your review.
But i can't see the way it helps.

In the NewLogger function, we have,

func NewLogger(t TestingT, opts ...LoggerOption) *zap.Logger {
...
	return zap.New(
		zapcore.NewCore(
			zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()),
			writer,    // <<< we need this writer
			cfg.Level,
		),
		zapOptions...,   // <<< but here whole core will be replaced by zap.WrapCore()
	)
}

zap.WrapCore() just replaces the whole log.core value, and trows away writer, too.
So, we can't combine our custom encoder with a writer which writes to *testing.T.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d27427d) 98.28% compared to head (8dcabdb) 98.42%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1399      +/-   ##
==========================================
+ Coverage   98.28%   98.42%   +0.14%     
==========================================
  Files          53       53              
  Lines        3495     3495              
==========================================
+ Hits         3435     3440       +5     
+ Misses         50       46       -4     
+ Partials       10        9       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@r-hang
Copy link
Contributor

r-hang commented Jan 16, 2024

@dimmo I think I understand. Just to confirm my understanding you could replace the core or constructor a new logger today if you duplicated and hand wrote an implementation for the testingWriter but since we already have one you want to reuse that one.

I ran CI and see a go lint failure. It should be green after updating the exported comment.

@dimmo
Copy link
Contributor Author

dimmo commented Jan 17, 2024

you could replace the core or constructor a new logger today if you duplicated and hand wrote an implementation for the testingWriter but since we already have one you want to reuse that one.

Exactly! Right now I temporarily copied implementation of newTestingWriter(),

	encoder := NewConsoleEncoder(encoderConfig, cfg.vars) // our custom encoder
	level := zap.NewAtomicLevelAt(zapcore.DebugLevel)

	var core zapcore.Core
	if cfg.developMode {
		var writer zapcore.WriteSyncer
		if cfg.t == nil {
			writer = zapcore.Lock(os.Stderr)
		} else {
			writer = newTestingWriter(cfg.t)  // this is a copy!
		}

		if term.IsTerminal(int(os.Stderr.Fd())) {
			core = NewColoredCore(encoder, writer, level)
		} else {
			core = zapcore.NewCore(encoder, writer, level)
		}
	} else {
		core = newJournaldCore(encoder, level)
	}

	logger := zap.New(core, zap.AddCaller())

I ran CI and see a go lint failure. It should be green after updating the exported comment.

Thank you.

@r-hang r-hang merged commit 27b96e3 into uber-go:master Jan 31, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants