Skip to content

fix: isolate test environment from external variables#613

Merged
robertolopezlopez merged 4 commits into
mainfrom
fix/unit_test_env_coupled
May 26, 2026
Merged

fix: isolate test environment from external variables#613
robertolopezlopez merged 4 commits into
mainfrom
fix/unit_test_env_coupled

Conversation

@robertolopezlopez
Copy link
Copy Markdown
Contributor

@robertolopezlopez robertolopezlopez commented May 22, 2026

Description

This PR adds an extendable mechanism to ignore environment variables in unit tests.

Checklist

  • Tests added and all succeed (make test)
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint)
  • Test your changes work for the CLI
    1. Clone / pull the latest CLI main.
    2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
      • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
    3. Run go mod tidy in the cliv2 directory.
    4. Run the CLI tests and do any required manual testing.
    5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
    • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 22, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 22, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@robertolopezlopez robertolopezlopez marked this pull request as ready for review May 26, 2026 08:00
@robertolopezlopez robertolopezlopez requested review from a team as code owners May 26, 2026 08:00
@snyk-pr-review-bot

This comment has been minimized.

Comment thread pkg/configtest/env.go
Comment thread pkg/configtest/env.go
Comment thread pkg/configtest/env.go
Comment on lines +16 to +21
func IsolateEnvironmentForTest(t *testing.T, keys ...string) {
t.Helper()
toClear := keys
if len(toClear) == 0 {
toClear = KnownLeakEnvironmentKeys
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we ALWAYS consider ignoring the KnownLeakEnvironmentKeys values?
If so, we should concat always.
In the current way, if parameters for keys are passed, the KnownLeakEnvironmentKeys are not being considered

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this behavior has been documented in my last commit

@robertolopezlopez robertolopezlopez force-pushed the fix/unit_test_env_coupled branch from b4853bd to 30ab48f Compare May 26, 2026 12:17
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Partial Isolation 🟡 [minor]

The function IsolateEnvironmentForTest uses t.Setenv(k, "") to clear variables. While this overwrites the value, it still leaves the environment variable 'set' with an empty string value. If the configuration logic uses IsSet(key) to determine whether to use a default value, it will see the empty string as a user-provided value rather than falling back to the default. This may lead to unexpected behavior in tests that expect a 'nil' or 'unset' state to trigger default logic.

func IsolateEnvironmentForTest(t *testing.T, keys ...string) {
	t.Helper()
	toClear := keys
	if len(toClear) == 0 {
		toClear = KnownLeakEnvironmentKeys
	}
	for _, k := range toClear {
		if k == "" {
			continue
		}
		t.Setenv(k, "")
	}
📚 Repository Context Analyzed

This review considered 4 relevant code sections from 4 files (average relevance: 0.89)

@robertolopezlopez robertolopezlopez merged commit 94e5ec4 into main May 26, 2026
13 checks passed
@robertolopezlopez robertolopezlopez deleted the fix/unit_test_env_coupled branch May 26, 2026 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants