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

The config tests unset variables without restoring them and read whatever the developer's shell exported, so results could differ from one machine to the next. config.ClearEnvForTest, in internal/config/testing.go, unsets every variable in the process environment and, when the test ends, leaves it exactly as it found it; every config test and the first-boot test call it before setting their own. TestEnvPositiveInt and TestEnvPort share one table runner, and the port errors name the bad value. A PORT of zero, below zero, or too large to parse now matches ErrInvalidPort, as one above 65535 already did. The README and the Settings page say an unparseable or non-positive RETENTION_SWEEP_INTERVAL stops startup.

Model: opus-5-5

internal/config/env_test.go

@@ -1,7 +1,6 @@
 package config_test
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -121,10 +120,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 {
-				require.NoError(t, os.Unsetenv(testEnvKey))
 			}
 
 			got, err := config.EnvBoolForTest(
@@ -145,17 +144,62 @@ 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
 
-	tests := []struct {
-		name        string
-		set         bool
-		value       string
-		expectError bool
-		errIs       error
-		expected    int
-	}{
+	runEnvIntCases(t, config.EnvPositiveIntForTest, defaultValue, []envIntCase{
 		{
 			name:     "unset returns the default integer",
 			expected: defaultValue,
@@ -192,51 +236,14 @@ func TestEnvPositiveInt(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrNonPositiveValue,
 		},
-	}
-
-	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 {
-				require.NoError(t, os.Unsetenv(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
 
-	tests := []struct {
-		name        string
-		set         bool
-		value       string
-		expectError bool
-		errIs       error
-		expected    int
-	}{
+	runEnvIntCases(t, config.EnvPortForTest, defaultValue, []envIntCase{
 		{
 			name:     "unset returns the default port",
 			expected: defaultValue,
@@ -264,7 +271,14 @@ func TestEnvPort(t *testing.T) {
 			set:         true,
 			value:       "0",
 			expectError: true,
-			errIs:       config.ErrNonPositiveValue,
+			errIs:       config.ErrInvalidPort,
+		},
+		{
+			name:        "negative is rejected",
+			set:         true,
+			value:       "-1",
+			expectError: true,
+			errIs:       config.ErrInvalidPort,
 		},
 		{
 			name:        "above the port range is rejected",
@@ -273,37 +287,14 @@ func TestEnvPort(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrInvalidPort,
 		},
-	}
-
-	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 {
-				require.NoError(t, os.Unsetenv(testEnvKey))
-			}
-
-			got, err := config.EnvPortForTest(
-				testEnvKey, defaultValue,
-			)
-
-			if tt.expectError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), testEnvKey)
-
-				if tt.errIs != nil {
-					require.ErrorIs(t, err, tt.errIs)
-				}
-
-				return
-			}
-
-			require.NoError(t, err)
-			assert.Equal(t, tt.expected, got)
-		})
-	}
+		{
+			name:        "too large for an int is rejected",
+			set:         true,
+			value:       "99999999999999999999",
+			expectError: true,
+			errIs:       config.ErrInvalidPort,
+		},
+	})
 }
 
 // TestEnvBindAddress covers BIND_ADDRESS parsing.
@@ -319,10 +310,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 {
-				require.NoError(t, os.Unsetenv(testEnvKey))
 			}
 
 			got, err := config.EnvBindAddressForTest(
@@ -485,6 +476,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)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 			t.Setenv(tt.key, tt.value)
 
@@ -646,14 +638,9 @@ func sentryEnvValueCases() []badEnvValueCase {
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
 func TestNewUsesDefaultsWhenUnset(t *testing.T) {
+	config.ClearEnvForTest(t)
 	t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
-	for _, key := range []string{
-		envKeyPort, envKeyDebug, envKeyBindAddress, envKeySentryDSN,
-	} {
-		require.NoError(t, os.Unsetenv(key))
-	}
-
 	cfg, err := buildConfig(t)
 	require.NoError(t, err)
 	require.NotNil(t, cfg)