sneak
b7aee81fa5
All checks were successful
check / check (pull_request) Successful in 2m34s
Two defects in internal/log, fixed together because both live in the handler construction path. Logger on stdout (#82). Initialize built both handlers over os.Stdout. Every --json subcommand writes its document to that same stream, and WARN/ERROR are never suppressed by any flag, so a log record could land inside a JSON document and break the parse. This was not theoretical: a config file with group- or world-readable permissions triggers a WARN during startup, which was enough to make `snapshot list --json | jq` fail. Diagnostics now go to stderr, and the TTY/JSON format choice follows stderr's TTY-ness rather than stdout's -- testing the wrong stream would colorize records on a redirected stderr whenever stdout happened to be a terminal, and emit JSON to a terminal in the reverse case. This is user-visible: --verbose and --debug output moves to stderr as well, so `vaultik snapshot list -v > out.txt` no longer captures the diagnostics. README.md documents the split under a new "stdout and stderr" section. --quiet and --cron semantics are untouched: level selection is unchanged, and warnings and errors are still emitted in both modes. It also retires the local workaround in internal/vaultik/snapshot_list.go. warnWhileListing had been hand-rolling structured-log formatting to reach a writer that was not stdout, and the jsonOutput parameter threaded through the remote-listing helpers existed only to choose between the two writers; both are gone, and those warnings go through log.Warn in every mode. The collect-then-emit machinery around listingWarning stays, on its remaining merit rather than its original one: slog handlers are safe for concurrent use, but emitting from the manifest-fetch workers would order warnings by network timing, where collecting and emitting in key order after group.Wait makes two runs over the same damaged store produce the same diagnostics in the same order. TTYHandler dropped attributes (#97). WithAttrs and WithGroup discarded their arguments and returned the receiver, while their doc comments claimed the opposite. Because the handler is chosen by TTY-ness, this failed only on a terminal and worked correctly in CI -- so it broke exactly when someone was debugging interactively. Both now return a new handler: the receiver is never written to, since slog permits a handler to be shared and derived from concurrently, and the derived handler copies its slices rather than reslicing so two concurrent derivations cannot overwrite each other's attributes. The mutex became a pointer so handlers sharing a stream keep sharing one lock. Attributes persist across every subsequent record, and grouping is implemented as dotted key prefixes, which is the only honest rendering for a format with nowhere to nest. Tests: an attribute attached through the exported log.With reaches TTYHandler output; attributes persist across records; groups qualify keys; deriving does not leak between siblings or back to the parent; sixteen goroutines derive from and write through one handler under -race; and the TTY and JSON handlers are fed identical derivation chains and compared attribute set by attribute set, which is the test that would have caught the original defect and the one that stops the two paths drifting again. All of these fail against the unfixed handler. The existing snapshot-list tests that asserted these warnings on an injected writer now capture the process's real stderr, which is where they go; the assertions are otherwise unchanged.
65 lines
2.0 KiB
Go
65 lines
2.0 KiB
Go
//nolint:testpackage // needs the package logger; see TestWithAttributesReachTTYOutput
|
|
package log //nolint:revive,nolintlint // stdlib log unused here; see #76
|
|
|
|
import (
|
|
"bytes"
|
|
"log/slog"
|
|
"regexp"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// withTestANSIEscape matches the SGR sequences TTYHandler emits.
|
|
var withTestANSIEscape = regexp.MustCompile(`\x1b\[[0-9;]*m`)
|
|
|
|
// TestWithAttributesReachTTYOutput exercises the exported package-level
|
|
// With through a TTYHandler, which is the path the reported defect was
|
|
// on: the handler is selected by TTY-ness, so on a terminal With's
|
|
// attributes were silently dropped while the same code printed them
|
|
// correctly in CI.
|
|
//
|
|
// This is an in-package test so it can point the package logger at a
|
|
// buffer. Building an slog.Logger over a TTYHandler by hand would test
|
|
// slog, not this package's With, and there is no injectable sink to
|
|
// reach it from outside. The package logger is process-global, so this
|
|
// test must not run in parallel.
|
|
//
|
|
//nolint:paralleltest // replaces the process-global package logger
|
|
func TestWithAttributesReachTTYOutput(t *testing.T) {
|
|
var buf bytes.Buffer
|
|
|
|
previous := logger
|
|
|
|
t.Cleanup(func() { logger = previous })
|
|
|
|
logger = slog.New(NewTTYHandler(&buf, &slog.HandlerOptions{
|
|
Level: slog.LevelDebug,
|
|
}))
|
|
|
|
With("key", "value").Info("hello")
|
|
|
|
plain := withTestANSIEscape.ReplaceAllString(buf.String(), "")
|
|
|
|
require.NotEmpty(t, plain)
|
|
assert.Contains(t, plain, "hello")
|
|
assert.Contains(t, plain, "key=value",
|
|
"log.With attributes must reach TTYHandler output")
|
|
}
|
|
|
|
// TestWithoutInitializedLoggerFallsBack pins the documented behavior of
|
|
// With before Initialize has run: it hands back the slog default rather
|
|
// than a nil logger that would panic at the call site.
|
|
//
|
|
//nolint:paralleltest // replaces the process-global package logger
|
|
func TestWithoutInitializedLoggerFallsBack(t *testing.T) {
|
|
previous := logger
|
|
|
|
t.Cleanup(func() { logger = previous })
|
|
|
|
logger = nil
|
|
|
|
assert.NotNil(t, With("key", "value"))
|
|
}
|