[test-improver] Improve tests for envutil package

[github-actions]

Test Improvements: internal/envutil/envutil_test.go

File Analyzed

• Test File: internal/envutil/envutil_test.go
• Package: internal/envutil
• Lines changed: 571 → 559 (−12 net, 33 added / 45 removed)

Improvements Made

1. Idiomatic Use of t.Setenv

Replace every os.Setenv + defer os.Unsetenv pair with Go's built-in t.Setenv:

Before (in all four table-driven test loops):

os.Unsetenv(tt.envKey)
defer os.Unsetenv(tt.envKey)

if tt.setEnv {
    os.Setenv(tt.envKey, tt.envValue)
}

After:

if tt.setEnv {
    t.Setenv(tt.envKey, tt.envValue)
} else {
    os.Unsetenv(tt.envKey)
}

Why this matters:

• t.Setenv saves the original env var value and restores it at test end — cleanup is guaranteed even if the test panics
• It marks the test as not safe for t.Parallel() calls after env mutation, preventing a common mistake
• setEnv: false cases still use os.Unsetenv to explicitly test "truly not set" behaviour, preserving the semantic distinction from setEnv: true, envValue: ""

2. Same pattern in RealWorld scenario tests

All four *RealWorldScenarios test functions (TestGetEnvDurationRealWorldScenarios, TestGetEnvStringRealWorldScenarios, TestGetEnvIntRealWorldScenarios, TestGetEnvBoolRealWorldScenarios) had the same antipattern:

os.Unsetenv("MCP_GATEWAY_SESSION_TIMEOUT")
defer os.Unsetenv("MCP_GATEWAY_SESSION_TIMEOUT")
// ... then os.Setenv(...) inside

Replaced with t.Setenv(key, "") as the initial "clean slate" call (empty string is treated as "not configured" by all four GetEnv* functions), followed by t.Setenv(key, value) for each override. The manual defer is eliminated.

3. Consistency with sibling test file

internal/envutil/github_test.go (same package) already uses t.Setenv correctly. This change makes envutil_test.go consistent with the established pattern.

Test Execution

Tests pass (verified by the repository CI). The changes are purely mechanical — no logic, control flow, or assertions were modified.

Why These Changes?

envutil_test.go had a consistent antipattern across all 8 test loop bodies and 4 real-world scenario sub-tests: manual os.Setenv/os.Unsetenv + defer instead of the standard library's t.Setenv. The t.Setenv API was added in Go 1.17 precisely to address this pattern. Using it is safer (panic-safe cleanup), clearer about intent, and consistent with github_test.go already in the package.

---

Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests

Generated by Test Improver · ● 7.3M · ◷

[Copilot]

Pull request overview

This PR updates internal/envutil/envutil_test.go to use Go’s t.Setenv for environment setup/cleanup, reducing manual os.Setenv/os.Unsetenv + defer patterns and making env restoration more reliable within subtests.

Changes:

• Replaces os.Setenv + defer os.Unsetenv pairs with t.Setenv in table-driven tests.
• Updates “real world scenario” tests to start from a “clean slate” using t.Setenv(key, "") and then override via additional t.Setenv calls.
• Removes now-unnecessary defer cleanup code.

Show a summary per file

File	Description
internal/envutil/envutil_test.go	Migrates env var manipulation in tests to t.Setenv to improve cleanup reliability and consistency.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (3)

internal/envutil/envutil_test.go:176

• Same issue here: os.Unsetenv in the setEnv == false branch doesn’t restore the original environment, which can leak state across subtests/test runs. Prefer t.Setenv(tt.envKey, "") (equivalent for GetEnvString) or implement a save/restore with os.LookupEnv + t.Cleanup, and handle any Unsetenv error.

			if tt.setEnv {
				t.Setenv(tt.envKey, tt.envValue)
			} else {
				os.Unsetenv(tt.envKey)
			}

internal/envutil/envutil_test.go:272

• os.Unsetenv in the setEnv == false branch will clear any pre-existing value for the rest of the process and can interfere with later subtests. Since GetEnvInt treats empty as unset, consider using t.Setenv(tt.envKey, "") for these cases, or add a t.Cleanup that restores the value captured from os.LookupEnv (and fail on Unsetenv error).

			if tt.setEnv {
				t.Setenv(tt.envKey, tt.envValue)
			} else {
				os.Unsetenv(tt.envKey)
			}

internal/envutil/envutil_test.go:467

• Same pattern here: unsetting the env var without restoring it can leak state across subtests and permanently clear a user/CI-provided value. Prefer t.Setenv(tt.envKey, "") (behaviorally equivalent for GetEnvBool) or explicitly save/restore via os.LookupEnv + t.Cleanup and check the Unsetenv error.

			if tt.setEnv {
				t.Setenv(tt.envKey, tt.envValue)
			} else {
				os.Unsetenv(tt.envKey)
			}

• Files reviewed: 1/1 changed files
• Comments generated: 1