Isolate config tests from the shell; any out-of-range PORT is ErrInvalidPort (closes #94)

Config tests that unset a variable now restore it when they end, and
every test that builds a Config first unsets every variable the config
reads, so a value exported in the developer's shell cannot change the
result. TestEnvPort checks that the bad value appears in its errors.

A PORT of zero or below now wraps ErrInvalidPort, as one above 65535
already did.

The README configuration table and the Settings page now say that a
RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative,
fails startup.

Model: opus-5-5

README.md

@@ -2924,8 +2924,7 @@ webhooker/
 │   ├── resetpw/
 │   │   └── 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
+│   │   └── config.go          # Configuration loading from environment variables
 │   ├── database/
 │   │   ├── base_model.go      # BaseModel with UUID primary keys
 │   │   ├── database.go        # GORM connection, migrations, admin seed

internal/config/config.go

@@ -362,24 +362,22 @@ func envPositiveInt(
 // envPort returns the value of the named environment variable parsed
 // as a TCP port number. Returns defaultValue if not set. A set value
 // that is unparseable, below 1, or above maxPort is a hard error
-// naming the key and the bad value; every out-of-range value wraps
-// ErrInvalidPort, including one too large or too small for an int.
+// naming the key and the bad value; the two out-of-range cases both
+// wrap ErrInvalidPort.
 func envPort(key string, defaultValue int) (int, error) {
 	v := os.Getenv(key)
 	if v == "" {
 		return defaultValue, nil
 	}
 
-	// strconv.ErrRange means a number too large or too small for an
-	// int, which is outside the port range as well.
 	port, err := strconv.Atoi(v)
-	if err != nil && !errors.Is(err, strconv.ErrRange) {
+	if err != nil {
 		return 0, fmt.Errorf(
 			"invalid integer for %s: %q: %w", key, v, err,
 		)
 	}
 
-	if err != nil || port < 1 || port > maxPort {
+	if port < 1 || port > maxPort {
 		return 0, fmt.Errorf(
 			"%w: %s must be from 1 to %d, got %q",
 			ErrInvalidPort, key, maxPort, v,

internal/config/config_test.go

@@ -70,7 +70,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)
+			unsetEnv(t, configEnvKeys()...)
 
 			if tt.envValue != "" {
 				t.Setenv(
@@ -196,7 +196,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
@@ -335,7 +335,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
@@ -388,7 +388,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)
+			unsetEnv(t, configEnvKeys()...)
 
 			if env != "" {
 				t.Setenv("WEBHOOKER_ENVIRONMENT", env)
@@ -433,9 +433,9 @@ 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)
-
-			if set != "" {
+			if set == "" {
+				unsetEnv(t, "DATA_DIR")
+			} else {
 				t.Setenv("DATA_DIR", set)
 			}
 
@@ -498,7 +498,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
@@ -614,7 +614,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
@@ -725,7 +725,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
@@ -797,7 +797,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev)
 
 			if tt.allowed != "" {
@@ -933,7 +933,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)
+			unsetEnv(t, configEnvKeys()...)
 
 			if tt.username.set {
 				t.Setenv("METRICS_USERNAME", tt.username.value)

internal/config/dotenv_test.go

@@ -39,9 +39,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 // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetEnv(t, dotEnvKey)
 
 	absent := filepath.Join(t.TempDir(), config.DotEnvPath)
 	require.NoError(t, config.LoadDotEnvFileForTest(absent))
@@ -54,9 +54,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 // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_AppliesValues(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetEnv(t, dotEnvKey)
 
 	path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n")
 
@@ -82,9 +82,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 // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetEnv(t, dotEnvKey)
 
 	path := writeDotEnv(
 		t, malformedDotEnv+dotEnvKey+"=from-dot-env\n",
@@ -132,7 +132,7 @@ func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) {
 //
 //nolint:paralleltest // t.Chdir moves the whole process.
 func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetEnv(t, dotEnvKey)
 
 	dir := t.TempDir()
 	require.NoError(t, os.WriteFile(

internal/config/env_test.go

@@ -1,6 +1,7 @@
 package config_test
 
 import (
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -37,6 +38,41 @@ const (
 	bindAddressSample = "10.1.2.3"
 )
 
+// configEnvKeys is every variable config.New reads. Its tests unset
+// all of them before setting the ones under test, so a variable
+// exported in the developer's shell cannot change their outcome.
+func configEnvKeys() []string {
+	return []string{
+		"WEBHOOKER_ENVIRONMENT",
+		envKeyPort,
+		envKeyBindAddress,
+		"DATA_DIR",
+		envKeyDebug,
+		"METRICS_USERNAME",
+		"METRICS_PASSWORD",
+		envKeySentryDSN,
+		"RETENTION_SWEEP_INTERVAL",
+		"SESSION_IDLE_TIMEOUT",
+		"RECEIVER_RATE_LIMIT",
+		"TRUSTED_PROXIES",
+		"ALLOWED_EGRESS_CIDRS",
+	}
+}
+
+// unsetEnv makes each key absent for the rest of the test and puts
+// back whatever it held when the test ends. t.Setenv registers that
+// restore; the Unsetenv after it is what makes the key absent, since
+// a key set to the empty string is still present, and godotenv will
+// not overwrite a present key.
+func unsetEnv(t *testing.T, keys ...string) {
+	t.Helper()
+
+	for _, key := range keys {
+		t.Setenv(key, "")
+		require.NoError(t, os.Unsetenv(key))
+	}
+}
+
 // envBoolCase is one row of the envBool table.
 type envBoolCase struct {
 	name         string
@@ -120,10 +156,10 @@ 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)
-
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
+			} else {
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvBoolForTest(
@@ -144,62 +180,17 @@ func TestEnvBool(t *testing.T) {
 	}
 }
 
-// envIntCase is one row of the envPositiveInt and envPort tables.
-type envIntCase struct {
-	name        string
-	set         bool
-	value       string
-	expectError bool
-	errIs       error
-	expected    int
-}
-
-// runEnvIntCases runs each row through parse, which is
-// envPositiveInt or envPort, with testEnvKey set to the row's value
-// or left unset.
-func runEnvIntCases(
-	t *testing.T,
-	parse func(key string, defaultValue int) (int, error),
-	defaultValue int,
-	tests []envIntCase,
-) {
-	t.Helper()
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			config.ClearEnvForTest(t)
-
-			if tt.set {
-				t.Setenv(testEnvKey, tt.value)
-			}
-
-			got, err := parse(testEnvKey, defaultValue)
-
-			if tt.expectError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), testEnvKey)
-				assert.Contains(t, err.Error(), tt.value)
-
-				if tt.errIs != nil {
-					require.ErrorIs(t, err, tt.errIs)
-				}
-
-				return
-			}
-
-			require.NoError(t, err)
-			assert.Equal(t, tt.expected, got)
-		})
-	}
-}
-
-//nolint:paralleltest // runEnvIntCases uses t.Setenv.
 func TestEnvPositiveInt(t *testing.T) {
 	const defaultValue = 7
 
-	runEnvIntCases(t, config.EnvPositiveIntForTest, defaultValue, []envIntCase{
+	tests := []struct {
+		name        string
+		set         bool
+		value       string
+		expectError bool
+		errIs       error
+		expected    int
+	}{
 		{
 			name:     "unset returns the default integer",
 			expected: defaultValue,
@@ -236,14 +227,52 @@ func TestEnvPositiveInt(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrNonPositiveValue,
 		},
-	})
+	}
+
+	//nolint:dupl // TestEnvPort makes the same checks, on purpose.
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			if tt.set {
+				t.Setenv(testEnvKey, tt.value)
+			} else {
+				unsetEnv(t, testEnvKey)
+			}
+
+			got, err := config.EnvPositiveIntForTest(
+				testEnvKey, defaultValue,
+			)
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), testEnvKey)
+				assert.Contains(t, err.Error(), tt.value)
+
+				if tt.errIs != nil {
+					require.ErrorIs(t, err, tt.errIs)
+				}
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
 }
 
-//nolint:paralleltest // runEnvIntCases uses t.Setenv.
 func TestEnvPort(t *testing.T) {
 	const defaultValue = 8080
 
-	runEnvIntCases(t, config.EnvPortForTest, defaultValue, []envIntCase{
+	tests := []struct {
+		name        string
+		set         bool
+		value       string
+		expectError bool
+		errIs       error
+		expected    int
+	}{
 		{
 			name:     "unset returns the default port",
 			expected: defaultValue,
@@ -273,13 +302,6 @@ func TestEnvPort(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrInvalidPort,
 		},
-		{
-			name:        "negative is rejected",
-			set:         true,
-			value:       "-1",
-			expectError: true,
-			errIs:       config.ErrInvalidPort,
-		},
 		{
 			name:        "above the port range is rejected",
 			set:         true,
@@ -287,14 +309,39 @@ func TestEnvPort(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrInvalidPort,
 		},
-		{
-			name:        "too large for an int is rejected",
-			set:         true,
-			value:       "99999999999999999999",
-			expectError: true,
-			errIs:       config.ErrInvalidPort,
-		},
-	})
+	}
+
+	//nolint:dupl // TestEnvPositiveInt makes the same checks, on purpose.
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			if tt.set {
+				t.Setenv(testEnvKey, tt.value)
+			} else {
+				unsetEnv(t, testEnvKey)
+			}
+
+			got, err := config.EnvPortForTest(
+				testEnvKey, defaultValue,
+			)
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), testEnvKey)
+				assert.Contains(t, err.Error(), tt.value)
+
+				if tt.errIs != nil {
+					require.ErrorIs(t, err, tt.errIs)
+				}
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
 }
 
 // TestEnvBindAddress covers BIND_ADDRESS parsing.
@@ -310,10 +357,10 @@ 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)
-
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
+			} else {
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvBindAddressForTest(
@@ -476,7 +523,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)
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 			t.Setenv(tt.key, tt.value)
 
@@ -638,7 +685,7 @@ func sentryEnvValueCases() []badEnvValueCase {
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
 func TestNewUsesDefaultsWhenUnset(t *testing.T) {
-	config.ClearEnvForTest(t)
+	unsetEnv(t, configEnvKeys()...)
 	t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 	cfg, err := buildConfig(t)

internal/config/sentry_test.go

@@ -100,10 +100,10 @@ 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)
-
 			if tt.set {
 				t.Setenv(envKeySentryDSN, tt.value)
+			} else {
+				unsetEnv(t, envKeySentryDSN)
 			}
 
 			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)

internal/config/testing.go

@@ -1,30 +0,0 @@
-package config
-
-import (
-	"os"
-	"strings"
-	"testing"
-)
-
-// ClearEnvForTest unsets every variable in the process environment
-// for the rest of the test and puts each back when the test ends, so
-// a test sees only the variables it sets itself, not whatever the
-// developer's shell exports.
-func ClearEnvForTest(t *testing.T) {
-	t.Helper()
-
-	for _, entry := range os.Environ() {
-		key, _, _ := strings.Cut(entry, "=")
-
-		// t.Setenv registers the restore; the Unsetenv after it is
-		// what makes the key absent, since a key set to the empty
-		// string is still present, and godotenv will not overwrite a
-		// present key.
-		t.Setenv(key, "")
-
-		err := os.Unsetenv(key)
-		if err != nil {
-			t.Fatalf("unsetting %s: %v", key, err)
-		}
-	}
-}

internal/database/totals_test.go

@@ -102,13 +102,6 @@ func TestWebhookDBManager_TotalsSurviveReopen(t *testing.T) {
 // seedExpiredEvents stores count events created at the given time,
 // each with a delivered delivery to one target and a failed delivery
 // to the other, and one attempt for each delivery.
-//
-// It and seedBareEvents insert 50 rows per statement, not more. The
-// SQLite driver looks up each parameter's value by scanning the
-// statement's arguments from the first until it reaches that
-// parameter's, so the time to bind a statement grows with the square of
-// its parameter count: at 500 rows, several thousand parameters, the
-// seeding took most of these tests' time under -race.
 func seedExpiredEvents(
 	t *testing.T,
 	db *gorm.DB,
@@ -145,8 +138,8 @@ func seedExpiredEvents(
 		)
 	}
 
-	require.NoError(t, db.CreateInBatches(events, 50).Error)
-	require.NoError(t, db.CreateInBatches(deliveries, 50).Error)
+	require.NoError(t, db.CreateInBatches(events, 500).Error)
+	require.NoError(t, db.CreateInBatches(deliveries, 500).Error)
 
 	results := make([]database.DeliveryResult, len(deliveries))
 	for i := range deliveries {
@@ -155,7 +148,7 @@ func seedExpiredEvents(
 		}
 	}
 
-	require.NoError(t, db.CreateInBatches(results, 50).Error)
+	require.NoError(t, db.CreateInBatches(results, 500).Error)
 }
 
 // seedBareEvents stores count events created at the given time, with
@@ -179,7 +172,7 @@ func seedBareEvents(
 		events[i].CreatedAt = createdAt
 	}
 
-	require.NoError(t, db.CreateInBatches(events, 50).Error)
+	require.NoError(t, db.CreateInBatches(events, 500).Error)
 }
 
 // TestRetentionReaper_PrunesMoreThanOneBatch verifies that a prune

internal/gormlog/firstboot_test.go

@@ -117,8 +117,8 @@ func readFirstBootSecrets(
 }
 
 // bootAtDebug starts and stops the real application graph against
-// dataDir with DEBUG=true and nothing else set, and returns everything
-// it wrote to standard output.
+// dataDir with DEBUG=true, and returns everything it wrote to standard
+// output.
 //
 // config.New reads DEBUG from the environment exactly as the binary
 // does, internal/logger builds the handler it builds in production,
@@ -128,7 +128,6 @@ func readFirstBootSecrets(
 func bootAtDebug(t *testing.T, dataDir string) string {
 	t.Helper()
 
-	config.ClearEnvForTest(t)
 	t.Setenv("DEBUG", "true")
 	t.Setenv("DATA_DIR", dataDir)
 

script/test

@@ -27,11 +27,7 @@
 # Those figures predate tests hashing the admin password at 1 MB instead of
 # 64 MB (https://git.eeqj.de/sneak/webhooker/pulls/404). After that change, in
 # a cache-defeated build at host load 44-109 (2026-10-02), internal/handlers
-# took 8.5s and the slowest package was internal/database at 15.8s. Once its
-# retention tests seeded 50 rows per insert instead of 500
-# (https://git.eeqj.de/sneak/webhooker/issues/198), internal/database took
-# 7.3s and the slowest package was internal/handlers at 8.1s to 10.0s, at host
-# load 25-48 (2026-10-02).
+# took 8.5s and the slowest package was internal/database at 15.8s.
 #
 # -p 4 -parallel 8 keep the run under 2 GB of memory: at most four test
 # binaries build or run at once, each with at most eight parallel tests. Under