Skip to content

[test-improver] Improve tests for cmd/flags_logging#6871

Merged
lpcox merged 2 commits into
mainfrom
test-improver/improve-flags-logging-test-193f827a7f0ec5fb
Jun 2, 2026
Merged

[test-improver] Improve tests for cmd/flags_logging#6871
lpcox merged 2 commits into
mainfrom
test-improver/improve-flags-logging-test-193f827a7f0ec5fb

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot commented Jun 2, 2026

Test Improvements: flags_logging_test.go

File Analyzed

  • Test File: internal/cmd/flags_logging_test.go
  • Package: internal/cmd
  • Lines Changed: 133 → 123 (net -10 lines, cleaner)

Improvements Made

1. Better Testing Patterns

  • ✅ Replaced manual os.LookupEnv / os.Setenv / os.Unsetenv boilerplate (3 calls + t.Cleanup) with t.Setenv — which handles setup and restore automatically
  • ✅ Removed 18 lines of error-prone manual env teardown, replaced with 8 simpler lines

2. Increased Coverage

  • ✅ Added new test case: whitespace-only env var falls back to log-dir default
    • Tests the strings.TrimSpace(envValue) != "" branch in resolveWasmCacheDir when the env var is set to whitespace (e.g. " "), which was previously untested
  • Total TestResolveWasmCacheDir subtests: 7 → 8

3. Cleaner & More Stable Tests

  • t.Setenv guarantees automatic cleanup even if the test panics, unlike the manual t.Cleanup + os.Setenv/os.Unsetenv pattern
  • ✅ Removed duplicate manual env save/restore logic that appeared in two test cases
  • ✅ Tests remain fully parallel-safe (each subtest uses its own t.Setenv scope)

Test Execution

All tests pass:

--- PASS: TestResolveWasmCacheDir (0.00s)
    --- PASS: TestResolveWasmCacheDir/defaults_next_to_log_dir_when_no_override_is_set
    --- PASS: TestResolveWasmCacheDir/environment_override_is_used_when_flag_is_unchanged
    --- PASS: TestResolveWasmCacheDir/flag_override_takes_precedence_over_environment
    --- PASS: TestResolveWasmCacheDir/blank_flag_falls_back_to_environment_or_default
    --- PASS: TestResolveWasmCacheDir/whitespace-only_flag_and_unset_env_uses_log-dir_default
    --- PASS: TestResolveWasmCacheDir/flag_changed_with_valid_value_ignores_env_var
    --- PASS: TestResolveWasmCacheDir/whitespace-only_env_var_falls_back_to_log-dir_default  ← new
    --- PASS: TestResolveWasmCacheDir/flag_unchanged_and_empty_env_var_uses_log-dir_default
--- PASS: TestConfigureWasmCompilationCache (0.00s)

Why These Changes?

The two os.Unsetenv test cases were using a 7-line manual env save/restore pattern that is verbose, error-prone (must restore in t.Cleanup, easy to forget), and doesn't benefit from testing.T's built-in env management. Using t.Setenv(key, "") achieves the same observable behavior (the function checks trimmed != "" after strings.TrimSpace, so an empty string and an unset var both fall through to the default path).

The new whitespace-only env var test ensures the strings.TrimSpace guard in resolveWasmCacheDir is exercised for the env-var code path, complementing the existing whitespace-only flag test.


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

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • index.crates.io

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "index.crates.io"

See Network Configuration for more information.

Generated by Test Improver · sonnet46 5.1M ·

- Replace manual os.LookupEnv/Setenv/Unsetenv cleanup with t.Setenv
  for cleaner, safer test teardown
- Add missing edge case: whitespace-only env var should fall back to
  log-dir default (tests the trimmed != "" branch in resolveWasmCacheDir)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox lpcox marked this pull request as ready for review June 2, 2026 16:41
Copilot AI review requested due to automatic review settings June 2, 2026 16:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates internal/cmd/flags_logging_test.go to simplify environment variable setup/teardown and add coverage for the whitespace-only env-var path when resolving the WASM cache directory.

Changes:

  • Replace manual os.LookupEnv/t.Cleanup/os.Unsetenv boilerplate with t.Setenv in relevant subtests.
  • Add a new subtest covering whitespace-only MCP_GATEWAY_WASM_CACHE_DIR falling back to the default cache dir.
Show a summary per file
File Description
internal/cmd/flags_logging_test.go Refactors env-var handling to t.Setenv and adds a whitespace-only env-var test case for resolveWasmCacheDir.

Copilot's findings

Tip

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

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

Comment thread internal/cmd/flags_logging_test.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@lpcox lpcox merged commit 3d6fc3e into main Jun 2, 2026
16 checks passed
@lpcox lpcox deleted the test-improver/improve-flags-logging-test-193f827a7f0ec5fb branch June 2, 2026 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants