diff --git a/.golangci.yml b/.golangci.yml index 1b73eb9..80801b5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -61,6 +61,12 @@ linters: desc: >- Test-support code belongs in test files and in packages whose directory name ends in test, not in the shipped binary. + - pkg: sneak.berlin/go/webhooker/internal/config/configtest + desc: test support; a file that is not a test must not import it + - pkg: sneak.berlin/go/webhooker/internal/database/databasetest + desc: test support; a file that is not a test must not import it + - pkg: sneak.berlin/go/webhooker/internal/middleware/middlewaretest + desc: test support; a file that is not a test must not import it # Only decisions already recorded in the Go package defaults are # listed here. Every entry matches the module path exactly. gomodguard_v2: diff --git a/README.md b/README.md index 2f9bd66..0e53c78 100644 --- a/README.md +++ b/README.md @@ -2984,7 +2984,7 @@ webhooker/ │ │ └── resetpw.go # `webhooker resetpw`: set an account's password, stopped deployments only │ ├── config/ │ │ ├── config.go # Configuration loading from environment variables -│ │ └── testing.go # ClearEnvForTest: an empty environment for one test +│ │ └── configtest/ # Test support: ClearEnv, an empty environment for one test │ ├── database/ │ │ ├── base_model.go # BaseModel with UUID primary keys │ │ ├── database.go # GORM connection, migrations, admin seed @@ -3001,8 +3001,8 @@ webhooker/ │ │ ├── model_apikey.go # APIKey entity │ │ ├── password.go # Argon2id hashing and verification │ │ ├── retention.go # Retention reaper (per-webhook event expiry) -│ │ ├── testing.go # NewTestDatabase: wrapper for tests, no fx lifecycle -│ │ └── webhook_db_manager.go # Per-webhook DB lifecycle manager +│ │ ├── webhook_db_manager.go # Per-webhook DB lifecycle manager +│ │ └── databasetest/ # Test support: a WebhookDBManager for tests in other packages │ ├── datadir/ │ │ └── lock.go # Exclusive advisory lock on DATA_DIR (one instance) │ ├── globals/ @@ -3063,7 +3063,7 @@ webhooker/ │ │ ├── csrf.go # CSRF protection middleware (gorilla/csrf) │ │ ├── ratelimit.go # Per-IP rate limiting middleware (go-chi/httprate) │ │ ├── loginguard.go # Login failure counters and the Argon2id verification semaphore -│ │ └── testing.go # NewForTest: Middleware without the fx lifecycle +│ │ └── middlewaretest/ # Test support: a Middleware for tests in other packages │ ├── reqtls/ │ │ └── reqtls.go # IsTLS: the one TLS predicate, r.TLS or X-Forwarded-Proto │ ├── server/ @@ -3071,8 +3071,7 @@ webhooker/ │ │ ├── http.go # HTTP server setup with timeouts │ │ └── routes.go # All route definitions │ ├── session/ -│ │ ├── session.go # Cookie-based session management -│ │ └── testing.go # NewForTest: Session without the fx lifecycle +│ │ └── session.go # Cookie-based session management │ └── versionscript/ │ └── doc.go # Tests for script/version and the build files that use it ├── static/ diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 9269e99..5990801 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -4,6 +4,7 @@ package main import ( "fmt" "io" + "log/slog" "os" "time" @@ -187,6 +188,9 @@ func newApp() *fx.App { fx.Provide( globals.New, logger.New, + // The plain logger the session, the middleware and the + // webhook database manager take. + func(l *logger.Logger) *slog.Logger { return l.Get() }, config.New, database.New, database.NewWebhookDBManager, diff --git a/cmd/webhooker/main_test.go b/cmd/webhooker/main_test.go index b225db6..160cb1d 100644 --- a/cmd/webhooker/main_test.go +++ b/cmd/webhooker/main_test.go @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/resetpw" "sneak.berlin/go/webhooker/internal/server" @@ -37,7 +37,7 @@ const dockerStopGrace = 10 * time.Second // fx.New applies options before it executes invokes, so the timeout // is set whether or not the graph itself can be constructed here. func TestNewApp_StopTimeout(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("DATA_DIR", t.TempDir()) got := newApp().StopTimeout() @@ -75,7 +75,7 @@ func freePort(t *testing.T) int { // anything is built, and the run of logger.New, which happens before // the configuration sets the level. func TestNewApp_SendsFxEventsToTheLogger(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("DATA_DIR", t.TempDir()) t.Setenv("PORT", strconv.Itoa(freePort(t))) t.Setenv("DEBUG", "true") diff --git a/internal/config/testing_test.go b/internal/config/clearenv_test.go similarity index 75% rename from internal/config/testing_test.go rename to internal/config/clearenv_test.go index 4c2e315..17f8757 100644 --- a/internal/config/testing_test.go +++ b/internal/config/clearenv_test.go @@ -7,21 +7,22 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" ) -// TestClearEnvForTest_RemovesAddedVariables pins that a variable set +// TestClearEnv_RemovesAddedVariables pins that a variable set // after the clear other than through t.Setenv, as a test's .env file // sets one, is gone once the test ends, so it cannot reach the tests // that run after it. // -//nolint:paralleltest // ClearEnvForTest uses t.Setenv. -func TestClearEnvForTest_RemovesAddedVariables(t *testing.T) { +//nolint:paralleltest // ClearEnv uses t.Setenv. +func TestClearEnv_RemovesAddedVariables(t *testing.T) { // The outer clear keeps a value of the key exported in the shell // from making it a variable the inner clear has to put back. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Run("loads a .env file after the clear", func(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) path := writeDotEnv(t, dotEnvKey+"=from-dot-env\n") require.NoError(t, config.LoadDotEnvFileForTest(path)) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 8b11f5f..717377b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -11,6 +11,7 @@ import ( "go.uber.org/fx" "go.uber.org/fx/fxtest" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/logger" ) @@ -70,7 +71,7 @@ func TestEnvironmentConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if tt.envValue != "" { t.Setenv( @@ -196,7 +197,7 @@ func TestRetentionSweepInterval(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { @@ -335,7 +336,7 @@ func TestSessionIdleTimeout(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { @@ -388,7 +389,7 @@ func TestDefaultDataDir(t *testing.T) { t.Run("env="+name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if env != "" { t.Setenv("WEBHOOKER_ENVIRONMENT", env) @@ -433,7 +434,7 @@ func TestDataDirHelper(t *testing.T) { t.Run(name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if set != "" { t.Setenv("DATA_DIR", set) @@ -498,7 +499,7 @@ func TestReceiverRateLimit(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { @@ -614,7 +615,7 @@ func TestTrustedProxies(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { @@ -725,7 +726,7 @@ func TestAllowedEgressCIDRs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") if tt.set { @@ -797,7 +798,7 @@ func TestEgressAllowlistWarning(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev) if tt.allowed != "" { @@ -933,7 +934,7 @@ func TestMetricsAuthConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if tt.username.set { t.Setenv("METRICS_USERNAME", tt.username.value) diff --git a/internal/config/testing.go b/internal/config/configtest/configtest.go similarity index 85% rename from internal/config/testing.go rename to internal/config/configtest/configtest.go index 3f0b2f0..abaefac 100644 --- a/internal/config/testing.go +++ b/internal/config/configtest/configtest.go @@ -1,4 +1,6 @@ -package config +// Package configtest holds test support for code that reads the +// process environment. +package configtest import ( "os" @@ -6,12 +8,12 @@ import ( "testing" ) -// ClearEnvForTest unsets every variable in the process environment +// ClearEnv unsets every variable in the process environment // for the rest of the test, so a test sees only the variables it sets // itself, not whatever the developer's shell exports. When the test // ends it leaves the environment exactly as it found it: each variable // it unset is put back, and any variable added since is removed. -func ClearEnvForTest(t *testing.T) { +func ClearEnv(t *testing.T) { t.Helper() present := make(map[string]bool) diff --git a/internal/config/dotenv_test.go b/internal/config/dotenv_test.go index b2ccbfa..9a9f073 100644 --- a/internal/config/dotenv_test.go +++ b/internal/config/dotenv_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" ) // dotEnvKey is a throwaway variable name the .env tests write and @@ -39,9 +40,9 @@ func writeDotEnv(t *testing.T, contents string) string { // normally rather than be refused for a file it was never meant to // have. // -//nolint:paralleltest // ClearEnvForTest uses t.Setenv. +//nolint:paralleltest // ClearEnv uses t.Setenv. func TestLoadDotEnv_MissingFileIsFine(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) absent := filepath.Join(t.TempDir(), config.DotEnvPath) require.NoError(t, config.LoadDotEnvFileForTest(absent)) @@ -54,9 +55,9 @@ func TestLoadDotEnv_MissingFileIsFine(t *testing.T) { // reaches the environment, which is the whole reason the file is read // at all. // -//nolint:paralleltest // ClearEnvForTest uses t.Setenv. +//nolint:paralleltest // ClearEnv uses t.Setenv. func TestLoadDotEnv_AppliesValues(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n") @@ -82,9 +83,9 @@ func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) { // reverts to its default; the process used to start that way with no // log line naming the file at all. // -//nolint:paralleltest // ClearEnvForTest uses t.Setenv. +//nolint:paralleltest // ClearEnv uses t.Setenv. func TestLoadDotEnv_MalformedFileAborts(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) path := writeDotEnv( t, malformedDotEnv+dotEnvKey+"=from-dot-env\n", @@ -132,7 +133,7 @@ func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) { // //nolint:paralleltest // t.Chdir moves the whole process. func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) dir := t.TempDir() require.NoError(t, os.WriteFile( diff --git a/internal/config/env_test.go b/internal/config/env_test.go index 81448c3..067d677 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/fx" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/logger" ) @@ -120,7 +121,7 @@ func TestEnvBool(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if tt.set { t.Setenv(testEnvKey, tt.value) @@ -169,7 +170,7 @@ func runEnvIntCases( t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if tt.set { t.Setenv(testEnvKey, tt.value) @@ -310,7 +311,7 @@ func TestEnvBindAddress(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if tt.set { t.Setenv(testEnvKey, tt.value) @@ -476,7 +477,7 @@ func TestNewRejectsBadEnvValues(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") t.Setenv(tt.key, tt.value) @@ -638,7 +639,7 @@ func sentryEnvValueCases() []badEnvValueCase { // break the legitimate unset case: absent variables still get their // documented defaults. func TestNewUsesDefaultsWhenUnset(t *testing.T) { - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") cfg, err := buildConfig(t) diff --git a/internal/config/sentry_test.go b/internal/config/sentry_test.go index 871ecb6..d713f62 100644 --- a/internal/config/sentry_test.go +++ b/internal/config/sentry_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" ) // envKeySentryDSN is the variable envSentryDSN reads in production. @@ -100,7 +101,7 @@ func TestEnvSentryDSN(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Cannot use t.Parallel() here because t.Setenv // is incompatible with parallel subtests. - config.ClearEnvForTest(t) + configtest.ClearEnv(t) if tt.set { t.Setenv(envKeySentryDSN, tt.value) diff --git a/internal/database/databasetest/databasetest.go b/internal/database/databasetest/databasetest.go new file mode 100644 index 0000000..f999355 --- /dev/null +++ b/internal/database/databasetest/databasetest.go @@ -0,0 +1,55 @@ +// Package databasetest builds a WebhookDBManager for tests in other +// packages. +package databasetest + +import ( + "log/slog" + "os" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/fx/fxtest" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/database" +) + +// NewWebhookDBManager creates a WebhookDBManager backed by the given +// data directory, logging at DEBUG to standard error. +func NewWebhookDBManager( + t *testing.T, dataDir string, +) *database.WebhookDBManager { + t.Helper() + + return NewWebhookDBManagerWithLogger( + t, + dataDir, + slog.New(slog.NewTextHandler( + os.Stderr, + &slog.HandlerOptions{Level: slog.LevelDebug}, + )), + ) +} + +// NewWebhookDBManagerWithLogger is NewWebhookDBManager with the +// logger supplied by the caller. The per-webhook databases this manager +// opens hand that logger to gormlog, so a test that needs to see the SQL +// the service emits can capture it. +// +// It is built through database.NewWebhookDBManager on a lifecycle that +// is never started, so nothing closes its databases but the caller. +func NewWebhookDBManagerWithLogger( + t *testing.T, dataDir string, log *slog.Logger, +) *database.WebhookDBManager { + t.Helper() + + mgr, err := database.NewWebhookDBManager( + fxtest.NewLifecycle(t), + database.WebhookDBManagerParams{ + Config: &config.Config{DataDir: dataDir}, + Logger: log, + }, + ) + require.NoError(t, err) + + return mgr +} diff --git a/internal/database/event_db_isolation_test.go b/internal/database/event_db_isolation_test.go index 66c28e0..04d6fb4 100644 --- a/internal/database/event_db_isolation_test.go +++ b/internal/database/event_db_isolation_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" _ "modernc.org/sqlite" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" ) // testDataDirPerm is the mode the test data directory is created @@ -133,7 +134,7 @@ func TestOpenPurgesLeakedTargetRows(t *testing.T) { // Create the file the way the application does, so the targets // table has exactly the shape AutoMigrate gives it, then write // a leaked row into it the way the association upsert did. - initial := database.NewTestWebhookDBManager(dataDir) + initial := databasetest.NewWebhookDBManager(t, dataDir) _, err := initial.GetDB(webhookID) require.NoError(t, err) @@ -156,7 +157,7 @@ func TestOpenPurgesLeakedTargetRows(t *testing.T) { clearEventDBSweptMarker(t, seed) require.NoError(t, seed.Close()) - mgr := database.NewTestWebhookDBManager(dataDir) + mgr := databasetest.NewWebhookDBManager(t, dataDir) _, err = mgr.GetDB(webhookID) require.NoError(t, err) @@ -172,7 +173,7 @@ func TestOpenPurgesLeakedTargetRows(t *testing.T) { // Idempotent: a second open leaves it at zero and does not // error. - again := database.NewTestWebhookDBManager(dataDir) + again := databasetest.NewWebhookDBManager(t, dataDir) _, err = again.GetDB(webhookID) require.NoError(t, err) @@ -195,7 +196,7 @@ func TestOpenPurgeRemovesCredentialBytes(t *testing.T) { webhookID := uuid.New().String() credential := "T00000000/B00000000/" + uuid.New().String() - initial := database.NewTestWebhookDBManager(dataDir) + initial := databasetest.NewWebhookDBManager(t, dataDir) _, err := initial.GetDB(webhookID) require.NoError(t, err) @@ -230,7 +231,7 @@ func TestOpenPurgeRemovesCredentialBytes(t *testing.T) { "seeded credential is not in the file, so this test proves nothing", ) - mgr := database.NewTestWebhookDBManager(dataDir) + mgr := databasetest.NewWebhookDBManager(t, dataDir) _, err = mgr.GetDB(webhookID) require.NoError(t, err) @@ -258,7 +259,7 @@ func TestOpenRevacuumsAfterIncompleteSweep(t *testing.T) { webhookID := uuid.New().String() credential := "T00000000/B00000000/" + uuid.New().String() - initial := database.NewTestWebhookDBManager(dataDir) + initial := databasetest.NewWebhookDBManager(t, dataDir) _, err := initial.GetDB(webhookID) require.NoError(t, err) @@ -298,7 +299,7 @@ func TestOpenRevacuumsAfterIncompleteSweep(t *testing.T) { "test proves nothing", ) - mgr := database.NewTestWebhookDBManager(dataDir) + mgr := databasetest.NewWebhookDBManager(t, dataDir) _, err = mgr.GetDB(webhookID) require.NoError(t, err) @@ -325,7 +326,7 @@ func TestOpenSkipsSweptDatabase(t *testing.T) { dataDir := eventDBDataDir(t) webhookID := uuid.New().String() - mgr := database.NewTestWebhookDBManager(dataDir) + mgr := databasetest.NewWebhookDBManager(t, dataDir) _, err := mgr.GetDB(webhookID) require.NoError(t, err) @@ -347,7 +348,7 @@ func TestOpenSkipsSweptDatabase(t *testing.T) { require.NoError(t, err) require.NoError(t, marked.Close()) - again := database.NewTestWebhookDBManager(dataDir) + again := databasetest.NewWebhookDBManager(t, dataDir) _, err = again.GetDB(webhookID) require.NoError(t, err) @@ -378,7 +379,7 @@ func TestOpenSucceedsWithoutTargetsTable(t *testing.T) { require.NoError(t, err) require.NoError(t, seed.Close()) - mgr := database.NewTestWebhookDBManager(dataDir) + mgr := databasetest.NewWebhookDBManager(t, dataDir) db, err := mgr.GetDB(webhookID) require.NoError(t, err) @@ -396,7 +397,7 @@ func TestEventDBCreateOmitsAssociations(t *testing.T) { dataDir := eventDBDataDir(t) webhookID := uuid.New().String() - mgr := database.NewTestWebhookDBManager(dataDir) + mgr := databasetest.NewWebhookDBManager(t, dataDir) db, err := mgr.GetDB(webhookID) require.NoError(t, err) diff --git a/internal/database/retention_test.go b/internal/database/retention_test.go index 7097677..710ffc2 100644 --- a/internal/database/retention_test.go +++ b/internal/database/retention_test.go @@ -51,7 +51,7 @@ func setupRetentionTest(t *testing.T) *retentionTestEnv { mgr, err := database.NewWebhookDBManager( lc, - database.WebhookDBManagerParams{Config: cfg, Logger: l}, + database.WebhookDBManagerParams{Config: cfg, Logger: l.Get()}, ) require.NoError(t, err) diff --git a/internal/database/testing.go b/internal/database/testing.go deleted file mode 100644 index 72c8816..0000000 --- a/internal/database/testing.go +++ /dev/null @@ -1,47 +0,0 @@ -package database - -import ( - "log/slog" - "os" - - "gorm.io/gorm" -) - -// NewTestDatabase creates a Database wrapper around a pre-opened *gorm.DB. -// Intended for use in tests that need a *database.Database without the -// full fx lifecycle. The caller is responsible for closing the underlying -// sql.DB connection. -func NewTestDatabase(db *gorm.DB) *Database { - return &Database{ - db: db, - log: slog.New(slog.NewTextHandler( - os.Stderr, - &slog.HandlerOptions{Level: slog.LevelDebug}, - )), - } -} - -// NewTestWebhookDBManager creates a WebhookDBManager backed by the given -// data directory. Intended for use in tests without the fx lifecycle. -func NewTestWebhookDBManager(dataDir string) *WebhookDBManager { - return NewTestWebhookDBManagerWithLogger( - dataDir, - slog.New(slog.NewTextHandler( - os.Stderr, - &slog.HandlerOptions{Level: slog.LevelDebug}, - )), - ) -} - -// NewTestWebhookDBManagerWithLogger is NewTestWebhookDBManager with the -// logger supplied by the caller. The per-webhook databases this manager -// opens hand that logger to gormlog, so a test that needs to see the SQL -// the service emits can capture it. -func NewTestWebhookDBManagerWithLogger( - dataDir string, log *slog.Logger, -) *WebhookDBManager { - return &WebhookDBManager{ - dataDir: dataDir, - log: log, - } -} diff --git a/internal/database/webhook_db_manager.go b/internal/database/webhook_db_manager.go index 023e997..6dc17a9 100644 --- a/internal/database/webhook_db_manager.go +++ b/internal/database/webhook_db_manager.go @@ -15,7 +15,6 @@ import ( "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/gormlog" - "sneak.berlin/go/webhooker/internal/logger" ) // WebhookDBManagerParams holds the fx dependencies for @@ -24,7 +23,7 @@ type WebhookDBManagerParams struct { fx.In Config *config.Config - Logger *logger.Logger + Logger *slog.Logger } // errInvalidCachedDBType indicates a type assertion failure @@ -70,7 +69,7 @@ func NewWebhookDBManager( ) (*WebhookDBManager, error) { m := &WebhookDBManager{ dataDir: params.Config.DataDir, - log: params.Logger.Get(), + log: params.Logger, } // Create data directory if it doesn't exist. datadir.DirPerm is the diff --git a/internal/database/webhook_db_manager_test.go b/internal/database/webhook_db_manager_test.go index b5954be..3c914d1 100644 --- a/internal/database/webhook_db_manager_test.go +++ b/internal/database/webhook_db_manager_test.go @@ -18,6 +18,7 @@ import ( "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/logger" ) @@ -50,7 +51,7 @@ func setupTestWebhookDBManager( lc, database.WebhookDBManagerParams{ Config: cfg, - Logger: l, + Logger: l.Get(), }, ) require.NoError(t, err) @@ -117,7 +118,8 @@ func TestWebhookDBManager_ConcurrentFirstTouchOpensOnce(t *testing.T) { var logs bytes.Buffer - mgr := database.NewTestWebhookDBManagerWithLogger( + mgr := databasetest.NewWebhookDBManagerWithLogger( + t, t.TempDir(), slog.New(slog.NewTextHandler(&logs, nil)), ) @@ -307,7 +309,8 @@ func TestWebhookDBManager_LostDatabaseIsLogged(t *testing.T) { var logs bytes.Buffer - mgr := database.NewTestWebhookDBManagerWithLogger( + mgr := databasetest.NewWebhookDBManagerWithLogger( + t, t.TempDir(), slog.New(slog.NewTextHandler(&logs, nil)), ) diff --git a/internal/delivery/archive_sweeper_test.go b/internal/delivery/archive_sweeper_test.go index 5563349..1630ae5 100644 --- a/internal/delivery/archive_sweeper_test.go +++ b/internal/delivery/archive_sweeper_test.go @@ -21,6 +21,7 @@ import ( "gorm.io/gorm/clause" _ "modernc.org/sqlite" // Pure Go SQLite driver. "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/gormlog" ) @@ -59,29 +60,14 @@ func setupArchiveTest(t *testing.T) *archiveEnv { dataDir := t.TempDir() log := archiveTestLogger() - sqlDB, err := sql.Open( - "sqlite", - fmt.Sprintf( - "file:%s?mode=rwc", - filepath.Join(dataDir, "main.db"), - ), - ) + mainDB, err := database.Open(dataDir, slog.New(slog.DiscardHandler)) require.NoError(t, err) - t.Cleanup(func() { _ = sqlDB.Close() }) - - gdb, err := gorm.Open( - sqlite.Dialector{Conn: sqlDB}, - &gorm.Config{Logger: gormlog.New(slog.New(slog.DiscardHandler))}, - ) - require.NoError(t, err) - - mainDB := database.NewTestDatabase(gdb) - require.NoError(t, mainDB.Migrate()) + t.Cleanup(func() { _ = mainDB.Close() }) eng := delivery.NewTestEngineWithDB( mainDB, - database.NewTestWebhookDBManager(dataDir), + databasetest.NewWebhookDBManager(t, dataDir), log, &http.Client{Timeout: 5 * time.Second}, 1, diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index cc08753..115576f 100644 --- a/internal/delivery/engine_integration_test.go +++ b/internal/delivery/engine_integration_test.go @@ -10,7 +10,6 @@ import ( "net/http" "net/http/httptest" "os" - "path/filepath" "strings" "sync/atomic" "testing" @@ -19,12 +18,11 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gorm.io/driver/sqlite" "gorm.io/gorm" _ "modernc.org/sqlite" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/delivery" - "sneak.berlin/go/webhooker/internal/gormlog" ) // iSetup holds common integration test dependencies. @@ -45,12 +43,12 @@ func newISetup(t *testing.T) iSetup { wDB := iSeedWebhookDB(t, dbMgr, wID) return iSetup{ - MainDB: mainDB, + MainDB: mainDB.DB(), DBMgr: dbMgr, WebhookID: wID, WebhookDB: wDB, Engine: delivery.NewTestEngineWithDB( - database.NewTestDatabase(mainDB), + mainDB, dbMgr, slog.New(slog.NewTextHandler( os.Stderr, @@ -64,35 +62,16 @@ func newISetup(t *testing.T) iSetup { } } -func iMainDB(t *testing.T) *gorm.DB { +// iMainDB opens a main database through database.Open, the way the +// service opens it, so these tests cannot pass against journal and +// locking settings production does not use. +func iMainDB(t *testing.T) *database.Database { t.Helper() - dbPath := filepath.Join( - t.TempDir(), "main-test.db", - ) - - // Opened the way the service opens the main database, so these - // tests cannot pass against journal and locking settings - // production does not use. - sqlDB, err := database.OpenSQLite( - dbPath, database.SQLiteModeCreate, - ) + db, err := database.Open(t.TempDir(), slog.New(slog.DiscardHandler)) require.NoError(t, err) - t.Cleanup(func() { _ = sqlDB.Close() }) - - db, err := gorm.Open( - sqlite.Dialector{Conn: sqlDB}, - &gorm.Config{Logger: gormlog.New(slog.New(slog.DiscardHandler))}, - ) - require.NoError(t, err) - - require.NoError(t, db.AutoMigrate( - &database.Webhook{}, - &database.Target{}, - &database.User{}, - &database.Setting{}, - )) + t.Cleanup(func() { _ = db.Close() }) return db } @@ -102,7 +81,7 @@ func iDBManager( ) *database.WebhookDBManager { t.Helper() - return database.NewTestWebhookDBManager(t.TempDir()) + return databasetest.NewWebhookDBManager(t, t.TempDir()) } func iSeedWebhookDB( @@ -1154,17 +1133,17 @@ func TestRecoverInFlight_ReportsAMissingWebhookDatabase(t *testing.T) { mainDB := iMainDB(t) webhookID := uuid.New().String() - iCreateWebhook(t, mainDB, webhookID, "lost-database") + iCreateWebhook(t, mainDB.DB(), webhookID, "lost-database") var logs bytes.Buffer - dbMgr := database.NewTestWebhookDBManagerWithLogger( - t.TempDir(), slog.New(slog.NewTextHandler(&logs, nil)), + dbMgr := databasetest.NewWebhookDBManagerWithLogger( + t, t.TempDir(), slog.New(slog.NewTextHandler(&logs, nil)), ) t.Cleanup(func() { _ = dbMgr.CloseAll() }) engine := delivery.NewTestEngineWithDB( - database.NewTestDatabase(mainDB), dbMgr, + mainDB, dbMgr, slog.New(slog.DiscardHandler), &http.Client{Timeout: 5 * time.Second}, 1, ) @@ -1188,14 +1167,14 @@ func TestRecoverInFlight_SkipsAWebhookDeletedAfterTheListIsRead( mainDB := iMainDB(t) webhookID := uuid.New().String() - iCreateWebhook(t, mainDB, webhookID, "deleted-during-recovery") + iCreateWebhook(t, mainDB.DB(), webhookID, "deleted-during-recovery") // The first query to return is recovery's read of the list of // webhooks. Deleting the webhook right after it puts the delete // between that read and the opening of the webhook's database. deleted := false - require.NoError(t, mainDB.Callback().Query().After("gorm:query"). + require.NoError(t, mainDB.DB().Callback().Query().After("gorm:query"). Register("delete-after-list", func(*gorm.DB) { if deleted { return @@ -1203,16 +1182,16 @@ func TestRecoverInFlight_SkipsAWebhookDeletedAfterTheListIsRead( deleted = true - require.NoError(t, mainDB.Delete( + require.NoError(t, mainDB.DB().Delete( &database.Webhook{}, "id = ?", webhookID, ).Error) })) - dbMgr := database.NewTestWebhookDBManager(t.TempDir()) + dbMgr := databasetest.NewWebhookDBManager(t, t.TempDir()) t.Cleanup(func() { _ = dbMgr.CloseAll() }) engine := delivery.NewTestEngineWithDB( - database.NewTestDatabase(mainDB), dbMgr, + mainDB, dbMgr, slog.New(slog.DiscardHandler), &http.Client{Timeout: 5 * time.Second}, 1, ) diff --git a/internal/delivery/queue_depth_gormlog_test.go b/internal/delivery/queue_depth_gormlog_test.go index 8c89734..51ccd75 100644 --- a/internal/delivery/queue_depth_gormlog_test.go +++ b/internal/delivery/queue_depth_gormlog_test.go @@ -5,7 +5,6 @@ import ( "context" "log/slog" "net/http" - "path/filepath" "strings" "sync" "testing" @@ -14,11 +13,9 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gorm.io/driver/sqlite" - "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/delivery" - "sneak.berlin/go/webhooker/internal/gormlog" ) // qdAggregateMarker identifies the queue-depth aggregate in the @@ -49,27 +46,13 @@ func (q *qdSyncBuf) String() string { // qdMainDB opens a main database whose GORM logger is the service's // adapter, writing through log. -func qdMainDB(t *testing.T, log *slog.Logger) *gorm.DB { +func qdMainDB(t *testing.T, log *slog.Logger) *database.Database { t.Helper() - sqlDB, err := database.OpenSQLite( - filepath.Join(t.TempDir(), "main-gormlog.db"), - database.SQLiteModeCreate, - ) + db, err := database.Open(t.TempDir(), log) require.NoError(t, err) - t.Cleanup(func() { _ = sqlDB.Close() }) - - db, err := gorm.Open( - sqlite.Dialector{Conn: sqlDB}, - &gorm.Config{Logger: gormlog.New(log)}, - ) - require.NoError(t, err) - - require.NoError(t, db.AutoMigrate( - &database.Webhook{}, - &database.Target{}, - )) + t.Cleanup(func() { _ = db.Close() }) return db } @@ -106,18 +89,18 @@ func TestQueueDepthSample_LogsNoBoundValue(t *testing.T) { )) mainDB := qdMainDB(t, log) - dbMgr := database.NewTestWebhookDBManagerWithLogger( - t.TempDir(), log, + dbMgr := databasetest.NewWebhookDBManagerWithLogger( + t, t.TempDir(), log, ) webhookID := uuid.New().String() webhookDB := iSeedWebhookDB(t, dbMgr, webhookID) - iCreateWebhook(t, mainDB, webhookID, "queue-depth-gormlog") + iCreateWebhook(t, mainDB.DB(), webhookID, "queue-depth-gormlog") targetID := uuid.New().String() - iCreateTarget(t, mainDB, targetID, webhookID, + iCreateTarget(t, mainDB.DB(), targetID, webhookID, "queue-depth-gormlog-target", database.TargetTypeHTTP, iHTTPConfig("https://example.com/hook"), 3, ) @@ -136,7 +119,7 @@ func TestQueueDepthSample_LogsNoBoundValue(t *testing.T) { ) engine := delivery.NewTestEngineWithDB( - database.NewTestDatabase(mainDB), + mainDB, dbMgr, log, &http.Client{Timeout: 5 * time.Second}, diff --git a/internal/delivery/target_database_rotation_test.go b/internal/delivery/target_database_rotation_test.go index 8bfb7c3..667a23b 100644 --- a/internal/delivery/target_database_rotation_test.go +++ b/internal/delivery/target_database_rotation_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/database/databasetest" "sneak.berlin/go/webhooker/internal/delivery" ) @@ -335,7 +336,7 @@ func TestArchivePathAt(t *testing.T) { t.Parallel() dataDir := t.TempDir() - dbMgr := database.NewTestWebhookDBManager(dataDir) + dbMgr := databasetest.NewWebhookDBManager(t, dataDir) webhook := &database.Webhook{ BaseModel: database.BaseModel{ID: "wh-id"}, Name: "Orders", } diff --git a/internal/gormlog/firstboot_test.go b/internal/gormlog/firstboot_test.go index c511eb4..337aed5 100644 --- a/internal/gormlog/firstboot_test.go +++ b/internal/gormlog/firstboot_test.go @@ -3,6 +3,7 @@ package gormlog_test import ( "context" "database/sql" + "log/slog" "os" "path/filepath" "testing" @@ -13,6 +14,7 @@ import ( "go.uber.org/fx/fxtest" _ "modernc.org/sqlite" // Pure Go SQLite driver. "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/config/configtest" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/logger" @@ -128,7 +130,7 @@ func readFirstBootSecrets( func bootAtDebug(t *testing.T, dataDir string) string { t.Helper() - config.ClearEnvForTest(t) + configtest.ClearEnv(t) t.Setenv("DEBUG", "true") t.Setenv("DATA_DIR", dataDir) @@ -145,6 +147,7 @@ func bootAtDebug(t *testing.T, dataDir string) string { fx.Provide( globals.New, logger.New, + func(l *logger.Logger) *slog.Logger { return l.Get() }, config.New, database.New, session.New, diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index a834342..2cbeaf1 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "html/template" + "log/slog" "net/http" "net/http/httptest" "sync" @@ -249,6 +250,7 @@ func newTestAppWithConfig( fx.Provide( globals.New, logger.New, + func(l *logger.Logger) *slog.Logger { return l.Get() }, func() *config.Config { return cfg }, database.New, database.NewWebhookDBManager, diff --git a/internal/handlers/profile_test.go b/internal/handlers/profile_test.go index 8b8be2d..8468358 100644 --- a/internal/handlers/profile_test.go +++ b/internal/handlers/profile_test.go @@ -15,7 +15,7 @@ import ( "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/logger" - "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" "sneak.berlin/go/webhooker/internal/session" ) @@ -135,7 +135,7 @@ func TestUserRoute_Unauthenticated_RedirectedByMiddleware(t *testing.T) { t.Cleanup(app.RequireStop) - mw := middleware.NewForTest(log.Get(), cfg, sess) + mw := middlewaretest.New(t, log.Get(), cfg, sess) var handlerReached bool diff --git a/internal/handlers/target_create_query_test.go b/internal/handlers/target_create_query_test.go index d8c542c..3eb3914 100644 --- a/internal/handlers/target_create_query_test.go +++ b/internal/handlers/target_create_query_test.go @@ -15,7 +15,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) // targetSecretSegments are the path segments of an incoming-webhook @@ -65,7 +65,8 @@ func postTargetCreate( t.Helper() logBuf := new(bytes.Buffer) - mw := middleware.NewForTest( + mw := middlewaretest.New( + t, slog.New(slog.NewJSONHandler( logBuf, &slog.HandlerOptions{Level: slog.LevelInfo}, )), diff --git a/internal/handlers/target_download_test.go b/internal/handlers/target_download_test.go index 755e6bc..f52b6c7 100644 --- a/internal/handlers/target_download_test.go +++ b/internal/handlers/target_download_test.go @@ -24,7 +24,7 @@ import ( "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/delivery" - "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) // errClientGone is the write failure of a client that has gone away. @@ -310,7 +310,8 @@ func limitedServer( const sendBuffer = 4 << 10 logBuf := new(bytes.Buffer) - mw := middleware.NewForTest( + mw := middlewaretest.New( + t, slog.New(slog.NewJSONHandler(logBuf, nil)), &config.Config{Environment: config.EnvironmentDev}, nil, diff --git a/internal/middleware/accesslog_test.go b/internal/middleware/accesslog_test.go index f4b8639..4a6cb4a 100644 --- a/internal/middleware/accesslog_test.go +++ b/internal/middleware/accesslog_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) // floodRequests is the number of distinct invented paths each flood @@ -83,7 +84,7 @@ func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) { TrustedProxies: trustedProxies("192.0.2.1/32"), } - return middleware.NewForTest(log, cfg, nil), buf + return middlewaretest.New(t, log, cfg, nil), buf } // capturingTextMiddleware is capturingMiddleware for the other handler @@ -107,7 +108,7 @@ func capturingTextMiddleware( TrustedProxies: trustedProxies("192.0.2.1/32"), } - return middleware.NewForTest(log, cfg, nil), buf + return middlewaretest.New(t, log, cfg, nil), buf } // accessLogRouter mirrors the production route shapes that an diff --git a/internal/middleware/clientip_test.go b/internal/middleware/clientip_test.go index 82450a7..06ab297 100644 --- a/internal/middleware/clientip_test.go +++ b/internal/middleware/clientip_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) const ( @@ -133,8 +134,8 @@ func clientLogLines( TrustedProxies: trustedProxies(trustedProxyCIDR), } - m := middleware.NewForTest( - log, cfg, newTestSessionManager(cfg, log, nil), + m := middlewaretest.New( + t, log, cfg, newTestSessionManager(t, cfg), ) handler := m.Logging()(site.build(m)) diff --git a/internal/middleware/logbound_test.go b/internal/middleware/logbound_test.go index 47b8416..1690564 100644 --- a/internal/middleware/logbound_test.go +++ b/internal/middleware/logbound_test.go @@ -41,6 +41,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) // bodyLimitBytes is the MaxBodySize cap these tests install. Any @@ -155,9 +156,9 @@ func capturingBoundMiddleware( ReceiverRateLimit: receiverLimitPerMinute, } - sess := newTestSessionManager(cfg, log, nil) + sess := newTestSessionManager(t, cfg) - return middleware.NewForTest(log, cfg, sess), buf + return middlewaretest.New(t, log, cfg, sess), buf } // unreachable is a next-handler that fails the test if the middleware diff --git a/internal/middleware/metrics.go b/internal/middleware/metrics.go index af092e0..1479590 100644 --- a/internal/middleware/metrics.go +++ b/internal/middleware/metrics.go @@ -151,7 +151,7 @@ var _ httpmetrics.Recorder = boundedLabelRecorder{} // Metrics returns middleware that records Prometheus HTTP metrics // with the Middleware's one recorder, which New builds on the registry -// the /metrics route serves and NewForTest on a registry of its own. +// it is given: in the application, the one the /metrics route serves. // Every call reuses that recorder, so any number of routers can // install it. func (s *Middleware) Metrics() func(http.Handler) http.Handler { diff --git a/internal/middleware/metrics_test.go b/internal/middleware/metrics_test.go index 3cea24a..169be6b 100644 --- a/internal/middleware/metrics_test.go +++ b/internal/middleware/metrics_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) const ( @@ -70,8 +71,8 @@ func metricsTestRouter( Environment: "prod", ReceiverRateLimit: receiverLimit, } - m := middleware.NewForTest( - log, cfg, newTestSessionManager(cfg, log, nil), + m := middlewaretest.New( + t, log, cfg, newTestSessionManager(t, cfg), ) reg := prometheus.NewRegistry() @@ -455,11 +456,11 @@ func TestMetrics_StatusAndSizeStillRecorded(t *testing.T) { ) } -// TestMetrics_WorksOnNewForTestMiddleware pins that a Middleware built -// by NewForTest has a recorder of its own: its Metrics() serves a -// request instead of panicking, and a second one does not collide -// with the first. -func TestMetrics_WorksOnNewForTestMiddleware(t *testing.T) { +// TestMetrics_WorksOnMiddlewaretestNew pins that a Middleware built +// by middlewaretest.New has a recorder of its own: its Metrics() +// serves a request instead of panicking, and a second one does not +// collide with the first. +func TestMetrics_WorksOnMiddlewaretestNew(t *testing.T) { t.Parallel() log := slog.New(slog.DiscardHandler) @@ -469,7 +470,7 @@ func TestMetrics_WorksOnNewForTestMiddleware(t *testing.T) { }) for range 2 { - h := middleware.NewForTest(log, cfg, nil).Metrics()(ok) + h := middlewaretest.New(t, log, cfg, nil).Metrics()(ok) req := httptest.NewRequestWithContext( t.Context(), http.MethodGet, okRoute, nil, diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 930abbf..e70bfdd 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -22,7 +22,6 @@ import ( "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/logfield" - "sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/session" ) @@ -155,7 +154,7 @@ const ( type MiddlewareParams struct { fx.In - Logger *logger.Logger + Logger *slog.Logger Globals *globals.Globals Config *config.Config Session *session.Session @@ -169,12 +168,10 @@ type Middleware struct { params *MiddlewareParams session *session.Session - // metricsRecorder records the inbound HTTP metrics. New builds - // it on the registry /metrics serves, NewForTest on a registry - // of its own. Either way it is built once per Middleware and - // Metrics reuses it, because building it registers its - // collectors, and a second registration on the same registry - // panics. + // metricsRecorder records the inbound HTTP metrics on + // params.Registry. It is built once per Middleware and Metrics + // reuses it, because building it registers its collectors, and a + // second registration on the same registry panics. metricsRecorder httpmetrics.Recorder // loginGuard counts failed credential verifications and bounds @@ -193,7 +190,7 @@ func New( ) (*Middleware, error) { s := new(Middleware) s.params = ¶ms - s.log = params.Logger.Get() + s.log = params.Logger s.session = params.Session s.metricsRecorder = prommetrics.NewRecorder( prommetrics.Config{Registry: params.Registry}, diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index 4482232..717f3d4 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -14,36 +14,34 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/fx/fxtest" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" "sneak.berlin/go/webhooker/internal/session" ) const testKeySize = 32 // testMiddleware creates a Middleware with minimal dependencies -// for testing. It uses a real session.Session backed by an -// in-memory cookie store. +// for testing. It uses a real session.Session. func testMiddleware( t *testing.T, env string, ) (*middleware.Middleware, *session.Session) { t.Helper() - m, s, _ := testMiddlewareWithSessionClock(t, env, 0, nil) - - return m, s + return testMiddlewareWithIdleTimeout(t, env, 0) } -// testMiddlewareWithSessionClock is testMiddleware with a -// configurable session idle timeout and a manually advanced clock, -// for the session-expiry tests. A nil clock uses the real one. -func testMiddlewareWithSessionClock( +// testMiddlewareWithIdleTimeout is testMiddleware with a +// configurable session idle timeout, for the session-expiry tests. +func testMiddlewareWithIdleTimeout( t *testing.T, env string, idleTimeout time.Duration, - clock *fakeClock, -) (*middleware.Middleware, *session.Session, *fakeClock) { +) (*middleware.Middleware, *session.Session) { t.Helper() log := slog.New(slog.NewTextHandler( @@ -56,59 +54,44 @@ func testMiddlewareWithSessionClock( SessionIdleTimeout: idleTimeout, } - sessManager := newTestSessionManager(cfg, log, clock) + sessManager := newTestSessionManager(t, cfg) - m := middleware.NewForTest(log, cfg, sessManager) + m := middlewaretest.New(t, log, cfg, sessManager) - return m, sessManager, clock + return m, sessManager } // newTestSessionManager builds the real session.Session the -// middleware tests run against: an in-memory cookie store with a -// known key, and optionally a manually advanced clock. +// middleware tests run against, through session.New, with its key +// in a main database of its own. func newTestSessionManager( + t *testing.T, cfg *config.Config, - log *slog.Logger, - clock *fakeClock, ) *session.Session { - key := make([]byte, testKeySize) + t.Helper() - for i := range key { - key[i] = byte(i) - } + discard := slog.New(slog.DiscardHandler) - store := session.NewStore(key) + db, err := database.Open(t.TempDir(), discard) + require.NoError(t, err) - var now func() time.Time + t.Cleanup(func() { _ = db.Close() }) - if clock != nil { - now = clock.Now - } + lc := fxtest.NewLifecycle(t) - return session.NewForTest(store, cfg, log, key, now) -} + sessManager, err := session.New(lc, session.Params{ + Config: cfg, + Database: db, + Logger: discard, + }) + require.NoError(t, err) -// fakeClock is a manually advanced clock, so session expiry can be -// tested without sleeping. -type fakeClock struct { - t time.Time -} + // The start hook reads the key from db and builds the cookie + // store. + lc.RequireStart() + t.Cleanup(lc.RequireStop) -func (c *fakeClock) Now() time.Time { - return c.t -} - -func (c *fakeClock) Advance(d time.Duration) { - c.t = c.t.Add(d) -} - -// newFakeClock returns a clock started at a fixed instant. -func newFakeClock() *fakeClock { - return &fakeClock{ - t: time.Date( - 2026, time.January, 2, 3, 4, 5, 0, time.UTC, - ), - } + return sessManager } // --- Logging Middleware Tests --- @@ -583,6 +566,40 @@ func sessionCookies( return out } +// aged re-issues the session cookie in cookies with both of its +// timestamps moved back by d: the cookie as it stands once d has +// passed, so session expiry can be tested without sleeping. +func aged( + t *testing.T, + sessManager *session.Session, + cookies []*http.Cookie, + d time.Duration, +) []*http.Cookie { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil) + + for _, c := range cookies { + req.AddCookie(c) + } + + sess, err := sessManager.Get(req) + require.NoError(t, err) + + for _, key := range []string{session.CreatedAtKey, session.LastSeenKey} { + at, ok := sess.Values[key].(int64) + require.True(t, ok, "the session has no %s", key) + + sess.Values[key] = at - int64(d/time.Second) + } + + w := httptest.NewRecorder() + require.NoError(t, sessManager.Save(req, w, sess)) + + return sessionCookies(w) +} + func TestRequireAuth_IdleExpiredSession_RedirectsToLogin( t *testing.T, ) { @@ -590,13 +607,11 @@ func TestRequireAuth_IdleExpiredSession_RedirectsToLogin( idle := time.Hour - m, sessManager, clock := testMiddlewareWithSessionClock( - t, config.EnvironmentDev, idle, newFakeClock(), + m, sessManager := testMiddlewareWithIdleTimeout( + t, config.EnvironmentDev, idle, ) - cookies := loginCookies(t, sessManager) - - clock.Advance(idle) + cookies := aged(t, sessManager, loginCookies(t, sessManager), idle) called, w := runAuthed(t, m, cookies) @@ -621,14 +636,12 @@ func TestRequireAuth_RefreshesIdleDeadlineOnActivity( idle := time.Hour - m, sessManager, clock := testMiddlewareWithSessionClock( - t, config.EnvironmentDev, idle, newFakeClock(), + m, sessManager := testMiddlewareWithIdleTimeout( + t, config.EnvironmentDev, idle, ) - cookies := loginCookies(t, sessManager) - // Activity halfway through the idle window. - clock.Advance(idle / 2) + cookies := aged(t, sessManager, loginCookies(t, sessManager), idle/2) called, w := runAuthed(t, m, cookies) require.True(t, called, "handler should run while valid") @@ -640,16 +653,22 @@ func TestRequireAuth_RefreshesIdleDeadlineOnActivity( ) // Past the original deadline. The refreshed cookie is still - // good; the original one is not. - clock.Advance(idle - time.Second) + // good; the original one is not. A minute short of the idle + // window leaves room for the real clock, which the session + // reads, to tick on while the test runs. + later := idle - time.Minute - calledRefreshed, _ := runAuthed(t, m, refreshed) + calledRefreshed, _ := runAuthed( + t, m, aged(t, sessManager, refreshed, later), + ) assert.True( t, calledRefreshed, "refreshed session should outlive the original deadline", ) - calledStale, staleW := runAuthed(t, m, cookies) + calledStale, staleW := runAuthed( + t, m, aged(t, sessManager, cookies, later), + ) assert.False( t, calledStale, "the pre-refresh cookie carries the old idle deadline", @@ -662,8 +681,8 @@ func TestRequireAuth_UnauthenticatedRequestDoesNotRefresh( ) { t.Parallel() - m, sessManager, _ := testMiddlewareWithSessionClock( - t, config.EnvironmentDev, time.Hour, newFakeClock(), + m, sessManager := testMiddlewareWithIdleTimeout( + t, config.EnvironmentDev, time.Hour, ) // A session cookie that exists but was never authenticated. @@ -924,12 +943,9 @@ func metricsAuthMiddleware( MetricsPassword: "secret", } - key := make([]byte, testKeySize) - store := session.NewStore(key) - - sessManager := session.NewForTest(store, cfg, log, key, nil) - - return middleware.NewForTest(log, cfg, sessManager) + return middlewaretest.New( + t, log, cfg, newTestSessionManager(t, cfg), + ) } // runMetricsAuthRequest sends a GET /metrics request with the diff --git a/internal/middleware/middlewaretest/middlewaretest.go b/internal/middleware/middlewaretest/middlewaretest.go new file mode 100644 index 0000000..73dd976 --- /dev/null +++ b/internal/middleware/middlewaretest/middlewaretest.go @@ -0,0 +1,42 @@ +// Package middlewaretest builds a Middleware for tests in other +// packages. +package middlewaretest + +import ( + "log/slog" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + "go.uber.org/fx/fxtest" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/session" +) + +// New builds a Middleware through middleware.New, on a +// lifecycle that is never started. +// +// Its metrics recorder writes to a fresh registry of its own, so +// Metrics() works on it and two of them never collide. +func New( + t *testing.T, + log *slog.Logger, + cfg *config.Config, + sess *session.Session, +) *middleware.Middleware { + t.Helper() + + m, err := middleware.New( + fxtest.NewLifecycle(t), + middleware.MiddlewareParams{ + Logger: log, + Config: cfg, + Session: sess, + Registry: prometheus.NewRegistry(), + }, + ) + require.NoError(t, err) + + return m +} diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 373ca32..082641f 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/middleware/middlewaretest" ) func TestPostRateLimit_AllowsGET(t *testing.T) { @@ -198,7 +199,7 @@ func rateLimitMiddleware( &slog.HandlerOptions{Level: slog.LevelDebug}, )) - return middleware.NewForTest(log, cfg, nil) + return middlewaretest.New(t, log, cfg, nil) } // trustedProxies parses CIDR strings for a test Config. diff --git a/internal/middleware/testing.go b/internal/middleware/testing.go deleted file mode 100644 index cc862b2..0000000 --- a/internal/middleware/testing.go +++ /dev/null @@ -1,32 +0,0 @@ -package middleware - -import ( - "log/slog" - - "github.com/prometheus/client_golang/prometheus" - prommetrics "github.com/slok/go-http-metrics/metrics/prometheus" - "sneak.berlin/go/webhooker/internal/config" - "sneak.berlin/go/webhooker/internal/session" -) - -// NewForTest creates a Middleware with the minimum dependencies -// needed for testing. This bypasses the fx lifecycle. -// -// Its metrics recorder writes to a fresh registry of its own, so -// Metrics() works on it and two of them never collide. -func NewForTest( - log *slog.Logger, - cfg *config.Config, - sess *session.Session, -) *Middleware { - return &Middleware{ - log: log, - params: &MiddlewareParams{ - Config: cfg, - }, - session: sess, - metricsRecorder: prommetrics.NewRecorder( - prommetrics.Config{Registry: prometheus.NewRegistry()}, - ), - } -} diff --git a/internal/resetpw/resetpw_test.go b/internal/resetpw/resetpw_test.go index 99ba5ed..fe4fdce 100644 --- a/internal/resetpw/resetpw_test.go +++ b/internal/resetpw/resetpw_test.go @@ -174,6 +174,7 @@ func newServerApp( fx.Provide( globals.New, logger.New, + func(l *logger.Logger) *slog.Logger { return l.Get() }, func() *config.Config { return &config.Config{DataDir: dir} }, diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 27e8cc9..a74be16 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -3,6 +3,7 @@ package server_test import ( "context" "html" + "log/slog" "net/http" "net/http/httptest" "net/url" @@ -130,6 +131,7 @@ func newTestEnvWithConfig( fx.Provide( globals.New, logger.New, + func(l *logger.Logger) *slog.Logger { return l.Get() }, func() *config.Config { return cfg }, database.New, database.NewWebhookDBManager, diff --git a/internal/session/testing.go b/internal/session/export_test.go similarity index 92% rename from internal/session/testing.go rename to internal/session/export_test.go index 18ce298..46eb119 100644 --- a/internal/session/testing.go +++ b/internal/session/export_test.go @@ -16,8 +16,7 @@ func NewStore(key []byte) *sessions.CookieStore { } // NewForTest creates a Session with a pre-configured cookie store for use -// in tests. This bypasses the fx lifecycle and database dependency, allowing -// middleware and handler tests to use real session functionality. The key +// in tests. This bypasses the fx lifecycle and database dependency. The key // parameter is the raw 32-byte authentication key used for session encryption // and CSRF cookie signing. // diff --git a/internal/session/session.go b/internal/session/session.go index 8e97734..5335000 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -16,7 +16,6 @@ import ( "go.uber.org/fx" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" - "sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/reqtls" ) @@ -80,7 +79,7 @@ type Params struct { Config *config.Config Database *database.Database - Logger *logger.Logger + Logger *slog.Logger } // Session manages encrypted session storage. @@ -180,7 +179,7 @@ func New( params Params, ) (*Session, error) { s := &Session{ - log: params.Logger.Get(), + log: params.Logger, idleTimeout: params.Config.SessionIdleTimeout, now: time.Now, }