test: calendar test-debt batch - tz, empty-200, contract (hw-b6lh)#253
Merged
Conversation
Scout hw-xthx blind spot #8 found two untested calendar-producer edge cases: a genuine HTTP 200 with items:[] overwrites the cached last-good result (TUI renders Free while busy, and a later failing poll can only serve the empty envelope — the good cache is unrecoverable), and the events request is capped at MaxResults(20) with NextPageToken never followed, silently truncating busy calendars to the first 20 events. This adds test-only coverage that PINS both behaviors as-is (pinned-pending-policy): the accept-vs-quarantine call for empty-200 responses and the paginate-vs-cap call for truncation are product decisions deferred to Vishnu. The tests document this in their names and comments so nobody "fixes" production code without that call, and they fail loudly if a deliberate policy change lands so they get updated alongside it. The new pinnedCalendarMock emulates the Google API's maxResults semantics (returns at most maxResults items, sets nextPageToken when more remain) so the truncation pin exercises the real request shape: it asserts maxResults=20 is sent, exactly one events call is made, and no request ever carries a pageToken. Zero production changes. Validation: GOMEMLIMIT=4GiB go test -race -p 2 ./internal/feeds/ green (both new tests plus the full existing package).
Scout hw-xthx blind spot #7: every calendar test used uniform time.UTC, so parseGoogleEventTime's offset handling and its malformed-date silent-drop path (producer_calendar.go zero-time fallthrough) had no coverage. Google delivers event times in the calendar's own timezone (e.g. +05:30), so mixed-offset inputs are the production-normal case. Adds test-only fixtures across the three layers the bead names: (a) parseGoogleEventTime unit table — +05:30/-08:00/Z offsets must parse to the correct instant with the source offset preserved; malformed dateTime/date values (garbage, non-RFC3339, out-of-range, Feb 30) must yield zero-time without claiming all-day. (b) Producer end-to-end with +05:30 events against UTC-now: an in-progress offset event is flagged is_current, the upcoming one wins next_event, and formatted times keep the source offset. Malformed events stay in the list with empty start/end, are never current, and are skipped for next_event (nil when nothing valid remains) — pinning the ARCH-1 silent-drop behavior explicitly. (c) calendarRelativeTime mixed-offset table: durations are instant-based; tomorrow/weekday labels show the event's own wall clock. Cases document current behavior, no production changes. Validation: GOMEMLIMIT=4GiB go test -race -p 2 on internal/feeds and cmd/hookwise — all green, gofmt/vet clean on both touched files.
Scout hw-xthx blind spot #9: the calendar feed crossed the Go->JSON->Python boundary with no shared fixture, the existing cross-boundary pair did not assert the ttl_seconds freshness field the TUI now gates on (post-#249), most calendar renderer formatting branches were untested, and doctor's stale-feed test exercised weather only. Adds, test-only: - testdata/contracts/feeds/calendar.json: a shared calendar feed contract fixture consumed by BOTH sides. It lives in a feeds/ subdirectory because the ARCH-6 dispatch loader (internal/contract) reads only top-level .json and would reject a fixture without an event_type. The Go side (TestCalendarFeedContractFixture_FlattenForTUI) pins the fixture envelope to feeds.CalendarTestFixture() and FlattenForTUI output to expected_flattened via JSONEq; the Python side renders the same expected_flattened entry, so a producer field rename cannot pass one side. - Freshness: the existing Go calendar cross-boundary test now asserts ttl_seconds (it only pinned updated_at); the fixture's frozen updated_at doubles as the stale-case assertion in Python (renders absent as-written, renders the event once fresh-stamped). - TestCalendarFormattingBranches (tui/tests): pins ends-in/(+N more) suffixes, ValueError/KeyError parse fallbacks, and the NOW / in-Xmin / Free-for-Xh buckets, with mid-bucket offsets so test jitter cannot cross a bucket edge. - TestDoctorFeedHealthStale now also covers calendar, with real events in the cache so it hits the staleness WARN rather than the empty-data or zero-liveness paths that run first. Validation: go test -race -p 2 on internal/bridge, internal/contract, cmd/hookwise all pass; tui pytest test_status_segments.py 29/29 pass. Zero production changes.
📝 WalkthroughWalkthroughThe PR adds regression coverage for calendar event parsing, API truncation and cache fallback, bridge flattening, TUI rendering, mixed-offset relative times, and stale feed-health warnings. ChangesCalendar validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hookwise/cmd_status_line_calendar_test.go`:
- Line 147: Correct the inline comment for the event in the test case to state
2026-06-17 03:30 UTC, preserving the existing “41.5h out” value.
In `@internal/feeds/producer_calendar_pinned_test.go`:
- Around line 158-191: Add defer srv.Close() immediately after creating the
httptest server in
TestCalendarProducer_Pinned_Genuine200EmptyItems_OverwritesGoodCache, while
retaining the existing manual close used to simulate the network failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e62aec62-2eb9-4aac-b243-ee92e983be5c
📒 Files selected for processing (7)
cmd/hookwise/cli_test.gocmd/hookwise/cmd_status_line_calendar_test.gointernal/bridge/bridge_test.gointernal/feeds/producer_calendar_pinned_test.gointernal/feeds/producer_calendar_test.gotestdata/contracts/feeds/calendar.jsontui/tests/test_status_segments.py
| { | ||
| name: "tomorrow label shows the event's own wall clock", | ||
| now: time.Date(2026, 6, 15, 10, 0, 0, 0, time.UTC), | ||
| event: time.Date(2026, 6, 17, 9, 0, 0, 0, ist), // 2026-06-16 03:30 UTC, 41.5h out |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix incorrect UTC date in comment.
The comment says 2026-06-16 03:30 UTC but time.Date(2026, 6, 17, 9, 0, 0, 0, ist) converts to 2026-06-17 03:30 UTC (09:00 − 05:30 = 03:30, same day). The comment is internally inconsistent: "41.5h out" is correct for June 17 but would only be 17.5h for June 16.
Fix the UTC date in the inline comment
event: time.Date(2026, 6, 17, 9, 0, 0, 0, ist), // 2026-06-16 03:30 UTC, 41.5h outshould be:
event: time.Date(2026, 6, 17, 9, 0, 0, 0, ist), // 2026-06-17 03:30 UTC, 41.5h out📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| event: time.Date(2026, 6, 17, 9, 0, 0, 0, ist), // 2026-06-16 03:30 UTC, 41.5h out | |
| event: time.Date(2026, 6, 17, 9, 0, 0, 0, ist), // 2026-06-17 03:30 UTC, 41.5h out |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/hookwise/cmd_status_line_calendar_test.go` at line 147, Correct the
inline comment for the event in the test case to state 2026-06-17 03:30 UTC,
preserving the existing “41.5h out” value.
Comment on lines
+158
to
+191
| func TestCalendarProducer_Pinned_Genuine200EmptyItems_OverwritesGoodCache(t *testing.T) { | ||
| mock := &pinnedCalendarMock{totalEvents: 1} | ||
| srv := httptest.NewServer(mock) | ||
|
|
||
| p := newPinnedCalendarProducer(t, srv.URL) | ||
|
|
||
| // Poll 1: success with one real event — this is the "good cache". | ||
| result1, err := p.Produce(context.Background()) | ||
| require.NoError(t, err) | ||
| require.Len(t, calendarEvents(t, result1), 1, "poll 1 must cache one real event") | ||
|
|
||
| // Poll 2: genuine 200 with items:[]. Current behavior: accepted as | ||
| // success, cache overwritten, renders as "Free". | ||
| mock.setEmptyItems(true) | ||
| result2, err := p.Produce(context.Background()) | ||
| require.NoError(t, err, "ARCH-1: empty-200 poll must not error") | ||
| env2 := result2.(map[string]interface{}) | ||
| assert.Empty(t, calendarEvents(t, result2), | ||
| "PINNED: a genuine 200 with items:[] replaces the good result with an empty one") | ||
| assert.Nil(t, env2["data"].(map[string]interface{})["next_event"], | ||
| "PINNED: empty-200 poll clears next_event") | ||
|
|
||
| // Poll 3: the API dies (transient network failure). The fail-open | ||
| // fallback serves the cached envelope — which is now the EMPTY one from | ||
| // poll 2, not the good one from poll 1. This is the destructive part of | ||
| // the overwrite: fallback can only serve what the empty-200 left behind. | ||
| srv.Close() | ||
| result3, err := p.Produce(context.Background()) | ||
| require.NoError(t, err, "ARCH-1: failing poll must not error") | ||
| assert.Empty(t, calendarEvents(t, result3), | ||
| "PINNED: after an empty-200, the fallback cache holds zero events — poll 1's good data is unrecoverable") | ||
| assert.Equal(t, env2["timestamp"], result3.(map[string]interface{})["timestamp"], | ||
| "fallback must serve poll 2's envelope verbatim (frozen timestamp)") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add defer srv.Close() to prevent goroutine leak on test failure.
The first pinned test creates srv := httptest.NewServer(mock) at line 160 without a deferred close, then manually closes it at line 184 to simulate a network failure. If any require assertion between lines 165–183 fails, the server goroutine leaks. The second test in this file correctly uses defer srv.Close() (line 207). httptest.Server.Close() is safe to call multiple times, so adding a defer here won't interfere with the manual close.
🔧 Proposed fix
mock := &pinnedCalendarMock{totalEvents: 1}
srv := httptest.NewServer(mock)
+ defer srv.Close()
p := newPinnedCalendarProducer(t, srv.URL)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestCalendarProducer_Pinned_Genuine200EmptyItems_OverwritesGoodCache(t *testing.T) { | |
| mock := &pinnedCalendarMock{totalEvents: 1} | |
| srv := httptest.NewServer(mock) | |
| p := newPinnedCalendarProducer(t, srv.URL) | |
| // Poll 1: success with one real event — this is the "good cache". | |
| result1, err := p.Produce(context.Background()) | |
| require.NoError(t, err) | |
| require.Len(t, calendarEvents(t, result1), 1, "poll 1 must cache one real event") | |
| // Poll 2: genuine 200 with items:[]. Current behavior: accepted as | |
| // success, cache overwritten, renders as "Free". | |
| mock.setEmptyItems(true) | |
| result2, err := p.Produce(context.Background()) | |
| require.NoError(t, err, "ARCH-1: empty-200 poll must not error") | |
| env2 := result2.(map[string]interface{}) | |
| assert.Empty(t, calendarEvents(t, result2), | |
| "PINNED: a genuine 200 with items:[] replaces the good result with an empty one") | |
| assert.Nil(t, env2["data"].(map[string]interface{})["next_event"], | |
| "PINNED: empty-200 poll clears next_event") | |
| // Poll 3: the API dies (transient network failure). The fail-open | |
| // fallback serves the cached envelope — which is now the EMPTY one from | |
| // poll 2, not the good one from poll 1. This is the destructive part of | |
| // the overwrite: fallback can only serve what the empty-200 left behind. | |
| srv.Close() | |
| result3, err := p.Produce(context.Background()) | |
| require.NoError(t, err, "ARCH-1: failing poll must not error") | |
| assert.Empty(t, calendarEvents(t, result3), | |
| "PINNED: after an empty-200, the fallback cache holds zero events — poll 1's good data is unrecoverable") | |
| assert.Equal(t, env2["timestamp"], result3.(map[string]interface{})["timestamp"], | |
| "fallback must serve poll 2's envelope verbatim (frozen timestamp)") | |
| } | |
| func TestCalendarProducer_Pinned_Genuine200EmptyItems_OverwritesGoodCache(t *testing.T) { | |
| mock := &pinnedCalendarMock{totalEvents: 1} | |
| srv := httptest.NewServer(mock) | |
| defer srv.Close() | |
| p := newPinnedCalendarProducer(t, srv.URL) | |
| // Poll 1: success with one real event — this is the "good cache". | |
| result1, err := p.Produce(context.Background()) | |
| require.NoError(t, err) | |
| require.Len(t, calendarEvents(t, result1), 1, "poll 1 must cache one real event") | |
| // Poll 2: genuine 200 with items:[]. Current behavior: accepted as | |
| // success, cache overwritten, renders as "Free". | |
| mock.setEmptyItems(true) | |
| result2, err := p.Produce(context.Background()) | |
| require.NoError(t, err, "ARCH-1: empty-200 poll must not error") | |
| env2 := result2.(map[string]interface{}) | |
| assert.Empty(t, calendarEvents(t, result2), | |
| "PINNED: a genuine 200 with items:[] replaces the good result with an empty one") | |
| assert.Nil(t, env2["data"].(map[string]interface{})["next_event"], | |
| "PINNED: empty-200 poll clears next_event") | |
| // Poll 3: the API dies (transient network failure). The fail-open | |
| // fallback serves the cached envelope — which is now the EMPTY one from | |
| // poll 2, not the good one from poll 1. This is the destructive part of | |
| // the overwrite: fallback can only serve what the empty-200 left behind. | |
| srv.Close() | |
| result3, err := p.Produce(context.Background()) | |
| require.NoError(t, err, "ARCH-1: failing poll must not error") | |
| assert.Empty(t, calendarEvents(t, result3), | |
| "PINNED: after an empty-200, the fallback cache holds zero events — poll 1's good data is unrecoverable") | |
| assert.Equal(t, env2["timestamp"], result3.(map[string]interface{})["timestamp"], | |
| "fallback must serve poll 2's envelope verbatim (frozen timestamp)") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/feeds/producer_calendar_pinned_test.go` around lines 158 - 191, Add
defer srv.Close() immediately after creating the httptest server in
TestCalendarProducer_Pinned_Genuine200EmptyItems_OverwritesGoodCache, while
retaining the existing manual close used to simulate the network failure.
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
1 participant
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.
Convoy cal-test-debt (hw-b6lh = hw-rnqs + hw-8d9k + hw-q3vr): the last three blind spots from calendar-staleness scout hw-xthx, batched per the GCI-004 convoy trial. (1) Pins empty-200 overwrite + 20-event truncation behavior (policy decision deferred to Vishnu, tests document current semantics). (2) Non-UTC offset fixtures across parse/filter/render + malformed-date silent-drop pin. (3) First calendar contract fixture (ARCH-6), cross-boundary fixtures gain freshness fields with a stale-case assertion, renderer formatting branches + doctor stale-feed calendar case. Test-only, zero production changes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EjWqH4FMkdczkSagzXw3dQ
Summary by CodeRabbit
Bug Fixes
Tests