Fix test isolation issues with keyring storage#47
Conversation
This fixes test failures when running `go test ./... -count=1` caused by tests from different packages racing to use the same keyring service name. Solution implemented: - Added SetTestServiceName() function that creates unique keyring service names based on t.Name() - Each test gets its own isolated keyring namespace (e.g., "tiger-test-TestAuthLogin") - Automatic cleanup via t.Cleanup() - no manual cleanup needed - Prevents race conditions when tests run in parallel across packages Key changes: - api_key.go: Added SetTestServiceName() with automatic cleanup - All test files: Call SetTestServiceName(t) in setup functions - Removed manual ResetTestServiceName() calls - cleanup is automatic This ensures complete test isolation without requiring `-p 1` flag. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
nathanjcochran
left a comment
nathanjcochran
left a comment
There was a problem hiding this comment.
LGTM ✅
Can you also remove the -p 1 flag from the go test invocation in the GitHub Actions workflow? That's how I was working around this issue before in CI 😅 (i.e. by just running all of the tests sequentially, rather than the default behavior of running tests in different packages in parallel).
| // 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 = "" | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Yeah this needs a better refactor but merging for now
The test isolation fix makes this workaround unnecessary. Tests can now run in parallel without race conditions.
2 participants
Summary
go test ./... -count=1due to keyring race conditionst.Name()Problem
Tests were failing when run without cache (
-count=1) because:Solution
Implemented
SetTestServiceName(t *testing.T)function that:t.Cleanup()- no manual cleanup neededChanges
SetTestServiceName()with automatic cleanupSetTestServiceName(t)in setup functionsResetTestServiceName()calls - cleanup is now automaticTest Plan
go test ./... -count=1- all tests passgo test ./... -count=3- tests pass consistently🤖 Generated with Claude Code