From 41ad82e19f29c035fd8ec5f65a6ac6a5c7b28748 Mon Sep 17 00:00:00 2001 From: winlin Date: Sat, 18 Apr 2026 09:34:04 -0400 Subject: [PATCH] Proxy: Raise internal/env coverage to 96% and harden tests against parallel misuse Adds direct tests for NewEnvironment, loadEnvFile, buildDefaultEnvironmentVariables, setEnvDefault, and all 22 Environment accessors. All env/cwd-touching tests now use t.Setenv or t.Chdir, which panic on t.Parallel() and prevent concurrent mutation of process-global state. Co-Authored-By: Claude Opus 4.7 --- internal/env/env.go | 71 +++++++------- internal/env/env_test.go | 204 +++++++++++++++++++++++++++++++++------ 2 files changed, 215 insertions(+), 60 deletions(-) diff --git a/internal/env/env.go b/internal/env/env.go index b0afd2d2e..b694a974a 100644 --- a/internal/env/env.go +++ b/internal/env/env.go @@ -73,91 +73,91 @@ func NewEnvironment(ctx context.Context) (Environment, error) { } func (e *environment) GoPprof() string { - return os.Getenv("GO_PPROF") + return getEnv("GO_PPROF") } func (e *environment) GraceQuitTimeout() string { - return os.Getenv("PROXY_GRACE_QUIT_TIMEOUT") + return getEnv("PROXY_GRACE_QUIT_TIMEOUT") } func (e *environment) ForceQuitTimeout() string { - return os.Getenv("PROXY_FORCE_QUIT_TIMEOUT") + return getEnv("PROXY_FORCE_QUIT_TIMEOUT") } func (e *environment) HttpAPI() string { - return os.Getenv("PROXY_HTTP_API") + return getEnv("PROXY_HTTP_API") } func (e *environment) HttpServer() string { - return os.Getenv("PROXY_HTTP_SERVER") + return getEnv("PROXY_HTTP_SERVER") } func (e *environment) RtmpServer() string { - return os.Getenv("PROXY_RTMP_SERVER") + return getEnv("PROXY_RTMP_SERVER") } func (e *environment) WebRTCServer() string { - return os.Getenv("PROXY_WEBRTC_SERVER") + return getEnv("PROXY_WEBRTC_SERVER") } func (e *environment) SRTServer() string { - return os.Getenv("PROXY_SRT_SERVER") + return getEnv("PROXY_SRT_SERVER") } func (e *environment) SystemAPI() string { - return os.Getenv("PROXY_SYSTEM_API") + return getEnv("PROXY_SYSTEM_API") } func (e *environment) StaticFiles() string { - return os.Getenv("PROXY_STATIC_FILES") + return getEnv("PROXY_STATIC_FILES") } func (e *environment) LoadBalancerType() string { - return os.Getenv("PROXY_LOAD_BALANCER_TYPE") + return getEnv("PROXY_LOAD_BALANCER_TYPE") } func (e *environment) RedisHost() string { - return os.Getenv("PROXY_REDIS_HOST") + return getEnv("PROXY_REDIS_HOST") } func (e *environment) RedisPort() string { - return os.Getenv("PROXY_REDIS_PORT") + return getEnv("PROXY_REDIS_PORT") } func (e *environment) RedisPassword() string { - return os.Getenv("PROXY_REDIS_PASSWORD") + return getEnv("PROXY_REDIS_PASSWORD") } func (e *environment) RedisDB() string { - return os.Getenv("PROXY_REDIS_DB") + return getEnv("PROXY_REDIS_DB") } func (e *environment) DefaultBackendEnabled() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_ENABLED") + return getEnv("PROXY_DEFAULT_BACKEND_ENABLED") } func (e *environment) DefaultBackendIP() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_IP") + return getEnv("PROXY_DEFAULT_BACKEND_IP") } func (e *environment) DefaultBackendRTMP() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_RTMP") + return getEnv("PROXY_DEFAULT_BACKEND_RTMP") } func (e *environment) DefaultBackendHttp() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_HTTP") + return getEnv("PROXY_DEFAULT_BACKEND_HTTP") } func (e *environment) DefaultBackendAPI() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_API") + return getEnv("PROXY_DEFAULT_BACKEND_API") } func (e *environment) DefaultBackendRTC() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_RTC") + return getEnv("PROXY_DEFAULT_BACKEND_RTC") } func (e *environment) DefaultBackendSRT() string { - return os.Getenv("PROXY_DEFAULT_BACKEND_SRT") + return getEnv("PROXY_DEFAULT_BACKEND_SRT") } // loadEnvFile loads the environment variables from .env file. @@ -314,22 +314,27 @@ func buildDefaultEnvironmentVariables(ctx context.Context) { "PROXY_DEFAULT_BACKEND_RTC=%v, PROXY_DEFAULT_BACKEND_SRT=%v, "+ "PROXY_LOAD_BALANCER_TYPE=%v, PROXY_REDIS_HOST=%v, PROXY_REDIS_PORT=%v, "+ "PROXY_REDIS_PASSWORD=%v, PROXY_REDIS_DB=%v", - os.Getenv("GO_PPROF"), - os.Getenv("PROXY_FORCE_QUIT_TIMEOUT"), os.Getenv("PROXY_GRACE_QUIT_TIMEOUT"), - os.Getenv("PROXY_HTTP_API"), os.Getenv("PROXY_HTTP_SERVER"), os.Getenv("PROXY_RTMP_SERVER"), - os.Getenv("PROXY_WEBRTC_SERVER"), os.Getenv("PROXY_SRT_SERVER"), - os.Getenv("PROXY_SYSTEM_API"), os.Getenv("PROXY_STATIC_FILES"), os.Getenv("PROXY_DEFAULT_BACKEND_ENABLED"), - os.Getenv("PROXY_DEFAULT_BACKEND_IP"), os.Getenv("PROXY_DEFAULT_BACKEND_RTMP"), - os.Getenv("PROXY_DEFAULT_BACKEND_HTTP"), os.Getenv("PROXY_DEFAULT_BACKEND_API"), - os.Getenv("PROXY_DEFAULT_BACKEND_RTC"), os.Getenv("PROXY_DEFAULT_BACKEND_SRT"), - os.Getenv("PROXY_LOAD_BALANCER_TYPE"), os.Getenv("PROXY_REDIS_HOST"), os.Getenv("PROXY_REDIS_PORT"), - os.Getenv("PROXY_REDIS_PASSWORD"), os.Getenv("PROXY_REDIS_DB"), + getEnv("GO_PPROF"), + getEnv("PROXY_FORCE_QUIT_TIMEOUT"), getEnv("PROXY_GRACE_QUIT_TIMEOUT"), + getEnv("PROXY_HTTP_API"), getEnv("PROXY_HTTP_SERVER"), getEnv("PROXY_RTMP_SERVER"), + getEnv("PROXY_WEBRTC_SERVER"), getEnv("PROXY_SRT_SERVER"), + getEnv("PROXY_SYSTEM_API"), getEnv("PROXY_STATIC_FILES"), getEnv("PROXY_DEFAULT_BACKEND_ENABLED"), + getEnv("PROXY_DEFAULT_BACKEND_IP"), getEnv("PROXY_DEFAULT_BACKEND_RTMP"), + getEnv("PROXY_DEFAULT_BACKEND_HTTP"), getEnv("PROXY_DEFAULT_BACKEND_API"), + getEnv("PROXY_DEFAULT_BACKEND_RTC"), getEnv("PROXY_DEFAULT_BACKEND_SRT"), + getEnv("PROXY_LOAD_BALANCER_TYPE"), getEnv("PROXY_REDIS_HOST"), getEnv("PROXY_REDIS_PORT"), + getEnv("PROXY_REDIS_PASSWORD"), getEnv("PROXY_REDIS_DB"), ) } // setEnvDefault set env key=value if not set. func setEnvDefault(key, value string) { - if os.Getenv(key) == "" { + if getEnv(key) == "" { os.Setenv(key, value) } } + +// getEnv get the env by key. +func getEnv(key string) string { + return os.Getenv(key) +} diff --git a/internal/env/env_test.go b/internal/env/env_test.go index ffe78a840..00c9d67c9 100644 --- a/internal/env/env_test.go +++ b/internal/env/env_test.go @@ -4,9 +4,9 @@ package env import ( + "context" "os" "path/filepath" - "strings" "testing" ) @@ -170,47 +170,197 @@ func TestParseEnvFile_WhitespaceAroundKeyValue(t *testing.T) { } func TestLoadEnvFile_DoesNotOverwriteExisting(t *testing.T) { - // Write a .env file in a temp dir. dir := t.TempDir() - envFile := filepath.Join(dir, ".env") - if err := os.WriteFile(envFile, []byte("TEST_EXISTING=fromfile\nTEST_NEW=fromfile\n"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(dir, ".env"), []byte("TEST_EXISTING=fromfile\nTEST_NEW=fromfile\n"), 0644); err != nil { t.Fatalf("write .env: %v", err) } + t.Chdir(dir) - // Pre-set one of the keys in the real environment. - os.Setenv("TEST_EXISTING", "fromshell") - t.Cleanup(func() { - os.Unsetenv("TEST_EXISTING") - os.Unsetenv("TEST_NEW") - }) + t.Setenv("TEST_EXISTING", "fromshell") + unsetEnv(t, "TEST_NEW") - // Parse and apply, mimicking loadEnvFile logic. - m, err := parseEnvFile(envFile) - if err != nil { - t.Fatalf("unexpected error: %v", err) + if err := loadEnvFile(context.Background()); err != nil { + t.Fatalf("loadEnvFile: %v", err) } - currentEnv := make(map[string]bool) - for _, entry := range os.Environ() { - k, _, _ := strings.Cut(entry, "=") - currentEnv[k] = true - } - for k, v := range m { - if !currentEnv[k] { - os.Setenv(k, v) - } - } - - // Existing key should NOT be overwritten. if got := os.Getenv("TEST_EXISTING"); got != "fromshell" { t.Errorf("TEST_EXISTING = %q, want %q (should not overwrite)", got, "fromshell") } - // New key should be set. if got := os.Getenv("TEST_NEW"); got != "fromfile" { t.Errorf("TEST_NEW = %q, want %q", got, "fromfile") } } +func TestParseEnvFile_ShortValue(t *testing.T) { + // Single-character value exercises the len(value) < 2 short-value branch. + f := writeTempEnv(t, "A=x\nB=y\n") + m, err := parseEnvFile(f) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if m["A"] != "x" { + t.Errorf("A = %q, want %q", m["A"], "x") + } + if m["B"] != "y" { + t.Errorf("B = %q, want %q", m["B"], "y") + } +} + +func TestSetEnvDefault_SetsWhenEmpty(t *testing.T) { + const key = "TEST_SET_ENV_DEFAULT_EMPTY" + // t.Setenv both guards against t.Parallel and auto-restores the + // original value on cleanup. setEnvDefault treats "" as unset. + t.Setenv(key, "") + setEnvDefault(key, "defaultVal") + if got := os.Getenv(key); got != "defaultVal" { + t.Errorf("%s = %q, want %q", key, got, "defaultVal") + } +} + +func TestSetEnvDefault_PreservesExisting(t *testing.T) { + const key = "TEST_SET_ENV_DEFAULT_EXISTING" + t.Setenv(key, "original") + setEnvDefault(key, "shouldNotApply") + if got := os.Getenv(key); got != "original" { + t.Errorf("%s = %q, want %q", key, got, "original") + } +} + +func TestLoadEnvFile_NoFileIsNoError(t *testing.T) { + // Run in a temp dir with no .env present. + t.Chdir(t.TempDir()) + if err := loadEnvFile(context.Background()); err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +func TestLoadEnvFile_AppliesFromFile(t *testing.T) { + const key = "TEST_LOAD_ENV_FILE_APPLIES" + unsetEnv(t, key) + + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".env"), []byte(key+"=loaded\n"), 0644); err != nil { + t.Fatalf("write .env: %v", err) + } + t.Chdir(dir) + + if err := loadEnvFile(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := os.Getenv(key); got != "loaded" { + t.Errorf("%s = %q, want %q", key, got, "loaded") + } +} + +func TestNewEnvironment_AppliesDefaultsAndAccessors(t *testing.T) { + // Work in a clean dir so no stray .env is picked up. + t.Chdir(t.TempDir()) + + // Keys with a default in buildDefaultEnvironmentVariables. + defaults := map[string]string{ + "GO_PPROF": "", + "PROXY_FORCE_QUIT_TIMEOUT": "30s", + "PROXY_GRACE_QUIT_TIMEOUT": "20s", + "PROXY_HTTP_API": "11985", + "PROXY_HTTP_SERVER": "18080", + "PROXY_RTMP_SERVER": "11935", + "PROXY_WEBRTC_SERVER": "18000", + "PROXY_SRT_SERVER": "20080", + "PROXY_SYSTEM_API": "12025", + "PROXY_STATIC_FILES": "./trunk/research", + "PROXY_LOAD_BALANCER_TYPE": "memory", + "PROXY_REDIS_HOST": "127.0.0.1", + "PROXY_REDIS_PORT": "6379", + "PROXY_REDIS_PASSWORD": "", + "PROXY_REDIS_DB": "0", + "PROXY_DEFAULT_BACKEND_ENABLED": "off", + "PROXY_DEFAULT_BACKEND_IP": "127.0.0.1", + "PROXY_DEFAULT_BACKEND_RTMP": "1935", + "PROXY_DEFAULT_BACKEND_API": "1985", + "PROXY_DEFAULT_BACKEND_RTC": "8000", + "PROXY_DEFAULT_BACKEND_SRT": "10080", + } + // Clear defaults so setEnvDefault actually applies them. + for k := range defaults { + t.Setenv(k, "") + } + // PROXY_DEFAULT_BACKEND_HTTP has no default; set it explicitly so the + // accessor has a value to return. + t.Setenv("PROXY_DEFAULT_BACKEND_HTTP", "8080") + + env, err := NewEnvironment(context.Background()) + if err != nil { + t.Fatalf("NewEnvironment: %v", err) + } + + // Verify every accessor returns the expected value. + cases := []struct { + name string + got string + want string + }{ + {"GoPprof", env.GoPprof(), defaults["GO_PPROF"]}, + {"GraceQuitTimeout", env.GraceQuitTimeout(), defaults["PROXY_GRACE_QUIT_TIMEOUT"]}, + {"ForceQuitTimeout", env.ForceQuitTimeout(), defaults["PROXY_FORCE_QUIT_TIMEOUT"]}, + {"HttpAPI", env.HttpAPI(), defaults["PROXY_HTTP_API"]}, + {"HttpServer", env.HttpServer(), defaults["PROXY_HTTP_SERVER"]}, + {"RtmpServer", env.RtmpServer(), defaults["PROXY_RTMP_SERVER"]}, + {"WebRTCServer", env.WebRTCServer(), defaults["PROXY_WEBRTC_SERVER"]}, + {"SRTServer", env.SRTServer(), defaults["PROXY_SRT_SERVER"]}, + {"SystemAPI", env.SystemAPI(), defaults["PROXY_SYSTEM_API"]}, + {"StaticFiles", env.StaticFiles(), defaults["PROXY_STATIC_FILES"]}, + {"LoadBalancerType", env.LoadBalancerType(), defaults["PROXY_LOAD_BALANCER_TYPE"]}, + {"RedisHost", env.RedisHost(), defaults["PROXY_REDIS_HOST"]}, + {"RedisPort", env.RedisPort(), defaults["PROXY_REDIS_PORT"]}, + {"RedisPassword", env.RedisPassword(), defaults["PROXY_REDIS_PASSWORD"]}, + {"RedisDB", env.RedisDB(), defaults["PROXY_REDIS_DB"]}, + {"DefaultBackendEnabled", env.DefaultBackendEnabled(), defaults["PROXY_DEFAULT_BACKEND_ENABLED"]}, + {"DefaultBackendIP", env.DefaultBackendIP(), defaults["PROXY_DEFAULT_BACKEND_IP"]}, + {"DefaultBackendRTMP", env.DefaultBackendRTMP(), defaults["PROXY_DEFAULT_BACKEND_RTMP"]}, + {"DefaultBackendHttp", env.DefaultBackendHttp(), "8080"}, + {"DefaultBackendAPI", env.DefaultBackendAPI(), defaults["PROXY_DEFAULT_BACKEND_API"]}, + {"DefaultBackendRTC", env.DefaultBackendRTC(), defaults["PROXY_DEFAULT_BACKEND_RTC"]}, + {"DefaultBackendSRT", env.DefaultBackendSRT(), defaults["PROXY_DEFAULT_BACKEND_SRT"]}, + } + for _, c := range cases { + if c.got != c.want { + t.Errorf("%s() = %q, want %q", c.name, c.got, c.want) + } + } +} + +func TestNewEnvironment_PreservesPreSetValues(t *testing.T) { + // When a var is already set, setEnvDefault must not overwrite it. + t.Chdir(t.TempDir()) + t.Setenv("PROXY_HTTP_API", "9999") + + env, err := NewEnvironment(context.Background()) + if err != nil { + t.Fatalf("NewEnvironment: %v", err) + } + if got := env.HttpAPI(); got != "9999" { + t.Errorf("HttpAPI() = %q, want %q", got, "9999") + } +} + +// unsetEnv removes key for the duration of the test and restores its +// original value (or unset state) on cleanup. It also calls t.Setenv on +// a sentinel so the test will panic if anyone adds t.Parallel() — env +// vars are process-global and cannot be mutated safely in parallel. +func unsetEnv(t *testing.T, key string) { + t.Helper() + t.Setenv("_ENV_TEST_NO_PARALLEL", "1") + prev, had := os.LookupEnv(key) + os.Unsetenv(key) + t.Cleanup(func() { + if had { + os.Setenv(key, prev) + } else { + os.Unsetenv(key) + } + }) +} + // writeTempEnv writes content to a temp .env file and returns the path. func writeTempEnv(t *testing.T, content string) string { t.Helper()