Repository navigation
fix: sessions sorted by time are deterministic, in the code and in its test - #143
Merged
Merged
Conversation
session-manager.test.ts expected ['Session 1', 'Session 3'] from a sort by updatedAt, most recent first, as its own comment says. That held only when all three sessions were created within one millisecond: the sort then kept insertion order. On a runner where the clock ticked between them, Session 3 came first and the test failed, as on main's CI after #142 (a changelog-only change). The test now creates each session at its own fixed time (fake Date only) and expects the most recent first. With distinct times the old expectation fails every time; the new one passes every time. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv
Review of #143: the sort by updatedAt had no tie-breaker, so two sessions created (or restored) in the same millisecond came back oldest first, and the chat's delete fallback took the older one as the most recent sibling. Equal times now fall back to createdAt, then to the later one added. A test pins equal times (it fails without the tie-breaker); the first test's comment says what it needs rather than its history. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv
endrix
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Main's CI went red after #142, which is changelog-only, in
extension-core/test/session-manager.test.ts>getSessionsForWorkflow: expected['Session 1', 'Session 3'], got['Session 3', 'Session 1'].Cause
A flaky test, older than this cleanup work. The code sorts sessions by
updatedAt, most recent first, and the test's comment says so too, but the test expected insertion order. It passed only when all three sessions were created within one millisecond, because the sort then kept insertion order. On a runner where the clock ticked, the code did what its comment says and the test failed.Fix
Dateonly, so no timers change.Review (
/code-review medium), addressedThe review confirmed the diagnosis. It also confirmed that nothing in
createSessionwaits on a timer, that real timers come back even on failure, and that no other test depends on two calls landing in one millisecond. It found that the code itself had no tie-breaker, so two sessions created or restored in the same millisecond came back oldest first, and the chat's delete fallback took the older one as "the most recent sibling".createdAt, then to the later one added.Local results
session-manager.test.ts: 3/3 runs greenhttps://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv