From d00eb80223ff581fa4cf0e24a4ac5dda94159caa Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Mon, 6 Apr 2026 09:07:02 -0700 Subject: [PATCH 1/2] fix: isolate auth tests to prevent credential deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestAuthLogoutCmd_NoPanic and TestAuthStatusCmd_NoConfig were running against real system state — no mock, no temp dir. The logout test called ClearConfig() which deleted ~/.glean/config.json and removed keyring entries, wiping real credentials on every test run. Isolate both tests with HOME set to a temp dir, ConfigPath swapped, and keyring mocked — same pattern as auth_persistence_test.go. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/auth_test.go | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/cmd/auth_test.go b/cmd/auth_test.go index b82b3c3..8a6a91f 100644 --- a/cmd/auth_test.go +++ b/cmd/auth_test.go @@ -2,10 +2,13 @@ package cmd import ( "bytes" + "path/filepath" "testing" + "github.com/gleanwork/glean-cli/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/zalando/go-keyring" ) func TestAuthCmd_Help(t *testing.T) { @@ -30,29 +33,43 @@ func TestAuthCmd_HasSubcommands(t *testing.T) { assert.Contains(t, subNames, "status") } +func isolateAuthState(t *testing.T) { + t.Helper() + + home := t.TempDir() + t.Setenv("HOME", home) + + oldPath := config.ConfigPath + config.ConfigPath = filepath.Join(home, ".glean", "config.json") + t.Cleanup(func() { config.ConfigPath = oldPath }) + + oldService := config.ServiceName + config.ServiceName = "glean-cli-test-cmd-auth" + t.Cleanup(func() { config.ServiceName = oldService }) + + keyring.MockInit() +} + func TestAuthStatusCmd_NoConfig(t *testing.T) { - // With no config set, auth status should not panic. - // It prints to stdout directly (not cmd.OutOrStdout), so we just - // verify it doesn't return an error. + isolateAuthState(t) + root := NewCmdRoot() buf := &bytes.Buffer{} root.SetOut(buf) root.SetErr(buf) root.SetArgs([]string{"auth", "status"}) - // Should not crash — returns nil (prints "Not configured.") or a wrapped error. _ = root.Execute() } func TestAuthLogoutCmd_NoPanic(t *testing.T) { - // Verify logout doesn't panic regardless of system auth state. + isolateAuthState(t) + root := NewCmdRoot() buf := &bytes.Buffer{} root.SetOut(buf) root.SetErr(buf) root.SetArgs([]string{"auth", "logout"}) - // May succeed or fail depending on whether credentials exist on - // this machine — either outcome is fine, we just verify no panic. _ = root.Execute() } From ccf2263ea6a0ab7b1f82c51bcb90b20183dd50e2 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Mon, 6 Apr 2026 12:28:55 -0700 Subject: [PATCH 2/2] refactor: extract shared auth test helper into authtest package Hoists isolateAuthState into internal/auth/authtest.IsolateAuthState so both cmd/ and internal/auth/ tests use a single implementation. Adds CLAUDE.md guidance requiring all auth-touching tests to use it. Also fixes stale repo path references in CLAUDE.md after the repo moved from ~/workspace/personal/ to ~/workspace/glean/. Co-Authored-By: Claude Opus 4.6 (1M context) --- CLAUDE.md | 19 ++++++++++++++-- cmd/auth_test.go | 25 +++------------------ internal/auth/auth_persistence_test.go | 28 +++++------------------- internal/auth/authtest/authtest.go | 30 ++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 47 deletions(-) create mode 100644 internal/auth/authtest/authtest.go diff --git a/CLAUDE.md b/CLAUDE.md index 3091873..900f2c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -5,7 +5,7 @@ **Never say a feature is complete without running the actual binary.** ```bash -cd /Users/steve.calvert/workspace/personal/glean-cli +cd /Users/steve.calvert/workspace/glean/glean-cli mise run build # build the binary ./glean # run the TUI and test the feature manually ``` @@ -56,11 +56,26 @@ mise run test:all # lint + test + build (CI equivalent) ## TUI Architecture Rules -- **Worktree LSP errors are noise** — the worktree (`/.claude/worktrees/`) is stale. Always build in the main repo at `/Users/steve.calvert/workspace/personal/glean-cli` +- **Worktree LSP errors are noise** — the worktree (`/.claude/worktrees/`) is stale. Always build in the main repo at `/Users/steve.calvert/workspace/glean/glean-cli` - **Viewport key isolation** — never pass `tea.KeyMsg` to viewport in the catch-all; scroll keys are handled explicitly above - **Collect-then-display for content** — streaming stages show live via channel; content waits until complete - **No viewport jumping** — `conversationActive` pins viewport height; `recalculateLayout()` only called on terminal resize or deliberate state changes +## Auth Test Isolation + +**All tests that touch auth, config, or keyring state MUST call `authtest.IsolateAuthState(t)`.** + +```go +import "github.com/gleanwork/glean-cli/internal/auth/authtest" + +func TestSomethingWithAuth(t *testing.T) { + authtest.IsolateAuthState(t) + // ... test code +} +``` + +This redirects HOME, config path, and keyring to a temp directory so tests never read or delete real credentials. Without it, tests can silently wipe `~/.glean/config.json` and keyring entries. + ## Common Mistakes to Avoid 1. **Rendering picker only in active state** — UI elements visible before a conversation starts need to work in the welcome state too (`!m.conversationActive` branch in View()) diff --git a/cmd/auth_test.go b/cmd/auth_test.go index 8a6a91f..b2e9d6b 100644 --- a/cmd/auth_test.go +++ b/cmd/auth_test.go @@ -2,13 +2,11 @@ package cmd import ( "bytes" - "path/filepath" "testing" - "github.com/gleanwork/glean-cli/internal/config" + "github.com/gleanwork/glean-cli/internal/auth/authtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/zalando/go-keyring" ) func TestAuthCmd_Help(t *testing.T) { @@ -33,25 +31,8 @@ func TestAuthCmd_HasSubcommands(t *testing.T) { assert.Contains(t, subNames, "status") } -func isolateAuthState(t *testing.T) { - t.Helper() - - home := t.TempDir() - t.Setenv("HOME", home) - - oldPath := config.ConfigPath - config.ConfigPath = filepath.Join(home, ".glean", "config.json") - t.Cleanup(func() { config.ConfigPath = oldPath }) - - oldService := config.ServiceName - config.ServiceName = "glean-cli-test-cmd-auth" - t.Cleanup(func() { config.ServiceName = oldService }) - - keyring.MockInit() -} - func TestAuthStatusCmd_NoConfig(t *testing.T) { - isolateAuthState(t) + authtest.IsolateAuthState(t) root := NewCmdRoot() buf := &bytes.Buffer{} @@ -63,7 +44,7 @@ func TestAuthStatusCmd_NoConfig(t *testing.T) { } func TestAuthLogoutCmd_NoPanic(t *testing.T) { - isolateAuthState(t) + authtest.IsolateAuthState(t) root := NewCmdRoot() buf := &bytes.Buffer{} diff --git a/internal/auth/auth_persistence_test.go b/internal/auth/auth_persistence_test.go index 0b36597..21f2000 100644 --- a/internal/auth/auth_persistence_test.go +++ b/internal/auth/auth_persistence_test.go @@ -2,35 +2,17 @@ package auth_test import ( "context" - "path/filepath" "testing" "time" "github.com/gleanwork/glean-cli/internal/auth" + "github.com/gleanwork/glean-cli/internal/auth/authtest" gleanClient "github.com/gleanwork/glean-cli/internal/client" "github.com/gleanwork/glean-cli/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/zalando/go-keyring" ) -func isolateAuthState(t *testing.T) { - t.Helper() - - home := t.TempDir() - t.Setenv("HOME", home) - - oldConfigPath := config.ConfigPath - config.ConfigPath = filepath.Join(home, ".glean", "config.json") - t.Cleanup(func() { config.ConfigPath = oldConfigPath }) - - oldServiceName := config.ServiceName - config.ServiceName = "glean-cli-test-auth-persistence" - t.Cleanup(func() { config.ServiceName = oldServiceName }) - - keyring.MockInit() -} - func oauthToken() *auth.StoredTokens { return &auth.StoredTokens{ AccessToken: "oauth-access-token", @@ -43,7 +25,7 @@ func oauthToken() *auth.StoredTokens { } func TestOAuthLoginStateRequiresPersistedHostAfterEnvHostIsRemoved(t *testing.T) { - isolateAuthState(t) + authtest.IsolateAuthState(t) const host = "acme-be.glean.com" require.NoError(t, auth.SaveTokens(host, oauthToken())) @@ -68,7 +50,7 @@ func TestOAuthLoginStateRequiresPersistedHostAfterEnvHostIsRemoved(t *testing.T) } func TestOAuthTokenResolvesWhenHostIsPersisted(t *testing.T) { - isolateAuthState(t) + authtest.IsolateAuthState(t) const host = "acme-be.glean.com" require.NoError(t, config.SaveHostToFile(host)) @@ -84,7 +66,7 @@ func TestOAuthTokenResolvesWhenHostIsPersisted(t *testing.T) { } func TestShortFormHostNormalizesConsistently(t *testing.T) { - isolateAuthState(t) + authtest.IsolateAuthState(t) const shortHost = "acme" const normalizedHost = "acme-be.glean.com" @@ -107,7 +89,7 @@ func TestShortFormHostNormalizesConsistently(t *testing.T) { } func TestLogoutClearsPersistedHostAndOAuthTokens(t *testing.T) { - isolateAuthState(t) + authtest.IsolateAuthState(t) const host = "acme-be.glean.com" require.NoError(t, config.SaveHostToFile(host)) diff --git a/internal/auth/authtest/authtest.go b/internal/auth/authtest/authtest.go new file mode 100644 index 0000000..4371390 --- /dev/null +++ b/internal/auth/authtest/authtest.go @@ -0,0 +1,30 @@ +// Package authtest provides shared test helpers for auth-related tests. +package authtest + +import ( + "path/filepath" + "testing" + + "github.com/gleanwork/glean-cli/internal/config" + "github.com/zalando/go-keyring" +) + +// IsolateAuthState redirects config, HOME, and keyring to temporary +// locations so tests never touch real credentials. All mutations are +// reverted via t.Cleanup. +func IsolateAuthState(t *testing.T) { + t.Helper() + + home := t.TempDir() + t.Setenv("HOME", home) + + oldConfigPath := config.ConfigPath + config.ConfigPath = filepath.Join(home, ".glean", "config.json") + t.Cleanup(func() { config.ConfigPath = oldConfigPath }) + + oldServiceName := config.ServiceName + config.ServiceName = "glean-cli-test-isolated" + t.Cleanup(func() { config.ServiceName = oldServiceName }) + + keyring.MockInit() +}