Skip to content

test(config): stop inherited provider keys from failing the suite - #3526

Open
olifarhaan wants to merge 1 commit into
charmbracelet:mainfrom
olifarhaan:fix-config-test-env-leak
Open

test(config): stop inherited provider keys from failing the suite#3526
olifarhaan wants to merge 1 commit into
charmbracelet:mainfrom
olifarhaan:fix-config-test-env-leak

Conversation

@olifarhaan

Copy link
Copy Markdown

What

go test ./internal/config fails if you have a Hyper key in your shell, and the failure diff prints that key to the terminal

Why it happened

TestRefreshOAuthToken_StalePeerBorrowsRotatedRefreshToken refreshes a token, and persisting it goes through SetConfigFields, which auto-reloads. The reload resolves HYPER_API_KEY from the environment and puts it back over the access token the refresh had just applied. CRUSH_HYPER_API_KEY breaks it the same way, since PushPopCrushEnv (load.go:182) copies CRUSH_FOO over FOO on every load

Fix

TestMain drops inherited *_API_KEY and CRUSH_* variables before running anything. Tests that need a credential keep setting one with t.Setenv

Closes #3448

Proof

$ CRUSH_HYPER_API_KEY=totally-fake-not-a-real-key go test ./internal/config -run TestRefreshOAuthToken_StalePeerBorrowsRotatedRefreshToken
--- FAIL: TestRefreshOAuthToken_StalePeerBorrowsRotatedRefreshToken (1.43s)
                expected: "at4"
                actual  : "totally-fake-not-a-real-key"
FAIL

$ CRUSH_HYPER_API_KEY=totally-fake-not-a-real-key go test ./internal/config -run TestRefreshOAuthToken_StalePeerBorrowsRotatedRefreshToken
ok      github.com/charmbracelet/crush/internal/config  1.802s

HYPER_API_KEY on its own reproduces it too

Tests

TestInheritedCredentialEnvNames pins which names get dropped, including that a MALFORMED entry and API_KEY_SUFFIXED_WRONG are left alone. TestMainClearedInheritedCredentials asserts nothing survived into the test process

go test -race ./internal/config/... and golangci-lint v2.11 clean

The issue also names two Venice tests, but neither exists on main anymore, so only the Hyper case still reproduces

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.

Tests fail if you have provider API keys set in your environment

1 participant