Conversation
…guard (#861) Add two tests to TestSortedSessionsCacheSkipsRedundantSort: - test_sort_runs_after_plan_change: verifies that adding plan.md to a session (while events.jsonl is unchanged) forces a re-sort via the `not deferred_sessions` guard, and the returned summary carries the fresh session name. - test_sort_skipped_after_plan_change_then_unchanged: verifies that after the plan.md change settles, a subsequent call with no further changes hits the warm cache and skips the sort. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds regression coverage for the get_all_sessions sorted-sessions cache guard (not deferred_sessions) to ensure plan.md changes correctly trigger a resort and refresh cached session names.
Changes:
- Adds
test_sort_runs_after_plan_changeto verify aplan.mdadd/change forces a fresh sort and updates the returned session name. - Adds
test_sort_skipped_after_plan_change_then_unchangedto verify the sorted cache is repopulated and subsequent unchanged calls skip sorting again.
Contributor
There was a problem hiding this comment.
Quality Gate: APPROVE — Low-impact, test-only change with good quality.
What was evaluated:
- Single file changed:
tests/copilot_usage/test_parser.py(+136 lines, 0 deletions) - Two new regression tests in
TestSortedSessionsCacheSkipsRedundantSortcovering thenot deferred_sessionsguard inget_all_sessions - All 8 CI checks pass (lint, typecheck, security, tests, CodeQL, etc.)
Code quality assessment:
- Tests are meaningful — they verify cache invalidation when
plan.mdchanges and cache hits on subsequent unchanged calls - Follows existing testing patterns (spy-counting
session_sort_keycalls instead of wall-clock timing, consistent with the class's other tests) - Proper type annotations, clear docstrings, uses existing test utilities (
_write_events,patch.object) - Compliant with CODING_GUIDELINES.md (strict typing, pytest conventions, no banned patterns)
Impact: LOW — Test-only addition, no production code changes. Auto-approving for merge.
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.
Closes #861
Adds two regression tests to
TestSortedSessionsCacheSkipsRedundantSortcovering the previously untestednot deferred_sessionsguard inget_all_sessions:test_sort_runs_after_plan_change— creates two cached sessions, addsplan.mdto one (without touchingevents.jsonl), and asserts that:session_sort_keyis called (cache bypassed due to non-emptydeferred_sessions)plan.mdtest_sort_skipped_after_plan_change_then_unchanged— after the plan-change call, calls again with no further file changes and asserts thatsession_sort_keyis not called (sorted cache is now warm and up-to-date)All existing tests continue to pass.
make checkpasses cleanly (lint, typecheck, security, tests at 99% coverage).