diff --git a/config.go b/config.go index a198f16ea..cdd251981 100644 --- a/config.go +++ b/config.go @@ -217,13 +217,11 @@ func (cfg Config) buildOptions(errSink zapcore.WriteSyncer) []Option { func (cfg Config) openSinks() (zapcore.WriteSyncer, zapcore.WriteSyncer, error) { sink, closeOut, err := Open(cfg.OutputPaths...) if err != nil { - closeOut() return nil, nil, err } - errSink, closeErr, err := Open(cfg.ErrorOutputPaths...) + errSink, _, err := Open(cfg.ErrorOutputPaths...) if err != nil { closeOut() - closeErr() return nil, nil, err } return sink, errSink, nil diff --git a/config_test.go b/config_test.go index 65bcfb112..65cabdfd7 100644 --- a/config_test.go +++ b/config_test.go @@ -84,3 +84,25 @@ func TestConfig(t *testing.T) { }) } } + +func TestConfigWithInvalidPaths(t *testing.T) { + tests := []struct { + desc string + output string + errOutput string + }{ + {"output directory doesn't exist", "/tmp/not-there/foo.log", "stderr"}, + {"error output directory doesn't exist", "stdout", "/tmp/not-there/foo-errors.log"}, + {"neither output directory exists", "/tmp/not-there/foo.log", "/tmp/not-there/foo-errors.log"}, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + cfg := NewProductionConfig() + cfg.OutputPaths = []string{tt.output} + cfg.ErrorOutputPaths = []string{tt.errOutput} + _, err := cfg.Build() + assert.Error(t, err, "Expected an error opening a non-existent directory.") + }) + } +} diff --git a/writer.go b/writer.go index 238ca6f36..de39e5992 100644 --- a/writer.go +++ b/writer.go @@ -49,6 +49,11 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) { var errs multierror.Error writers := make([]zapcore.WriteSyncer, 0, len(paths)) files := make([]*os.File, 0, len(paths)) + close := func() { + for _, f := range files { + f.Close() + } + } for _, path := range paths { switch path { case "stdout": @@ -67,12 +72,13 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) { files = append(files, f) } } - close := func() { - for _, f := range files { - f.Close() - } + + if err := errs.AsError(); err != nil { + close() + return writers, nil, err } - return writers, close, errs.AsError() + + return writers, close, nil } // CombineWriteSyncers combines multiple WriteSyncers into a single, locked diff --git a/writer_test.go b/writer_test.go index 9cc3e775d..1293832ab 100644 --- a/writer_test.go +++ b/writer_test.go @@ -66,7 +66,10 @@ func TestOpen(t *testing.T) { for _, tt := range tests { wss, cleanup, err := open(tt.paths) - defer cleanup() + if err == nil { + defer cleanup() + } + if tt.error == "" { assert.NoError(t, err, "Unexpected error opening paths %v.", tt.paths) } else { @@ -82,6 +85,25 @@ func TestOpen(t *testing.T) { } } +func TestOpenFails(t *testing.T) { + tests := []struct { + paths []string + }{ + { + paths: []string{"./non-existent-dir/file"}, + }, + { + paths: []string{"stdout", "./non-existent-dir/file"}, + }, + } + + for _, tt := range tests { + _, cleanup, err := Open(tt.paths...) + require.Nil(t, cleanup, "Cleanup function should never be nil") + assert.Error(t, err, "Open with non-existent directory should fail") + } +} + type testWriter struct { expected string t testing.TB