Skip to content

test(httpapi): de-flake desc-events assertion in TestEventsAndSyncStatus#199

Merged
khaliqgant merged 1 commit into
mainfrom
fix/flaky-events-desc-assertion
May 22, 2026
Merged

test(httpapi): de-flake desc-events assertion in TestEventsAndSyncStatus#199
khaliqgant merged 1 commit into
mainfrom
fix/flaky-events-desc-assertion

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

Summary

The `desc&limit=1` assertion in `TestEventsAndSyncStatus` (added in #196) races with background event emission between the asc-feed and desc-feed calls. Observed failing in CI on #198's run:

```
desc&limit=1 must return newest event ("evt_3"), got "evt_4"
```

Replace strict equality with a race-tolerant comparison: extract the numeric ordinal from `evt_N` and require `desc_id >= asc_tail_id`. The original bug-class this assertion guards against — the self-host server silently ignoring `direction=desc` and returning the OLDEST event — still fails loudly (`desc_id` would be `1`, well below the asc tail).

Test plan

  • `go test ./internal/httpapi/... -run TestEventsAndSyncStatus -count=20` — 20 consecutive runs, all green.
  • Bug-class still caught: a hypothetical regression where the server ignored `direction=desc` would return `evt_1` (desc_id=1) and fail `1 < asc_tail_id`.

🤖 Generated with Claude Code

The desc&limit=1 assertion in TestEventsAndSyncStatus (introduced in
#196) computed `newest = ascFeed.Events[len(...)-1]` and asserted the
desc call returned the same id. That races with any background event
emission between the asc and desc calls — observed in CI failing with:

  desc&limit=1 must return newest event ("evt_3"), got "evt_4"

Switch to a race-tolerant assertion: extract the numeric ordinal from
the "evt_N" id and require desc_id >= asc_tail_id. The bug the
assertion was designed to catch — server silently ignoring direction
and returning the OLDEST event — still fails loudly (desc_id would be
1, well below the asc tail).

Verified by running TestEventsAndSyncStatus 20× consecutively, all
green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@github-actions
Copy link
Copy Markdown

Relayfile Eval Review

Run: .relayfile/evals/runs/2026-05-22T07-14-21-560Z-HEAD-provider
Mode: provider
Git SHA: e4c6566

Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0

Human Review Cases

No reviewable human-review cases captured Relayfile output.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 70885cdd-a529-4183-8e42-be4027ea2902

📥 Commits

Reviewing files that changed from the base of the PR and between cc418dd and b55c29b.

📒 Files selected for processing (1)
  • internal/httpapi/server_test.go

📝 Walkthrough

Walkthrough

The change adds race tolerance to pagination tests by extracting numeric ordinals from event IDs. A new helper function parses event ID suffixes, and the descending pagination assertion now validates that returned events have ordinal values >= the ascending tail's ordinal, accommodating concurrent event emission between the two test requests.

Changes

Pagination Test Race Tolerance

Layer / File(s) Summary
Race-tolerant pagination test with event ordinal helper
internal/httpapi/server_test.go
Adds strconv import, introduces eventIDOrdinal(t, id) helper to extract numeric suffixes from event IDs, and updates TestEventsAndSyncStatus descending pagination assertion to compare ordinals with >= instead of requiring exact ID equality.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🐰 The pagination test now hops gracefully past the race,
With ordinals compared in a safer place,
No more strict ID fetches causing dismay,
Just >= checks to save the test's day! 🏃‍♂️✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: fixing a flaky test assertion for descending event pagination in TestEventsAndSyncStatus.
Description check ✅ Passed The description is directly related to the changeset, explaining the race condition, the solution, why it works, and providing test evidence.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/flaky-events-desc-assertion

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@khaliqgant khaliqgant merged commit 0c0c60e into main May 22, 2026
10 checks passed
@khaliqgant khaliqgant deleted the fix/flaky-events-desc-assertion branch May 22, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant