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

internal/config/config.go

@@ -80,8 +80,7 @@ const (
 	// process over a Docker network or a private LAN connects from.
 	defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
 
-	// maxPort is the highest valid TCP port number. The lower
-	// bound (at least 1) is enforced by envPositiveInt.
+	// maxPort is the highest valid TCP port number.
 	maxPort = 65535
 
 	// mappedV4Offset is the number of leading bits an IPv4-mapped
@@ -105,7 +104,7 @@ var ErrInvalidEnvironment = errors.New("invalid environment")
 var ErrNonPositiveValue = errors.New("value must be positive")
 
 // ErrInvalidPort is returned when an environment variable holding a
-// TCP port number is set above the valid port range.
+// TCP port number is set to a number outside 1 to 65535.
 var ErrInvalidPort = errors.New("invalid port")
 
 // ErrInvalidCIDR is returned when an environment variable holding a
@@ -363,17 +362,25 @@ 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.
+// naming the key and the bad value; the two out-of-range cases both
+// wrap ErrInvalidPort.
 func envPort(key string, defaultValue int) (int, error) {
-	port, err := envPositiveInt(key, defaultValue)
-	if err != nil {
-		return 0, err
+	v := os.Getenv(key)
+	if v == "" {
+		return defaultValue, nil
 	}
 
-	if port > maxPort {
+	port, err := strconv.Atoi(v)
+	if err != nil {
 		return 0, fmt.Errorf(
-			"%w: %s must be at most %d, got %d",
-			ErrInvalidPort, key, maxPort, port,
+			"invalid integer for %s: %q: %w", key, v, err,
+		)
+	}
+
+	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

@@ -3,7 +3,6 @@ package config_test
 import (
 	"bytes"
 	"log/slog"
-	"os"
 	"testing"
 	"time"
 
@@ -71,14 +70,12 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
+
 			if tt.envValue != "" {
 				t.Setenv(
 					"WEBHOOKER_ENVIRONMENT", tt.envValue,
 				)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"WEBHOOKER_ENVIRONMENT",
-				))
 			}
 
 			for k, v := range tt.envVars {
@@ -199,14 +196,11 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("RETENTION_SWEEP_INTERVAL", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"RETENTION_SWEEP_INTERVAL",
-				))
 			}
 
 			if tt.expectError {
@@ -341,14 +335,11 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("SESSION_IDLE_TIMEOUT", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"SESSION_IDLE_TIMEOUT",
-				))
 			}
 
 			if tt.expectError {
@@ -397,16 +388,12 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
+
 			if env != "" {
 				t.Setenv("WEBHOOKER_ENVIRONMENT", env)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"WEBHOOKER_ENVIRONMENT",
-				))
 			}
 
-			require.NoError(t, os.Unsetenv("DATA_DIR"))
-
 			var cfg *config.Config
 
 			app := fxtest.New(
@@ -447,7 +434,7 @@ func TestDataDirHelper(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
 			if set == "" {
-				require.NoError(t, os.Unsetenv("DATA_DIR"))
+				unsetEnv(t, "DATA_DIR")
 			} else {
 				t.Setenv("DATA_DIR", set)
 			}
@@ -511,14 +498,11 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("RECEIVER_RATE_LIMIT", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"RECEIVER_RATE_LIMIT",
-				))
 			}
 
 			if tt.expectError {
@@ -630,12 +614,11 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("TRUSTED_PROXIES", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv("TRUSTED_PROXIES"))
 			}
 
 			if tt.expectError {
@@ -742,14 +725,11 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("ALLOWED_EGRESS_CIDRS", tt.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
-				)
 			}
 
 			if tt.expectError {
@@ -817,13 +797,10 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev)
 
-			if tt.allowed == "" {
-				require.NoError(
-					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
-				)
-			} else {
+			if tt.allowed != "" {
 				t.Setenv("ALLOWED_EGRESS_CIDRS", tt.allowed)
 			}
 
@@ -956,20 +933,14 @@ 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.
+			unsetEnv(t, configEnvKeys()...)
+
 			if tt.username.set {
 				t.Setenv("METRICS_USERNAME", tt.username.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("METRICS_USERNAME"),
-				)
 			}
 
 			if tt.password.set {
 				t.Setenv("METRICS_PASSWORD", tt.password.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("METRICS_PASSWORD"),
-				)
 			}
 
 			if tt.expectError {

internal/config/dotenv_test.go

@@ -22,17 +22,6 @@ const malformedDotEnv = "PORT 19615\n" +
 	"this is not = valid ! syntax\n" +
 	"\"unclosed\n"
 
-// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of
-// the test and restores it afterwards. t.Setenv registers the restore;
-// the Unsetenv that follows is what the test actually needs, because a
-// variable set to the empty string is still present in os.Environ and
-// godotenv would refuse to overwrite it.
-func unsetDotEnvKey(t *testing.T) {
-	t.Helper()
-	t.Setenv(dotEnvKey, "placeholder")
-	require.NoError(t, os.Unsetenv(dotEnvKey))
-}
-
 // writeDotEnv writes contents to a .env file in a fresh temporary
 // directory and returns its path.
 func writeDotEnv(t *testing.T, contents string) string {
@@ -50,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 // unsetDotEnvKey uses t.Setenv.
+//nolint:paralleltest // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	absent := filepath.Join(t.TempDir(), config.DotEnvPath)
 	require.NoError(t, config.LoadDotEnvFileForTest(absent))
@@ -65,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 // unsetDotEnvKey uses t.Setenv.
+//nolint:paralleltest // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_AppliesValues(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n")
 
@@ -93,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 // unsetDotEnvKey uses t.Setenv.
+//nolint:paralleltest // unsetEnv uses t.Setenv.
 func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	path := writeDotEnv(
 		t, malformedDotEnv+dotEnvKey+"=from-dot-env\n",
@@ -143,7 +132,7 @@ func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) {
 //
 //nolint:paralleltest // t.Chdir moves the whole process.
 func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) {
-	unsetDotEnvKey(t)
+	unsetEnv(t, dotEnvKey)
 
 	dir := t.TempDir()
 	require.NoError(t, os.WriteFile(

internal/config/env_test.go

@@ -38,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
@@ -124,7 +159,7 @@ func TestEnvBool(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvBoolForTest(
@@ -194,6 +229,7 @@ func TestEnvPositiveInt(t *testing.T) {
 		},
 	}
 
+	//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
@@ -201,7 +237,7 @@ func TestEnvPositiveInt(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvPositiveIntForTest(
@@ -264,7 +300,7 @@ func TestEnvPort(t *testing.T) {
 			set:         true,
 			value:       "0",
 			expectError: true,
-			errIs:       config.ErrNonPositiveValue,
+			errIs:       config.ErrInvalidPort,
 		},
 		{
 			name:        "above the port range is rejected",
@@ -275,6 +311,7 @@ func TestEnvPort(t *testing.T) {
 		},
 	}
 
+	//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
@@ -282,7 +319,7 @@ func TestEnvPort(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvPortForTest(
@@ -292,6 +329,7 @@ func TestEnvPort(t *testing.T) {
 			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)
@@ -322,7 +360,7 @@ func TestEnvBindAddress(t *testing.T) {
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
+				unsetEnv(t, testEnvKey)
 			}
 
 			got, err := config.EnvBindAddressForTest(
@@ -485,6 +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.
+			unsetEnv(t, configEnvKeys()...)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 			t.Setenv(tt.key, tt.value)
 
@@ -646,14 +685,9 @@ func sentryEnvValueCases() []badEnvValueCase {
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
 func TestNewUsesDefaultsWhenUnset(t *testing.T) {
+	unsetEnv(t, configEnvKeys()...)
 	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)

internal/config/sentry_test.go

@@ -1,7 +1,6 @@
 package config_test
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -104,7 +103,7 @@ func TestEnvSentryDSN(t *testing.T) {
 			if tt.set {
 				t.Setenv(envKeySentryDSN, tt.value)
 			} else {
-				require.NoError(t, os.Unsetenv(envKeySentryDSN))
+				unsetEnv(t, envKeySentryDSN)
 			}
 
 			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)