issues/413: Use testing/slogtest

CHANGELOG.md

@@ -9,6 +9,7 @@ Most recent version is listed first.
 - Replace math/rand with math/rand/v2: https://github.com/komuw/ong/pull/411
 - ong/id: Improve id generation: https://github.com/komuw/ong/pull/415
                                  https://github.com/komuw/ong/pull/416
+- ong/log: Improve conformance with slog.Handler interface: https://github.com/komuw/ong/pull/417
 
 # v0.0.90
 - ong/middleware: a http subdomain should be redirected to the same subdomain at https: https://github.com/komuw/ong/pull/406

log/log.go

@@ -95,6 +95,11 @@ type handler struct {
 	cBuf *circleBuf
 	// +checklocks:mu
 	logID string
+
+	// forceFlush is only used for test purposes.
+	// Specifically, it is for use with `testing/slogtest`
+	// See: https://github.com/golang/go/issues/61706#issuecomment-1674413648
+	forceFlush bool
 }
 
 func newHandler(ctx context.Context, w io.Writer, maxSize int) slog.Handler {
@@ -104,8 +109,17 @@ func newHandler(ctx context.Context, w io.Writer, maxSize int) slog.Handler {
 		ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr {
 			if a.Key == slog.SourceKey {
 				if t, ok := a.Value.Any().(*slog.Source); ok {
-					// log the source in one line.
-					return slog.String(a.Key, fmt.Sprintf("%s:%d", t.File, t.Line))
+					// t.Line is zero if `slog.Record.PC` is zero.
+					// And the slog docs says the source attribute should be ignored in such cases.
+					// Additionally `testing/slogtest` enforces that behaviour.
+					//
+					// todo: There must be a better way of checking if `slog.Record.PC` is zero
+					//       rather than doing it in ReplaceAttr, I think it should be done in `handler.Handler()`
+					//       Research how stdlib does it and implement it that way.
+					if t.Line != 0 {
+						// log the source in one line.
+						return slog.String(a.Key, fmt.Sprintf("%s:%d", t.File, t.Line))
+					}
 				}
 			}
 			return a
@@ -134,16 +148,18 @@ func (h *handler) WithAttrs(attrs []slog.Attr) slog.Handler {
 	h.mu.Lock()
 	cBuf := h.cBuf
 	id := h.logID
+	forceFlush := h.forceFlush
 	h.mu.Unlock()
-	return &handler{wrappedHandler: h.wrappedHandler.WithAttrs(attrs), mu: h.mu, cBuf: cBuf, logID: id}
+	return &handler{wrappedHandler: h.wrappedHandler.WithAttrs(attrs), mu: h.mu, cBuf: cBuf, logID: id, forceFlush: forceFlush}
 }
 
 func (h *handler) WithGroup(name string) slog.Handler {
 	h.mu.Lock()
 	cBuf := h.cBuf
 	id := h.logID
+	forceFlush := h.forceFlush
 	h.mu.Unlock()
-	return &handler{wrappedHandler: h.wrappedHandler.WithGroup(name), mu: h.mu, cBuf: cBuf, logID: id}
+	return &handler{wrappedHandler: h.wrappedHandler.WithGroup(name), mu: h.mu, cBuf: cBuf, logID: id, forceFlush: forceFlush}
 }
 
 func (h *handler) Handle(ctx context.Context, r slog.Record) error {
@@ -223,6 +239,10 @@ func (h *handler) Handle(ctx context.Context, r slog.Record) error {
 			return err
 		} else if r.Level == LevelImmediate {
 			return h.wrappedHandler.Handle(ctx, r)
+		} else if h.forceFlush {
+			// For `testing/slogtest`
+			// See: https://github.com/golang/go/issues/61706#issuecomment-1674413648
+			return h.wrappedHandler.Handle(ctx, r)
 		}
 	}
 

log/slog_test.go

@@ -5,12 +5,16 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"log/slog"
 	"strings"
 	"testing"
+	"testing/slogtest"
+
+	"go.akshayshah.org/attest"
 )
 
 // Check that our handler is conformant with log/slog expectations.
-// Taken from https://github.com/golang/go/blob/go1.21.0/src/log/slog/slogtest_test.go#L18-L26
+// Taken from https://github.com/golang/go/blob/go1.22.0/src/log/slog/slogtest_test.go#L18-L26
 //
 // We test this handler even though acording to Jonathan Amsterdam(jba):
 // `I think wrapping handlers like those that don't actually affect the format don't need testing/slogtest`
@@ -29,23 +33,6 @@ func TestSlogtest(t *testing.T) {
 		t.Log("hey::: ", buf.String())
 	}
 
-	parseLines := func(src []byte, parse func([]byte) (map[string]any, error)) ([]map[string]any, error) {
-		t.Helper()
-
-		var records []map[string]any
-		for _, line := range bytes.Split(src, []byte{'\n'}) {
-			if len(line) == 0 {
-				continue
-			}
-			m, err := parse(line)
-			if err != nil {
-				return nil, fmt.Errorf("%s: %w", string(line), err)
-			}
-			records = append(records, m)
-		}
-		return records, nil
-	}
-
 	parseJSON := func(bs []byte) (map[string]any, error) {
 		t.Helper()
 
@@ -85,21 +72,32 @@ func TestSlogtest(t *testing.T) {
 			t.Parallel()
 
 			var buf bytes.Buffer
-			l := New(context.Background(), &buf, tt.maxSize)
-			handler := l.Handler()
-
-			results := func() []map[string]any {
-				ms, err := parseLines(buf.Bytes(), tt.parseFunc)
-				if err != nil {
-					t.Fatal(err)
-				}
-				return ms
-			}
 
-			err := testHandler(handler, results)
-			if err != nil {
-				t.Fatal(err)
+			results := func(t *testing.T) map[string]any {
+				m := map[string]any{}
+				err := json.Unmarshal(buf.Bytes(), &m)
+				attest.Ok(t, err)
+
+				return m
 			}
+
+			slogtest.Run(
+				t,
+				func(*testing.T) slog.Handler {
+					buf.Reset()
+
+					l := New(context.Background(), &buf, tt.maxSize)
+					h := l.Handler()
+					{
+						underlyingHandler, ok := h.(*handler)
+						attest.Equal(t, ok, true)
+						underlyingHandler.forceFlush = true
+					}
+
+					return h
+				},
+				results,
+			)
 		})
 	}
 }