Skip to content

test(task): ci-safe stream timeout - #186

Open
moshloop wants to merge 1 commit into
mainfrom
test/ci-safe-stream-timeout-dl53ckue8ev4
Open

test(task): ci-safe stream timeout#186
moshloop wants to merge 1 commit into
mainfrom
test/ci-safe-stream-timeout-dl53ckue8ev4

Conversation

@moshloop

@moshloop moshloop commented Sep 2, 2026

Copy link
Copy Markdown
Member

What

Fixed flaky timing in the finished-snapshot stream spec by replacing an implicit one-second deadline with an explicit ten-second timeout.

Why

The test relied on four 200ms ticks completing within the default one-second Eventually timeout. Loaded CI runners cannot reliably meet this, causing intermittent failures. An explicit timeout removes the timing dependency.

Notes

  • Test still validates live re-polling with three fetches
  • File: task/managed_run_ginkgo_test.go

Summary by CodeRabbit

  • Tests
    • Improved reliability of external run snapshot testing by allowing additional time for polling on slower environments.
    • Added an explicit timeout to reduce failures caused by default assertion timing limits.

The spec waited for four 200ms stream ticks under the one-second
Eventually default, which a loaded CI runner cannot meet. Three fetches
still prove the live run is re-polled; the explicit ten-second timeout
removes the timing dependency.

Claude-Session: https://claude.ai/code/session_01VvUAapAqqyyucVk72hevpB

Claude-Session-Id: 186f18fc-6862-45c9-8970-a4da801ba1ab
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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: Team

Run ID: fca9be94-0f31-406f-9eae-0207bf8c7c59

📥 Commits

Reviewing files that changed from the base of the PR and between e6d1876 and df9e957.

📒 Files selected for processing (1)
  • task/managed_run_ginkgo_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The external run snapshot test now requires at least three polls and uses a 10-second timeout for the Eventually assertion.

Changes

Managed run test

Layer / File(s) Summary
Adjust snapshot polling assertion
task/managed_run_ginkgo_test.go
The test changes the minimum snapshot call count from four to three and adds a 10-second timeout with polling rationale.

Suggested reviewers: flanksource

Merge Risk: ⚪ Minimal · up to df9e9

This change only makes a polling test tolerate slower CI scheduling while preserving its live re-polling coverage; no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains what changed, why the change was needed, and which file was updated. It does not follow the repository template and omits the required Type of Change, Testing, Checklist, Brea… Rewrite the description using the repository template. Include the change type, testing performed and results, checklist status, breaking-change information, and any additional notes. Preserve the existing explanation of the explicit ten-se…
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies a test change that improves CI timeout reliability for the stream test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
Full details: Description check

Explanation

The description explains what changed, why the change was needed, and which file was updated. It does not follow the repository template and omits the required Type of Change, Testing, Checklist, Breaking Changes, and Additional Notes sections.

Resolution

Rewrite the description using the repository template. Include the change type, testing performed and results, checklist status, breaking-change information, and any additional notes. Preserve the existing explanation of the explicit ten-second timeout and three-fetch validation.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

  • Fix all pre-merge checks with AI
✨ 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 test/ci-safe-stream-timeout-dl53ckue8ev4
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch test/ci-safe-stream-timeout-dl53ckue8ev4

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.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Gavel results

Gavel exited with code 1.

View full results

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