diff --git a/.golangci.yml b/.golangci.yml index 154951275..fbc6df790 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -9,7 +9,7 @@ linters: disable-all: true enable: # golangci-lint defaults: - # - errcheck TODO: enable errcheck + - errcheck - gosimple - govet - ineffassign @@ -21,7 +21,7 @@ linters: - nolintlint # lints nolint directives - revive -linter-settings: +linters-settings: govet: # These govet checks are disabled by default, but they're useful. enable: @@ -30,6 +30,24 @@ linter-settings: - sortslice - unusedwrite + errcheck: + exclude-functions: + # These methods can not fail. + # They operate on an in-memory buffer. + - (*go.uber.org/zap/buffer.Buffer).Write + - (*go.uber.org/zap/buffer.Buffer).WriteByte + - (*go.uber.org/zap/buffer.Buffer).WriteString + + - (*go.uber.org/zap/zapio.Writer).Close + - (*go.uber.org/zap/zapio.Writer).Sync + - (*go.uber.org/zap/zapio.Writer).Write + # Write to zapio.Writer cannot fail, + # so io.WriteString on it cannot fail. + - io.WriteString(*go.uber.org/zap/zapio.Writer) + + # Writing a plain string to a fmt.State cannot fail. + - io.WriteString(fmt.State) + issues: # Print all issues reported by all linters. max-issues-per-linter: 0 @@ -51,3 +69,9 @@ issues: # for foo() { } - linters: [revive] text: 'empty-block: this block is empty, you can remove it' + + # Ignore logger.Sync() errcheck failures in example_test.go + # since those are intended to be uncomplicated examples. + - linters: [errcheck] + path: example_test.go + text: 'Error return value of `logger.Sync` is not checked' diff --git a/benchmarks/scenario_bench_test.go b/benchmarks/scenario_bench_test.go index b41b1b50e..4b867fa91 100644 --- a/benchmarks/scenario_bench_test.go +++ b/benchmarks/scenario_bench_test.go @@ -104,7 +104,6 @@ func BenchmarkDisabledWithoutFields(b *testing.B) { } }) }) - } func BenchmarkDisabledAccumulatedContext(b *testing.B) { @@ -183,7 +182,6 @@ func BenchmarkDisabledAccumulatedContext(b *testing.B) { } }) }) - } func BenchmarkDisabledAddingFields(b *testing.B) { @@ -253,7 +251,6 @@ func BenchmarkDisabledAddingFields(b *testing.B) { } }) }) - } func BenchmarkWithoutFields(b *testing.B) { @@ -323,7 +320,9 @@ func BenchmarkWithoutFields(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - logger.Log(getMessage(0), getMessage(1)) + if err := logger.Log(getMessage(0), getMessage(1)); err != nil { + b.Fatal(err) + } } }) }) @@ -401,7 +400,6 @@ func BenchmarkWithoutFields(b *testing.B) { } }) }) - } func BenchmarkAccumulatedContext(b *testing.B) { @@ -471,7 +469,9 @@ func BenchmarkAccumulatedContext(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - logger.Log(getMessage(0), getMessage(1)) + if err := logger.Log(getMessage(0), getMessage(1)); err != nil { + b.Fatal(err) + } } }) }) @@ -531,7 +531,6 @@ func BenchmarkAccumulatedContext(b *testing.B) { } }) }) - } func BenchmarkAddingFields(b *testing.B) { @@ -592,7 +591,9 @@ func BenchmarkAddingFields(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - logger.Log(fakeSugarFields()...) + if err := logger.Log(fakeSugarFields()...); err != nil { + b.Fatal(err) + } } }) }) @@ -643,5 +644,4 @@ func BenchmarkAddingFields(b *testing.B) { } }) }) - } diff --git a/encoder_test.go b/encoder_test.go index f6be665b1..b71eb654b 100644 --- a/encoder_test.go +++ b/encoder_test.go @@ -41,7 +41,7 @@ func TestRegisterEncoder(t *testing.T) { func TestDuplicateRegisterEncoder(t *testing.T) { testEncoders(func() { - RegisterEncoder("foo", newNilEncoder) + assert.NoError(t, RegisterEncoder("foo", newNilEncoder), "expected to be able to register the encoder foo") assert.Error(t, RegisterEncoder("foo", newNilEncoder), "expected an error when registering an encoder with the same name twice") }) } @@ -52,7 +52,7 @@ func TestRegisterEncoderNoName(t *testing.T) { func TestNewEncoder(t *testing.T) { testEncoders(func() { - RegisterEncoder("foo", newNilEncoder) + assert.NoError(t, RegisterEncoder("foo", newNilEncoder), "expected to be able to register the encoder foo") encoder, err := newEncoder("foo", zapcore.EncoderConfig{}) assert.NoError(t, err, "could not create an encoder for the registered name foo") assert.Nil(t, encoder, "the encoder from newNilEncoder is not nil") diff --git a/error.go b/error.go index 38cb768de..45f7b838d 100644 --- a/error.go +++ b/error.go @@ -61,9 +61,12 @@ func (errs errArray) MarshalLogArray(arr zapcore.ArrayEncoder) error { // allocating, pool the wrapper type. elem := _errArrayElemPool.Get() elem.error = errs[i] - arr.AppendObject(elem) + err := arr.AppendObject(elem) elem.error = nil _errArrayElemPool.Put(elem) + if err != nil { + return err + } } return nil } diff --git a/error_test.go b/error_test.go index 64dab191c..4bfa370d2 100644 --- a/error_test.go +++ b/error_test.go @@ -95,3 +95,39 @@ func TestErrorsArraysHandleRichErrors(t *testing.T) { require.True(t, ok, "Expected serialized error to be a map, got %T.", serialized) assert.Equal(t, "egad", errMap["error"], "Unexpected standard error string.") } + +func TestErrArrayBrokenEncoder(t *testing.T) { + t.Parallel() + + failWith := errors.New("great sadness") + err := (brokenArrayObjectEncoder{ + Err: failWith, + ObjectEncoder: zapcore.NewMapObjectEncoder(), + }).AddArray("errors", errArray{ + errors.New("foo"), + errors.New("bar"), + }) + require.Error(t, err, "Expected error from broken encoder.") + assert.ErrorIs(t, err, failWith, "Unexpected error.") +} + +// brokenArrayObjectEncoder is an ObjectEncoder +// that builds a broken ArrayEncoder. +type brokenArrayObjectEncoder struct { + zapcore.ObjectEncoder + zapcore.ArrayEncoder + + Err error // error to return +} + +func (enc brokenArrayObjectEncoder) AddArray(key string, marshaler zapcore.ArrayMarshaler) error { + return enc.ObjectEncoder.AddArray(key, + zapcore.ArrayMarshalerFunc(func(ae zapcore.ArrayEncoder) error { + enc.ArrayEncoder = ae + return marshaler.MarshalLogArray(enc) + })) +} + +func (enc brokenArrayObjectEncoder) AppendObject(zapcore.ObjectMarshaler) error { + return enc.Err +} diff --git a/http_handler.go b/http_handler.go index 632b6831a..2be8f6515 100644 --- a/http_handler.go +++ b/http_handler.go @@ -69,6 +69,13 @@ import ( // // curl -X PUT localhost:8080/log/level -H "Content-Type: application/json" -d '{"level":"debug"}' func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if err := lvl.serveHTTP(w, r); err != nil { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "internal error: %v", err) + } +} + +func (lvl AtomicLevel) serveHTTP(w http.ResponseWriter, r *http.Request) error { type errorResponse struct { Error string `json:"error"` } @@ -80,19 +87,20 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodGet: - enc.Encode(payload{Level: lvl.Level()}) + return enc.Encode(payload{Level: lvl.Level()}) + case http.MethodPut: requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r) if err != nil { w.WriteHeader(http.StatusBadRequest) - enc.Encode(errorResponse{Error: err.Error()}) - return + return enc.Encode(errorResponse{Error: err.Error()}) } lvl.SetLevel(requestedLvl) - enc.Encode(payload{Level: lvl.Level()}) + return enc.Encode(payload{Level: lvl.Level()}) + default: w.WriteHeader(http.StatusMethodNotAllowed) - enc.Encode(errorResponse{ + return enc.Encode(errorResponse{ Error: "Only GET and PUT are supported.", }) } @@ -129,5 +137,4 @@ func decodePutJSON(body io.Reader) (zapcore.Level, error) { return 0, errors.New("must specify logging level") } return *pld.Level, nil - } diff --git a/http_handler_test.go b/http_handler_test.go index 9fa9c64c1..9da3dc7b5 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -22,6 +22,7 @@ package zap_test import ( "encoding/json" + "errors" "net/http" "net/http/httptest" "strings" @@ -167,7 +168,9 @@ func TestAtomicLevelServeHTTP(t *testing.T) { res, err := http.DefaultClient.Do(req) require.NoError(t, err, "Error making %s request.", req.Method) - defer res.Body.Close() + defer func() { + assert.NoError(t, res.Body.Close(), "Error closing response body.") + }() require.Equal(t, tt.expectedCode, res.StatusCode, "Unexpected status code.") if tt.expectedCode != http.StatusOK { @@ -188,3 +191,27 @@ func TestAtomicLevelServeHTTP(t *testing.T) { }) } } + +func TestAtomicLevelServeHTTPBrokenWriter(t *testing.T) { + t.Parallel() + + lvl := zap.NewAtomicLevel() + + request, err := http.NewRequest(http.MethodGet, "http://localhost:1234/log/level", nil) + require.NoError(t, err, "Error constructing request.") + + recorder := httptest.NewRecorder() + lvl.ServeHTTP(&brokenHTTPResponseWriter{ + ResponseWriter: recorder, + }, request) + + assert.Equal(t, http.StatusInternalServerError, recorder.Code, "Unexpected status code.") +} + +type brokenHTTPResponseWriter struct { + http.ResponseWriter +} + +func (w *brokenHTTPResponseWriter) Write([]byte) (int, error) { + return 0, errors.New("great sadness") +} diff --git a/logger.go b/logger.go index 32d737276..e9b49cc9f 100644 --- a/logger.go +++ b/logger.go @@ -374,7 +374,7 @@ func (log *Logger) check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { if stack.Count() == 0 { if log.addCaller { fmt.Fprintf(log.errorOutput, "%v Logger.check error: failed to get caller\n", ent.Time.UTC()) - log.errorOutput.Sync() + _ = log.errorOutput.Sync() } return ce } diff --git a/sink.go b/sink.go index 478c9a10f..499772a00 100644 --- a/sink.go +++ b/sink.go @@ -66,7 +66,8 @@ func newSinkRegistry() *sinkRegistry { factories: make(map[string]func(*url.URL) (Sink, error)), openFile: os.OpenFile, } - sr.RegisterSink(schemeFile, sr.newFileSinkFromURL) + // Infallible operation: the registry is empty, so we can't have a conflict. + _ = sr.RegisterSink(schemeFile, sr.newFileSinkFromURL) return sr } @@ -154,7 +155,7 @@ func (sr *sinkRegistry) newFileSinkFromPath(path string) (Sink, error) { case "stderr": return nopCloserSink{os.Stderr}, nil } - return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0666) + return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666) } func normalizeScheme(s string) (string, error) { diff --git a/stacktrace_ext_test.go b/stacktrace_ext_test.go index 71f098333..9f018aa9b 100644 --- a/stacktrace_ext_test.go +++ b/stacktrace_ext_test.go @@ -97,7 +97,7 @@ func TestStacktraceFiltersVendorZap(t *testing.T) { testDir := filepath.Join(goPath, "src/go.uber.org/zap_test/") vendorDir := filepath.Join(testDir, "vendor") - require.NoError(t, os.MkdirAll(testDir, 0777), "Failed to create source director") + require.NoError(t, os.MkdirAll(testDir, 0o777), "Failed to create source director") curFile := getSelfFilename(t) setupSymlink(t, curFile, filepath.Join(testDir, curFile)) @@ -175,7 +175,7 @@ func getSelfFilename(t *testing.T) string { func setupSymlink(t *testing.T, src, dst string) { // Make sure the destination directory exists. - os.MkdirAll(filepath.Dir(dst), 0777) + require.NoError(t, os.MkdirAll(filepath.Dir(dst), 0o777)) // Get absolute path of the source for the symlink, otherwise we can create a symlink // that uses relative paths. diff --git a/writer.go b/writer.go index 625b06760..06768c679 100644 --- a/writer.go +++ b/writer.go @@ -62,7 +62,7 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) { closers := make([]io.Closer, 0, len(paths)) closeAll := func() { for _, c := range closers { - c.Close() + _ = c.Close() } } diff --git a/writer_test.go b/writer_test.go index b74345567..20e00b74b 100644 --- a/writer_test.go +++ b/writer_test.go @@ -91,7 +91,6 @@ func TestOpen(t *testing.T) { } assert.True(t, fileExists(tempName)) - os.Remove(tempName) } func TestOpenPathsNotFound(t *testing.T) { @@ -255,7 +254,8 @@ func TestOpenWithErroringSinkFactory(t *testing.T) { func TestCombineWriteSyncers(t *testing.T) { tw := &testWriter{"test", t} w := CombineWriteSyncers(tw) - w.Write([]byte("test")) + _, err := w.Write([]byte("test")) + assert.NoError(t, err, "Unexpected write error.") } func fileExists(name string) bool { diff --git a/zapcore/buffered_write_syncer_bench_test.go b/zapcore/buffered_write_syncer_bench_test.go index 1e3db59f1..56ad5f2c6 100644 --- a/zapcore/buffered_write_syncer_bench_test.go +++ b/zapcore/buffered_write_syncer_bench_test.go @@ -40,11 +40,15 @@ func BenchmarkBufferedWriteSyncer(b *testing.B) { w := &BufferedWriteSyncer{ WS: AddSync(file), } - defer w.Stop() + defer func() { + assert.NoError(b, w.Stop(), "failed to stop buffered write syncer") + }() b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) diff --git a/zapcore/buffered_write_syncer_test.go b/zapcore/buffered_write_syncer_test.go index 8a36ad69d..d0f6037af 100644 --- a/zapcore/buffered_write_syncer_test.go +++ b/zapcore/buffered_write_syncer_test.go @@ -101,7 +101,8 @@ func TestBufferWriter(t *testing.T) { n, err := ws.Write([]byte("foo")) require.NoError(t, err, "Unexpected error writing to WriteSyncer.") require.Equal(t, 3, n, "Wrote an unexpected number of bytes.") - ws.Write([]byte("foo")) + _, err = ws.Write([]byte("foo")) + assert.Error(t, err, "Expected error writing to WriteSyncer.") assert.Error(t, ws.Stop(), "Expected stop to fail.") }) diff --git a/zapcore/core.go b/zapcore/core.go index 9dfd64051..776e93f6f 100644 --- a/zapcore/core.go +++ b/zapcore/core.go @@ -102,9 +102,9 @@ func (c *ioCore) Write(ent Entry, fields []Field) error { return err } if ent.Level > ErrorLevel { - // Since we may be crashing the program, sync the output. Ignore Sync - // errors, pending a clean solution to issue #370. - c.Sync() + // Since we may be crashing the program, sync the output. + // Ignore Sync errors, pending a clean solution to issue #370. + _ = c.Sync() } return nil } diff --git a/zapcore/core_test.go b/zapcore/core_test.go index 1311097cc..e3186311a 100644 --- a/zapcore/core_test.go +++ b/zapcore/core_test.go @@ -148,7 +148,7 @@ func TestIOCoreSyncsOutput(t *testing.T) { DebugLevel, ) - core.Write(tt.entry, nil) + assert.NoError(t, core.Write(tt.entry, nil), "Unexpected error writing entry.") assert.Equal(t, tt.shouldSync, sink.Called(), "Incorrect Sync behavior.") } } diff --git a/zapcore/encoder_test.go b/zapcore/encoder_test.go index c0dbc5bb1..9b8142f5d 100644 --- a/zapcore/encoder_test.go +++ b/zapcore/encoder_test.go @@ -285,10 +285,11 @@ func TestEncoderConfiguration(t *testing.T) { }, extra: func(enc Encoder) { enc.AddTime("extra", _epoch) - enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { + err := enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { enc.AppendTime(_epoch) return nil })) + assert.NoError(t, err) }, expectedJSON: `{"L":"info","T":"1970-01-01 00:00:00 +0000 UTC","N":"main","C":"foo.go:42","F":"foo.Foo","M":"hello","extra":"1970-01-01 00:00:00 +0000 UTC","extras":["1970-01-01 00:00:00 +0000 UTC"],"S":"fake-stack"}` + "\n", expectedConsole: "1970-01-01 00:00:00 +0000 UTC\tinfo\tmain\tfoo.go:42\tfoo.Foo\thello\t" + // plain-text preamble @@ -313,10 +314,11 @@ func TestEncoderConfiguration(t *testing.T) { }, extra: func(enc Encoder) { enc.AddDuration("extra", time.Second) - enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { + err := enc.AddArray("extras", ArrayMarshalerFunc(func(enc ArrayEncoder) error { enc.AppendDuration(time.Minute) return nil })) + assert.NoError(t, err) }, expectedJSON: `{"L":"info","T":0,"N":"main","C":"foo.go:42","F":"foo.Foo","M":"hello","extra":"1s","extras":["1m0s"],"S":"fake-stack"}` + "\n", expectedConsole: "0\tinfo\tmain\tfoo.go:42\tfoo.Foo\thello\t" + // preamble @@ -720,10 +722,11 @@ func TestNameEncoders(t *testing.T) { func assertAppended(t testing.TB, expected interface{}, f func(ArrayEncoder), msgAndArgs ...interface{}) { mem := NewMapObjectEncoder() - mem.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error { + err := mem.AddArray("k", ArrayMarshalerFunc(func(arr ArrayEncoder) error { f(arr) return nil })) + assert.NoError(t, err, msgAndArgs...) arr := mem.Fields["k"].([]interface{}) require.Equal(t, 1, len(arr), "Expected to append exactly one element to array.") assert.Equal(t, expected, arr[0], msgAndArgs...) diff --git a/zapcore/entry.go b/zapcore/entry.go index 059844f92..459a5d7ce 100644 --- a/zapcore/entry.go +++ b/zapcore/entry.go @@ -242,7 +242,7 @@ func (ce *CheckedEntry) Write(fields ...Field) { // CheckedEntry is being used after it was returned to the pool, // the message may be an amalgamation from multiple call sites. fmt.Fprintf(ce.ErrorOutput, "%v Unsafe CheckedEntry re-use near Entry %+v.\n", ce.Time, ce.Entry) - ce.ErrorOutput.Sync() + _ = ce.ErrorOutput.Sync() // ignore error } return } @@ -254,7 +254,7 @@ func (ce *CheckedEntry) Write(fields ...Field) { } if err != nil && ce.ErrorOutput != nil { fmt.Fprintf(ce.ErrorOutput, "%v write error: %v\n", ce.Time, err) - ce.ErrorOutput.Sync() + _ = ce.ErrorOutput.Sync() // ignore error } hook := ce.after diff --git a/zapcore/entry_ext_test.go b/zapcore/entry_ext_test.go new file mode 100644 index 000000000..fd1a05cb5 --- /dev/null +++ b/zapcore/entry_ext_test.go @@ -0,0 +1,55 @@ +// Copyright (c) 2023 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package zapcore_test + +import ( + "bytes" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest" +) + +func TestCheckedEntryIllegalReuse(t *testing.T) { + t.Parallel() + + var errOut bytes.Buffer + + testCore := zaptest.NewLogger(t).Core() + ce := testCore.Check(zapcore.Entry{ + Level: zapcore.InfoLevel, + Time: time.Now(), + Message: "hello", + }, nil) + ce.ErrorOutput = zapcore.AddSync(&errOut) + + // The first write should succeed. + ce.Write(zap.String("k", "v"), zap.Int("n", 42)) + assert.Empty(t, errOut.String(), "Expected no errors on first write.") + + // The second write should fail. + ce.Write(zap.String("foo", "bar"), zap.Int("x", 1)) + assert.Contains(t, errOut.String(), "Unsafe CheckedEntry re-use near Entry", + "Expected error logged on second write.") +} diff --git a/zapcore/error.go b/zapcore/error.go index c67dd71df..c40df1326 100644 --- a/zapcore/error.go +++ b/zapcore/error.go @@ -98,8 +98,11 @@ func (errs errArray) MarshalLogArray(arr ArrayEncoder) error { } el := newErrArrayElem(errs[i]) - arr.AppendObject(el) + err := arr.AppendObject(el) el.Free() + if err != nil { + return err + } } return nil } diff --git a/zapcore/error_test.go b/zapcore/error_test.go index d8263abc1..c5d61b040 100644 --- a/zapcore/error_test.go +++ b/zapcore/error_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/assert" "go.uber.org/multierr" + "go.uber.org/zap/zapcore" . "go.uber.org/zap/zapcore" ) @@ -161,3 +162,49 @@ func TestRichErrorSupport(t *testing.T) { f.AddTo(enc) assert.Equal(t, "failed: egad", enc.Fields["k"], "Unexpected basic error message.") } + +func TestErrArrayBrokenEncoder(t *testing.T) { + t.Parallel() + + f := Field{ + Key: "foo", + Type: ErrorType, + Interface: multierr.Combine( + errors.New("foo"), + errors.New("bar"), + ), + } + + failWith := errors.New("great sadness") + enc := NewMapObjectEncoder() + f.AddTo(brokenArrayObjectEncoder{ + Err: failWith, + ObjectEncoder: enc, + }) + + // Failure to add the field to the encoder + // causes the error to be added as a string field. + assert.Equal(t, "great sadness", enc.Fields["fooError"], + "Unexpected error message.") +} + +// brokenArrayObjectEncoder is an ObjectEncoder +// that builds a broken ArrayEncoder. +type brokenArrayObjectEncoder struct { + ObjectEncoder + ArrayEncoder + + Err error // error to return +} + +func (enc brokenArrayObjectEncoder) AddArray(key string, marshaler ArrayMarshaler) error { + return enc.ObjectEncoder.AddArray(key, + ArrayMarshalerFunc(func(ae ArrayEncoder) error { + enc.ArrayEncoder = ae + return marshaler.MarshalLogArray(enc) + })) +} + +func (enc brokenArrayObjectEncoder) AppendObject(zapcore.ObjectMarshaler) error { + return enc.Err +} diff --git a/zapcore/json_encoder_bench_test.go b/zapcore/json_encoder_bench_test.go index bcb5a0133..ef8001993 100644 --- a/zapcore/json_encoder_bench_test.go +++ b/zapcore/json_encoder_bench_test.go @@ -31,10 +31,13 @@ import ( func BenchmarkJSONLogMarshalerFunc(b *testing.B) { for i := 0; i < b.N; i++ { enc := NewJSONEncoder(testEncoderConfig()) - enc.AddObject("nested", ObjectMarshalerFunc(func(enc ObjectEncoder) error { + err := enc.AddObject("nested", ObjectMarshalerFunc(func(enc ObjectEncoder) error { enc.AddInt64("i", int64(i)) return nil })) + if err != nil { + b.Fatal(err) + } } } @@ -95,7 +98,9 @@ func BenchmarkStandardJSON(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - json.Marshal(record) + if _, err := json.Marshal(record); err != nil { + b.Fatal(err) + } } }) } diff --git a/zapcore/json_encoder_impl_test.go b/zapcore/json_encoder_impl_test.go index fde241f56..5fc7b4db2 100644 --- a/zapcore/json_encoder_impl_test.go +++ b/zapcore/json_encoder_impl_test.go @@ -249,7 +249,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { desc: "object (no nested namespace)", expected: `"obj":{"obj-out":"obj-outside-namespace"},"not-obj":"should-be-outside-obj"`, f: func(e Encoder) { - e.AddObject("obj", maybeNamespace{false}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{false})) e.AddString("not-obj", "should-be-outside-obj") }, }, @@ -257,7 +257,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { desc: "object (with nested namespace)", expected: `"obj":{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"not-obj":"should-be-outside-obj"`, f: func(e Encoder) { - e.AddObject("obj", maybeNamespace{true}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{true})) e.AddString("not-obj", "should-be-outside-obj") }, }, @@ -265,7 +265,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { desc: "multiple open namespaces", expected: `"k":{"foo":1,"middle":{"foo":2,"inner":{"foo":3}}}`, f: func(e Encoder) { - e.AddObject("k", ObjectMarshalerFunc(func(enc ObjectEncoder) error { + err := e.AddObject("k", ObjectMarshalerFunc(func(enc ObjectEncoder) error { e.AddInt("foo", 1) e.OpenNamespace("middle") e.AddInt("foo", 2) @@ -273,6 +273,7 @@ func TestJSONEncoderObjectFields(t *testing.T) { e.AddInt("foo", 3) return nil })) + assert.NoError(t, err) }, }, } @@ -289,10 +290,11 @@ func TestJSONEncoderTimeFormats(t *testing.T) { f := func(e Encoder) { e.AddTime("k", date) - e.AddArray("a", ArrayMarshalerFunc(func(enc ArrayEncoder) error { + err := e.AddArray("a", ArrayMarshalerFunc(func(enc ArrayEncoder) error { enc.AppendTime(date) return nil })) + assert.NoError(t, err) } tests := []struct { desc string @@ -420,7 +422,7 @@ func TestJSONEncoderArrays(t *testing.T) { desc: "object (no nested namespace) then string", expected: `[{"obj-out":"obj-outside-namespace"},"should-be-outside-obj",{"obj-out":"obj-outside-namespace"},"should-be-outside-obj"]`, f: func(arr ArrayEncoder) { - arr.AppendObject(maybeNamespace{false}) + assert.NoError(t, arr.AppendObject(maybeNamespace{false})) arr.AppendString("should-be-outside-obj") }, }, @@ -428,7 +430,7 @@ func TestJSONEncoderArrays(t *testing.T) { desc: "object (with nested namespace) then string", expected: `[{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj",{"obj-out":"obj-outside-namespace","obj-namespace":{"obj-in":"obj-inside-namespace"}},"should-be-outside-obj"]`, f: func(arr ArrayEncoder) { - arr.AppendObject(maybeNamespace{true}) + assert.NoError(t, arr.AppendObject(maybeNamespace{true})) arr.AppendString("should-be-outside-obj") }, }, @@ -530,10 +532,13 @@ type turducken struct{} func (t turducken) MarshalLogObject(enc ObjectEncoder) error { return enc.AddArray("ducks", ArrayMarshalerFunc(func(arr ArrayEncoder) error { for i := 0; i < 2; i++ { - arr.AppendObject(ObjectMarshalerFunc(func(inner ObjectEncoder) error { + err := arr.AppendObject(ObjectMarshalerFunc(func(inner ObjectEncoder) error { inner.AddString("in", "chicken") return nil })) + if err != nil { + return err + } } return nil })) diff --git a/zapcore/level_test.go b/zapcore/level_test.go index ab97c98d0..d8eb96292 100644 --- a/zapcore/level_test.go +++ b/zapcore/level_test.go @@ -159,7 +159,7 @@ func TestLevelNils(t *testing.T) { }, "Level(nil).String() should panic") assert.Panics(t, func() { - l.MarshalText() + _, _ = l.MarshalText() // should panic }, "Expected to panic when marshalling a nil level.") err := l.UnmarshalText([]byte("debug")) diff --git a/zapcore/memory_encoder_test.go b/zapcore/memory_encoder_test.go index 052bdb0c6..d5f215fb6 100644 --- a/zapcore/memory_encoder_test.go +++ b/zapcore/memory_encoder_test.go @@ -218,7 +218,7 @@ func TestMapObjectEncoderAdd(t *testing.T) { desc: "object (no nested namespace) then string", f: func(e ObjectEncoder) { e.OpenNamespace("k") - e.AddObject("obj", maybeNamespace{false}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{false})) e.AddString("not-obj", "should-be-outside-obj") }, expected: map[string]interface{}{ @@ -232,7 +232,7 @@ func TestMapObjectEncoderAdd(t *testing.T) { desc: "object (with nested namespace) then string", f: func(e ObjectEncoder) { e.OpenNamespace("k") - e.AddObject("obj", maybeNamespace{true}) + assert.NoError(t, e.AddObject("obj", maybeNamespace{true})) e.AddString("not-obj", "should-be-outside-obj") }, expected: map[string]interface{}{ @@ -255,6 +255,7 @@ func TestMapObjectEncoderAdd(t *testing.T) { }) } } + func TestSliceArrayEncoderAppend(t *testing.T) { tests := []struct { desc string @@ -284,29 +285,33 @@ func TestSliceArrayEncoderAppend(t *testing.T) { {"AppendUint8", func(e ArrayEncoder) { e.AppendUint8(42) }, uint8(42)}, {"AppendUintptr", func(e ArrayEncoder) { e.AppendUintptr(42) }, uintptr(42)}, { - desc: "AppendReflected", - f: func(e ArrayEncoder) { e.AppendReflected(map[string]interface{}{"foo": 5}) }, + desc: "AppendReflected", + f: func(e ArrayEncoder) { + assert.NoError(t, e.AppendReflected(map[string]interface{}{"foo": 5})) + }, expected: map[string]interface{}{"foo": 5}, }, { desc: "AppendArray (arrays of arrays)", f: func(e ArrayEncoder) { - e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + err := e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { inner.AppendBool(true) inner.AppendBool(false) return nil })) + assert.NoError(t, err) }, expected: []interface{}{true, false}, }, { desc: "object (no nested namespace) then string", f: func(e ArrayEncoder) { - e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { - inner.AppendObject(maybeNamespace{false}) + err := e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + err := inner.AppendObject(maybeNamespace{false}) inner.AppendString("should-be-outside-obj") - return nil + return err })) + assert.NoError(t, err) }, expected: []interface{}{ map[string]interface{}{ @@ -318,11 +323,12 @@ func TestSliceArrayEncoderAppend(t *testing.T) { { desc: "object (with nested namespace) then string", f: func(e ArrayEncoder) { - e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { - inner.AppendObject(maybeNamespace{true}) + err := e.AppendArray(ArrayMarshalerFunc(func(inner ArrayEncoder) error { + err := inner.AppendObject(maybeNamespace{true}) inner.AppendString("should-be-outside-obj") - return nil + return err })) + assert.NoError(t, err) }, expected: []interface{}{ map[string]interface{}{ diff --git a/zapcore/tee_test.go b/zapcore/tee_test.go index b2b9c9dc0..f6be13316 100644 --- a/zapcore/tee_test.go +++ b/zapcore/tee_test.go @@ -120,7 +120,7 @@ func TestTeeWrite(t *testing.T) { debugEntry := Entry{Level: DebugLevel, Message: "log-at-debug"} warnEntry := Entry{Level: WarnLevel, Message: "log-at-warn"} for _, ent := range []Entry{debugEntry, warnEntry} { - tee.Write(ent, nil) + assert.NoError(t, tee.Write(ent, nil)) } for _, logs := range []*observer.ObservedLogs{debugLogs, warnLogs} { diff --git a/zapcore/write_syncer_bench_test.go b/zapcore/write_syncer_bench_test.go index db6ec4523..90ae47558 100644 --- a/zapcore/write_syncer_bench_test.go +++ b/zapcore/write_syncer_bench_test.go @@ -24,6 +24,7 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap/internal/ztest" ) @@ -37,7 +38,9 @@ func BenchmarkMultiWriteSyncer(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) @@ -51,7 +54,9 @@ func BenchmarkMultiWriteSyncer(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) @@ -64,11 +69,15 @@ func BenchmarkMultiWriteSyncer(b *testing.B) { &ztest.Discarder{}, ), } - defer w.Stop() + defer func() { + assert.NoError(b, w.Stop(), "Unexpected error stopping buffered write syncer.") + }() b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) @@ -83,7 +92,9 @@ func BenchmarkWriteSyncer(b *testing.B) { b.ResetTimer() b.RunParallel(func(pb *testing.PB) { for pb.Next() { - w.Write([]byte("foobarbazbabble")) + if _, err := w.Write([]byte("foobarbazbabble")); err != nil { + b.Fatal(err) + } } }) }) diff --git a/zapcore/write_syncer_test.go b/zapcore/write_syncer_test.go index 4748be7f5..c0c2698e9 100644 --- a/zapcore/write_syncer_test.go +++ b/zapcore/write_syncer_test.go @@ -70,7 +70,7 @@ func TestNewMultiWriteSyncerWorksForSingleWriter(t *testing.T) { ws := NewMultiWriteSyncer(w) assert.Equal(t, w, ws, "Expected NewMultiWriteSyncer to return the same WriteSyncer object for a single argument.") - ws.Sync() + assert.NoError(t, ws.Sync(), "Expected Sync to succeed.") assert.True(t, w.Called(), "Expected Sync to be called on the created WriteSyncer") } diff --git a/zaptest/observer/observer_test.go b/zaptest/observer/observer_test.go index 2a901b1ce..0a57a0f32 100644 --- a/zaptest/observer/observer_test.go +++ b/zaptest/observer/observer_test.go @@ -173,7 +173,7 @@ func TestFilters(t *testing.T) { logger, sink := New(zap.InfoLevel) for _, log := range logs { - logger.Write(log.Entry, log.Context) + assert.NoError(t, logger.Write(log.Entry, log.Context), "Unexpected error writing log entry.") } tests := []struct {