All checks were successful
check / check (pull_request) Successful in 2m46s
Entry writes the banner to stdout before cobra parses anything, and the scan that decides whether to write it knew --quiet, -q and --cron but not --json. Every --json document therefore arrived behind two lines of prose and a blank line, and `vaultik snapshot list --json | jq` failed. Passing opts.JSON as extraQuiet could not help: that reaches UI.SetQuiet through an fx OnStart hook, long after the banner is already written. With the logger moved to stderr in #82, this was the last writer that could put something on stdout the caller did not ask for. The design question the issue raised is answered in favour of extending the raw-argv scan rather than moving the banner after parsing. The banner is printed first deliberately, so that it still appears when cobra rejects the arguments and on --help; after parsing there is no single place that covers those paths, so "after parsing" means either reimplementing the banner in several handlers or losing it exactly where a human most wants to know which build just ran. The objection to the scan is that --json is a subcommand flag matched anywhere in the vector, but --cron is already in the list and is also a subcommand flag: it exists only on `snapshot create`. So this adds another instance of an imprecision the code already accepts, not a new kind of one. The two error directions are not symmetric either — a false positive loses a decorative banner, a false negative corrupts a document — so the scan errs toward suppression, and --json=false suppresses it exactly as --quiet=false already does. Three tests at the CLI layer, where internal/vaultik's existing guard cannot reach. TestEntryJSONStdoutIsExactlyOneDocument runs Entry itself over the process's real stdout descriptor, through cobra and the fx graph to the document, and asserts the capture decodes as one JSON value with nothing after it; it is hermetic because file:// storage needs no credentials and `snapshot list` treats a destination store with no metadata/ as an empty list rather than a failure. A second covers the argument vectors of all five --json commands plus the pre-subcommand and --json=true forms. A third asserts the banner is still printed without a suppressing flag, so the first cannot be satisfied by deleting it. AGENTS.md policy 9 still keyed the structured-log format on stdout's TTY-ness after #82 moved that decision to stderr; it now names the log stream. A rules file that misdescribes the code misleads exactly the readers who trust it most. Two smaller findings from the same review. bytesAttrKey's human-readable byte formatting stopped applying under an open group, because the key reaching the comparison is group-qualified: "bytes" logged under a group arrives as "transfer.bytes" and fell back to a bare number. The match is now made on the final dot-separated segment, tested both grouped and ungrouped. And listEnv.stderr in snapshot_list_test.go, assigned but never read since those tests began capturing the process's stderr, is removed. Vaultik.Stderr is kept — nothing writes to it today, which its comment now says outright rather than leaving the next reader to hunt for a writer that does not exist. `prune --json` still does not survive jq, for an unrelated reason found while verifying this: pruneLocalSnapshots writes three lines of prose to stdout with no --json awareness, on main and after this change alike, and -q never suppressed them either. Filed as #108 rather than fixed here, being a different writer on a different code path.
423 lines
12 KiB
Go
423 lines
12 KiB
Go
package log_test
|
|
|
|
import (
|
|
"bytes"
|
|
"context"
|
|
"encoding/json"
|
|
"fmt"
|
|
"log/slog"
|
|
"math"
|
|
"regexp"
|
|
"sort"
|
|
"strconv"
|
|
"strings"
|
|
"sync"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
"sneak.berlin/go/vaultik/internal/log"
|
|
)
|
|
|
|
// ansiEscape matches the SGR sequences TTYHandler wraps every field in.
|
|
// Stripping them is what lets a test compare TTYHandler's rendering with
|
|
// slog.JSONHandler's.
|
|
var ansiEscape = regexp.MustCompile(`\x1b\[[0-9;]*m`)
|
|
|
|
// countKey is an attribute key reused across the comparison cases.
|
|
const countKey = "count"
|
|
|
|
// debugHandlerOptions enables every level, so a test never has to reason
|
|
// about the default level while reasoning about attributes.
|
|
func debugHandlerOptions() *slog.HandlerOptions {
|
|
return &slog.HandlerOptions{Level: slog.LevelDebug}
|
|
}
|
|
|
|
// ttyAttrs renders one record through a TTYHandler and returns its
|
|
// attributes as key -> value, with color stripped.
|
|
//
|
|
// TTYHandler emits " key=value" per attribute after the message, and the
|
|
// message itself is the last thing before the first attribute, so
|
|
// splitting on spaces and keeping the tokens containing "=" recovers the
|
|
// attribute set. Test values below therefore avoid spaces and "=".
|
|
func ttyAttrs(t *testing.T, derive func(*slog.Logger) *slog.Logger,
|
|
msg string, args ...any,
|
|
) map[string]string {
|
|
t.Helper()
|
|
|
|
var buf bytes.Buffer
|
|
|
|
logger := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions()))
|
|
derive(logger).Info(msg, args...)
|
|
|
|
line := ansiEscape.ReplaceAllString(buf.String(), "")
|
|
attrs := make(map[string]string)
|
|
|
|
for token := range strings.FieldsSeq(line) {
|
|
key, value, found := strings.Cut(token, "=")
|
|
if !found {
|
|
continue
|
|
}
|
|
|
|
attrs[key] = value
|
|
}
|
|
|
|
return attrs
|
|
}
|
|
|
|
// jsonAttrs renders one record through slog.JSONHandler and returns its
|
|
// attributes flattened to the same dotted-key form TTYHandler uses, so
|
|
// the two are directly comparable. The built-in time/level/msg fields
|
|
// are dropped: they are the record, not its attributes.
|
|
func jsonAttrs(t *testing.T, derive func(*slog.Logger) *slog.Logger,
|
|
msg string, args ...any,
|
|
) map[string]string {
|
|
t.Helper()
|
|
|
|
var buf bytes.Buffer
|
|
|
|
logger := slog.New(slog.NewJSONHandler(&buf, debugHandlerOptions()))
|
|
derive(logger).Info(msg, args...)
|
|
|
|
var decoded map[string]any
|
|
|
|
require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
|
|
|
|
delete(decoded, slog.TimeKey)
|
|
delete(decoded, slog.LevelKey)
|
|
delete(decoded, slog.MessageKey)
|
|
|
|
attrs := make(map[string]string)
|
|
flattenJSON(attrs, "", decoded)
|
|
|
|
return attrs
|
|
}
|
|
|
|
// flattenJSON turns JSONHandler's nested group objects into the dotted
|
|
// keys TTYHandler writes.
|
|
func flattenJSON(dst map[string]string, prefix string, src map[string]any) {
|
|
for key, value := range src {
|
|
full := key
|
|
if prefix != "" {
|
|
full = prefix + "." + key
|
|
}
|
|
|
|
nested, ok := value.(map[string]any)
|
|
if ok {
|
|
flattenJSON(dst, full, nested)
|
|
|
|
continue
|
|
}
|
|
|
|
dst[full] = valueString(value)
|
|
}
|
|
}
|
|
|
|
// valueString renders a decoded JSON scalar the way slog.Value.String
|
|
// renders the corresponding Go value, so the two handlers' outputs can
|
|
// be compared as strings. encoding/json decodes every number as
|
|
// float64, so an integral one is rendered back as an integer — which is
|
|
// what the Go value that produced it was.
|
|
func valueString(v any) string {
|
|
switch typed := v.(type) {
|
|
case string:
|
|
return typed
|
|
case bool:
|
|
return strconv.FormatBool(typed)
|
|
case float64:
|
|
if typed == math.Trunc(typed) {
|
|
return strconv.FormatInt(int64(typed), 10)
|
|
}
|
|
|
|
return strconv.FormatFloat(typed, 'g', -1, 64)
|
|
default:
|
|
return fmt.Sprint(v)
|
|
}
|
|
}
|
|
|
|
// TestTTYHandlerWithAttrsEmitsAttributes is the direct regression test
|
|
// for the reported defect: WithAttrs discarded its argument, so an
|
|
// attribute attached to a logger never reached the output.
|
|
func TestTTYHandlerWithAttrsEmitsAttributes(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
attrs := ttyAttrs(t, func(l *slog.Logger) *slog.Logger {
|
|
return l.With("key", "value")
|
|
}, "hello")
|
|
|
|
assert.Equal(t, "value", attrs["key"],
|
|
"an attribute attached with With must appear on every record")
|
|
}
|
|
|
|
// TestTTYHandlerWithAttrsPersistsAcrossRecords checks that the
|
|
// attributes are retained rather than emitted once. A handler that
|
|
// stored them but consumed them would pass the test above.
|
|
func TestTTYHandlerWithAttrsPersistsAcrossRecords(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var buf bytes.Buffer
|
|
|
|
logger := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions())).
|
|
With("request", "abc123")
|
|
|
|
logger.Info("first")
|
|
logger.Info("second")
|
|
|
|
plain := ansiEscape.ReplaceAllString(buf.String(), "")
|
|
lines := strings.Split(strings.TrimSuffix(plain, "\n"), "\n")
|
|
|
|
require.Len(t, lines, 2)
|
|
|
|
for _, line := range lines {
|
|
assert.Contains(t, line, "request=abc123")
|
|
}
|
|
}
|
|
|
|
// TestTTYHandlerWithGroupQualifiesKeys checks that WithGroup does
|
|
// something real rather than being discarded. This format has no
|
|
// nesting, so grouping shows up as a dotted key prefix.
|
|
func TestTTYHandlerWithGroupQualifiesKeys(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
attrs := ttyAttrs(t, func(l *slog.Logger) *slog.Logger {
|
|
return l.WithGroup("db").With("rows", 3)
|
|
}, "queried", "table", "chunks")
|
|
|
|
assert.Equal(t, "3", attrs["db.rows"],
|
|
"an attribute added under a group must be qualified by it")
|
|
assert.Equal(t, "chunks", attrs["db.table"],
|
|
"a record attribute must also be qualified by the open group")
|
|
assert.NotContains(t, attrs, "rows")
|
|
}
|
|
|
|
// TestTTYHandlerByteFormattingSurvivesGrouping guards the interaction
|
|
// between the two features. The human-readable rendering of a "bytes"
|
|
// attribute is selected by comparing the key, and keys reaching that
|
|
// comparison are group-qualified, so a "bytes" attribute logged under an
|
|
// open group arrived as "transfer.bytes" and fell back to a bare number.
|
|
// No caller groups a byte count today, which is exactly why this needs a
|
|
// test rather than a bug report.
|
|
func TestTTYHandlerByteFormattingSurvivesGrouping(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
const oneAndAHalfKiB = 1536
|
|
|
|
for name, testCase := range map[string]struct {
|
|
derive func(*slog.Logger) *slog.Logger
|
|
key string
|
|
}{
|
|
"ungrouped": {
|
|
derive: func(l *slog.Logger) *slog.Logger { return l },
|
|
key: "bytes",
|
|
},
|
|
"grouped": {
|
|
derive: func(l *slog.Logger) *slog.Logger {
|
|
return l.WithGroup("transfer")
|
|
},
|
|
key: "transfer.bytes",
|
|
},
|
|
} {
|
|
t.Run(name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var buf bytes.Buffer
|
|
|
|
logger := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions()))
|
|
testCase.derive(logger).Info("uploaded", "bytes", oneAndAHalfKiB)
|
|
|
|
line := ansiEscape.ReplaceAllString(buf.String(), "")
|
|
|
|
assert.Contains(t, line, testCase.key+"=1.5 KB",
|
|
"a byte count must be human-readable however it is qualified")
|
|
assert.NotContains(t, line, strconv.Itoa(oneAndAHalfKiB),
|
|
"the raw number must not survive the formatting")
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestTTYHandlerMatchesJSONHandlerAttributes is the drift guard. The
|
|
// handler is chosen by TTY-ness, so a difference between these two is
|
|
// invisible in whichever environment the developer is not in — which is
|
|
// how the original defect survived: attributes vanished on a terminal
|
|
// and were correct in CI.
|
|
func TestTTYHandlerMatchesJSONHandlerAttributes(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
cases := []struct {
|
|
name string
|
|
derive func(*slog.Logger) *slog.Logger
|
|
args []any
|
|
}{
|
|
{
|
|
name: "record attributes only",
|
|
derive: func(l *slog.Logger) *slog.Logger { return l },
|
|
args: []any{"path", "/etc/vaultik", countKey, 7},
|
|
},
|
|
{
|
|
name: "handler attributes",
|
|
derive: func(l *slog.Logger) *slog.Logger {
|
|
return l.With("host", "alpha")
|
|
},
|
|
args: []any{countKey, 7},
|
|
},
|
|
{
|
|
name: "handler attributes accumulate",
|
|
derive: func(l *slog.Logger) *slog.Logger {
|
|
return l.With("host", "alpha").With("snapshot", "s1")
|
|
},
|
|
args: []any{countKey, 7},
|
|
},
|
|
{
|
|
name: "group qualifies later attributes",
|
|
derive: func(l *slog.Logger) *slog.Logger {
|
|
return l.WithGroup("db").With("rows", 3)
|
|
},
|
|
args: []any{"table", "chunks"},
|
|
},
|
|
{
|
|
name: "nested groups",
|
|
derive: func(l *slog.Logger) *slog.Logger {
|
|
return l.WithGroup("outer").WithGroup("inner").
|
|
With("leaf", "v")
|
|
},
|
|
args: []any{"other", "w"},
|
|
},
|
|
{
|
|
name: "attributes before and after a group",
|
|
derive: func(l *slog.Logger) *slog.Logger {
|
|
return l.With("top", "t").WithGroup("g").With("in", "i")
|
|
},
|
|
args: []any{"rec", "r"},
|
|
},
|
|
{
|
|
name: "inline group value on the record",
|
|
derive: func(l *slog.Logger) *slog.Logger { return l },
|
|
args: []any{slog.Group("net",
|
|
slog.String("proto", "s3"), slog.Int("retries", 2))},
|
|
},
|
|
}
|
|
|
|
for _, testCase := range cases {
|
|
t.Run(testCase.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
tty := ttyAttrs(t, testCase.derive, "message", testCase.args...)
|
|
js := jsonAttrs(t, testCase.derive, "message", testCase.args...)
|
|
|
|
assert.Equal(t, sortedKeys(js), sortedKeys(tty),
|
|
"TTY and JSON handlers must emit the same attribute keys")
|
|
assert.Equal(t, js, tty,
|
|
"TTY and JSON handlers must emit the same attribute values")
|
|
})
|
|
}
|
|
}
|
|
|
|
// sortedKeys returns m's keys in order, for a stable comparison message.
|
|
func sortedKeys(m map[string]string) []string {
|
|
keys := make([]string, 0, len(m))
|
|
for key := range m {
|
|
keys = append(keys, key)
|
|
}
|
|
|
|
sort.Strings(keys)
|
|
|
|
return keys
|
|
}
|
|
|
|
// TestTTYHandlerWithAttrsDoesNotMutateReceiver checks that deriving does
|
|
// not write through to the parent or to a sibling. slog permits a
|
|
// handler to be shared, so a WithAttrs that appended into the receiver's
|
|
// state would leak attributes between unrelated loggers.
|
|
func TestTTYHandlerWithAttrsDoesNotMutateReceiver(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var buf bytes.Buffer
|
|
|
|
base := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions()))
|
|
first := base.With("branch", "one")
|
|
second := base.With("branch", "two")
|
|
|
|
base.Info("base")
|
|
first.Info("first")
|
|
second.Info("second")
|
|
|
|
plain := ansiEscape.ReplaceAllString(buf.String(), "")
|
|
lines := strings.Split(strings.TrimSuffix(plain, "\n"), "\n")
|
|
|
|
require.Len(t, lines, 3)
|
|
|
|
assert.NotContains(t, lines[0], "branch=",
|
|
"deriving must not add attributes to the handler derived from")
|
|
assert.Contains(t, lines[1], "branch=one")
|
|
assert.NotContains(t, lines[1], "branch=two")
|
|
assert.Contains(t, lines[2], "branch=two")
|
|
assert.NotContains(t, lines[2], "branch=one")
|
|
}
|
|
|
|
// TestTTYHandlerConcurrentDerivation exercises the same handler being
|
|
// derived from and written through by several goroutines at once, which
|
|
// is what slog permits and what a mutating WithAttrs would make a data
|
|
// race. Run under -race by script/test.
|
|
func TestTTYHandlerConcurrentDerivation(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
const workers = 16
|
|
|
|
var buf bytes.Buffer
|
|
|
|
base := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions())).
|
|
With("shared", "yes")
|
|
|
|
var group sync.WaitGroup
|
|
|
|
group.Add(workers)
|
|
|
|
for worker := range workers {
|
|
go func() {
|
|
defer group.Done()
|
|
|
|
base.With("worker", worker).
|
|
WithGroup("g").
|
|
With("nested", worker).
|
|
Info("concurrent")
|
|
}()
|
|
}
|
|
|
|
group.Wait()
|
|
|
|
plain := ansiEscape.ReplaceAllString(buf.String(), "")
|
|
lines := strings.Split(strings.TrimSuffix(plain, "\n"), "\n")
|
|
|
|
require.Len(t, lines, workers)
|
|
|
|
for _, line := range lines {
|
|
assert.Contains(t, line, "shared=yes")
|
|
assert.Contains(t, line, "worker=")
|
|
assert.Contains(t, line, "g.nested=")
|
|
}
|
|
}
|
|
|
|
// TestTTYHandlerEmptyGroupAndAttrsAreNoOps covers the slog.Handler
|
|
// contract corners: WithGroup("") and WithAttrs(nil) change nothing, and
|
|
// an empty Attr is dropped rather than rendered as "=".
|
|
func TestTTYHandlerEmptyGroupAndAttrsAreNoOps(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var buf bytes.Buffer
|
|
|
|
handler := log.NewTTYHandler(&buf, debugHandlerOptions())
|
|
|
|
assert.Same(t, handler, handler.WithGroup(""),
|
|
"an empty group name must not open a group")
|
|
assert.Same(t, handler, handler.WithAttrs(nil),
|
|
"deriving with no attributes must not allocate a handler")
|
|
|
|
slog.New(handler).LogAttrs(context.Background(), slog.LevelInfo, "msg",
|
|
slog.Attr{}, slog.String("kept", "yes"))
|
|
|
|
plain := ansiEscape.ReplaceAllString(buf.String(), "")
|
|
|
|
assert.Contains(t, plain, "kept=yes")
|
|
assert.NotContains(t, plain, " =")
|
|
}
|