Skip to content

Fix test isolation issues with keyring storage #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cevian merged 2 commits into from
Oct 8, 2025
Merged

Fix test isolation issues with keyring storage #47

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6af533c
Fix test isolation issues with keyring storage
cevian Oct 7, 2025
1fecdc5
Remove -p 1 flag from test workflow
cevian Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions internal/tiger/cmd/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import (
func setupAuthTest(t *testing.T) string {
t.Helper()

// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)

// Mock the API key validation for testing
originalValidator := validateAPIKeyForLogin
validateAPIKeyForLogin = func(apiKey, projectID string) error {
Expand All @@ -47,10 +50,6 @@ func setupAuthTest(t *testing.T) string {
t.Fatalf("Failed to use test config: %v", err)
}

// Also ensure config file doesn't exist
configFile := config.GetConfigFile(tmpDir)
os.Remove(configFile)

t.Cleanup(func() {
// Clean up test keyring
config.RemoveAPIKeyFromKeyring()
Expand Down
6 changes: 6 additions & 0 deletions internal/tiger/cmd/auth_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ func TestAuthLogin_APIKeyValidationFailure(t *testing.T) {
}
defer os.RemoveAll(tmpDir)

// Use a unique service name for this test
config.SetTestServiceName(t)

originalValidator := validateAPIKeyForLogin

// Mock the validator to return an error
Expand Down Expand Up @@ -73,6 +76,9 @@ func TestAuthLogin_APIKeyValidationSuccess(t *testing.T) {
}
defer os.RemoveAll(tmpDir)

// Use a unique service name for this test
config.SetTestServiceName(t)

originalValidator := validateAPIKeyForLogin

// Mock the validator to return success
Expand Down
6 changes: 6 additions & 0 deletions internal/tiger/cmd/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,9 @@ func TestLaunchPsqlWithAdditionalFlags(t *testing.T) {
}

func TestBuildPsqlCommand_KeyringPasswordEnvVar(t *testing.T) {
// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)

// Set keyring as the password storage method for this test
originalStorage := viper.GetString("password_storage")
viper.Set("password_storage", "keyring")
Expand Down Expand Up @@ -950,6 +953,9 @@ func TestDBConnectionString_WithPassword(t *testing.T) {
// This test verifies the end-to-end --with-password flag functionality
// using direct function testing since full integration would require a real service

// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)

// Set keyring as the password storage method for this test
originalStorage := viper.GetString("password_storage")
viper.Set("password_storage", "keyring")
Expand Down
30 changes: 24 additions & 6 deletions internal/tiger/config/api_key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,40 @@ import (

// Keyring parameters
const (
keyringServiceName = "tiger-cli"
keyringTestServiceName = "tiger-cli-test"
keyringUsername = "api-key"
keyringServiceName = "tiger-cli"
keyringUsername = "api-key"
)

// testServiceNameOverride allows tests to override the service name for isolation
var testServiceNameOverride string

// GetServiceName returns the appropriate service name for keyring operations
// Uses a test-specific service name when running in test mode to avoid polluting the real keyring
func GetServiceName() string {
// Use Go's built-in testing detection
// Tests should set a unique service name to avoid conflicts
if testServiceNameOverride != "" {
return testServiceNameOverride
}

// In test mode without an override, panic to catch missing test setup
if testing.Testing() {
return keyringTestServiceName
panic("test must call SetTestServiceName() to set a unique keyring service name")
}

return keyringServiceName
}

// SetTestServiceName sets a unique service name for testing based on the test name
// This allows tests to use unique service names to avoid conflicts when running in parallel
// The cleanup is automatically registered with t.Cleanup()
func SetTestServiceName(t *testing.T) {
testServiceNameOverride = "tiger-test-" + t.Name()

// Automatically clean up when the test finishes
t.Cleanup(func() {
testServiceNameOverride = ""
})
}

Comment on lines +37 to +48
Copy link
Member

@nathanjcochran nathanjcochran Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One potential worry I have with this approach is that there could be race conditions where multiple parallel tests call SetTestServiceName concurrently, thereby overwriting the value of testServiceNameOverride set by the other tests.

I guess that's not an issue right now, because we don't have any parallel tests within the same package, and tests in different packages are fully isolated from each other (i.e. they don't share memory or this testServiceNameOverride variable). Right?

Still, it seems like a potential gotcha for down the road. I feel like it would be better to somehow pass the keyring service name into the calls that access the keyring (even though that would be annoying in many ways).

In any case, I think this is fine for now. Definitely an improvement over what we had 👍

Copy link
Contributor Author

@cevian cevian Oct 8, 2025

Choose a reason for hiding this comment

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

Yeah this needs a better refactor but merging for now

// storeAPIKey stores the API key using keyring with file fallback
func StoreAPIKey(apiKey string) error {
// Try keyring first
Expand Down
20 changes: 11 additions & 9 deletions internal/tiger/config/api_key_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ import (
"os"
"path/filepath"
"testing"

"github.com/spf13/viper"
)

func setupAPIKeyTest(t *testing.T) string {
t.Helper()

// Use a unique service name for this test to avoid conflicts
SetTestServiceName(t)

// Clean up any existing keyring entries before test
RemoveAPIKeyFromKeyring()

Expand All @@ -20,8 +21,11 @@ func setupAPIKeyTest(t *testing.T) string {
t.Fatalf("Failed to create temp dir: %v", err)
}

// Wire up the tmp directory as the viper config directory
viper.SetConfigFile(GetConfigFile(tmpDir))
// Reset viper completely and set up with test directory
// This ensures proper test isolation by resetting all viper state
if _, err := UseTestConfig(tmpDir, map[string]any{}); err != nil {
t.Fatalf("Failed to use test config: %v", err)
}

t.Cleanup(func() {
// Clean up keyring entries
Expand Down Expand Up @@ -69,15 +73,15 @@ func TestStoreAPIKeyToFile(t *testing.T) {

func TestGetAPIKeyFromFile(t *testing.T) {
tmpDir := setupAPIKeyTest(t)
viper.SetConfigFile(GetConfigFile(tmpDir))

// Write API key to file
apiKeyFile := filepath.Join(tmpDir, "api-key")
if err := os.WriteFile(apiKeyFile, []byte("file-get-test-key"), 0600); err != nil {
t.Fatalf("Failed to write test API key file: %v", err)
}

// Get API key from file
// Get API key - should get from file since keyring is empty
// (each test uses a unique keyring service name)
apiKey, err := GetAPIKey()
if err != nil {
t.Fatalf("Failed to get API key from file: %v", err)
Expand All @@ -89,8 +93,7 @@ func TestGetAPIKeyFromFile(t *testing.T) {
}

func TestGetAPIKeyFromFile_NotExists(t *testing.T) {
tmpDir := setupAPIKeyTest(t)
viper.SetConfigFile(GetConfigFile(tmpDir))
setupAPIKeyTest(t)

// Try to get API key when file doesn't exist
_, err := GetAPIKey()
Expand All @@ -105,7 +108,6 @@ func TestGetAPIKeyFromFile_NotExists(t *testing.T) {

func TestRemoveAPIKeyFromFile(t *testing.T) {
tmpDir := setupAPIKeyTest(t)
viper.SetConfigFile(GetConfigFile(tmpDir))

// Write API key to file
apiKeyFile := filepath.Join(tmpDir, "api-key")
Expand Down
10 changes: 10 additions & 0 deletions internal/tiger/password/connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/spf13/viper"

"github.com/timescale/tiger-cli/internal/tiger/api"
"github.com/timescale/tiger-cli/internal/tiger/config"
"github.com/timescale/tiger-cli/internal/tiger/util"
)

Expand Down Expand Up @@ -186,6 +187,9 @@ func TestBuildConnectionString_Basic(t *testing.T) {
}

func TestBuildConnectionString_WithPassword_KeyringStorage(t *testing.T) {
// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)

// Set keyring as the password storage method for this test
originalStorage := viper.GetString("password_storage")
viper.Set("password_storage", "keyring")
Expand Down Expand Up @@ -328,6 +332,9 @@ func TestBuildConnectionString_WithPassword_NoStorage(t *testing.T) {
}

func TestBuildConnectionString_WithPassword_NoPasswordAvailable(t *testing.T) {
// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)

// Set keyring as the password storage method for this test
originalStorage := viper.GetString("password_storage")
viper.Set("password_storage", "keyring")
Expand Down Expand Up @@ -366,6 +373,9 @@ func TestBuildConnectionString_WithPassword_NoPasswordAvailable(t *testing.T) {
}

func TestBuildConnectionString_WithPassword_InvalidServiceEndpoint(t *testing.T) {
// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)

// Set keyring as the password storage method for this test
originalStorage := viper.GetString("password_storage")
viper.Set("password_storage", "keyring")
Expand Down
5 changes: 5 additions & 0 deletions internal/tiger/password/password_storage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/spf13/viper"
"github.com/timescale/tiger-cli/internal/tiger/api"
"github.com/timescale/tiger-cli/internal/tiger/config"
"github.com/timescale/tiger-cli/internal/tiger/util"
)

Expand Down Expand Up @@ -573,6 +574,10 @@ func TestPasswordStorage_OverwritePreviousValue(t *testing.T) {
{
name: "KeyringStorage",
storage: &KeyringStorage{},
setup: func(t *testing.T) {
// Use a unique service name for this test to avoid conflicts
config.SetTestServiceName(t)
},
cleanup: func(t *testing.T, service api.Service) {
storage := &KeyringStorage{}
storage.Remove(service) // Ignore errors in cleanup
Expand Down