[test-improver] Improve tests for server session package#2989
Conversation
Add tests for four previously-untested methods in session.go: - getSessionID: verifies it delegates correctly to SessionIDFromContext - ensureSessionDirectory: verifies it creates per-session dirs under payloadDir - requireSession: verifies auto-creation, deduplication, and concurrency safety - getSessionKeys: verifies correct enumeration of active session IDs Introduces a newMinimalUnifiedServer helper to reduce boilerplate in tests that need a real UnifiedServer instance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR expands unit test coverage for the internal/server session lifecycle by adding focused tests for previously untested UnifiedServer session helpers.
Changes:
- Added
newMinimalUnifiedServerhelper to reduce setup boilerplate across session tests. - Added table-driven and subtest-based coverage for
getSessionID,ensureSessionDirectory,requireSession(including concurrency), andgetSessionKeys. - Introduced filesystem-isolated tests using
t.TempDir()to validate per-session directory creation behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const goroutines = 20 | ||
| wg.Add(goroutines) | ||
| for range goroutines { | ||
| go func() { | ||
| defer wg.Done() | ||
| assert.NoError(t, us.requireSession(ctx)) | ||
| }() | ||
| } |
There was a problem hiding this comment.
for range goroutines does not compile because range can’t be used on an int. Use a conventional index loop (for i := 0; i < goroutines; i++ { ... }) or range over a slice/channel of length goroutines.
| } | ||
| us, err := NewUnified(context.Background(), cfg) | ||
| require.NoError(t, err, "NewUnified should not fail with an empty config") | ||
| t.Cleanup(func() { us.Close() }) |
There was a problem hiding this comment.
t.Cleanup(func() { us.Close() }) drops the error returned by Close(), which can hide cleanup failures. Consider asserting/require-ing that Close() returns nil inside the cleanup function (e.g., assert.NoError(t, us.Close())).
| t.Cleanup(func() { us.Close() }) | |
| t.Cleanup(func() { | |
| require.NoError(t, us.Close()) | |
| }) |
File Analyzed
internal/server/session_test.gointernal/serverImprovements Made
1. Increased Coverage
Four methods in
session.gohad zero test coverage. This PR adds tests for all of them:getSessionIDensureSessionDirectoryrequireSessiongetSessionKeys2. Better Testing Patterns
newMinimalUnifiedServerhelper witht.Helper()andt.Cleanup()to reduce boilerplate across testsrequire.NoErrorfor critical setup steps,assertfor non-fatal checkst.Run()with descriptive namest.TempDir()for filesystem isolation — no leftover state between runs3. Cleaner & More Stable Tests
UnifiedServer(or resetspayloadDir) so tests are fully independentrequireSessionverifies the double-checked locking ensures sessions are created exactly once under loadassert.Sameused to verify pointer identity (not just value equality) for session reuseKey New Tests
TestGetSessionID— confirmsgetSessionIDis a transparent wrapper forSessionIDFromContextacross absent, empty, non-string, and valid inputs.TestEnsureSessionDirectory— confirms the function createspayloadDir/sessionID/, is idempotent, and handles multiple sessions independently.TestRequireSession— confirms sessions are auto-created on first call, reused on subsequent calls, and that 20 concurrent goroutines all see the same single session instance.TestGetSessionKeys— confirms an empty server returns no keys, and that keys match exactly the set of sessions created viarequireSession.Why This File?
session.goimplements the session lifecycle for the gateway (creation, directory management, lookup). The existingsession_test.goonly tested the two pure functions (SessionIDFromContext,NewSession) and left the fourUnifiedServermethods completely uncovered. These methods contain non-trivial logic: directory creation,syncutil.GetOrCreatedouble-checked locking, and map iteration.Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests