Skip to content

refactor(tests): replace duplicated wait-for helpers with testutil.Wa… - #1039

Open
Fayupable wants to merge 1 commit into
Gitlawb:mainfrom
Fayupable:1023-shared-waitfor-helper
Open

refactor(tests): replace duplicated wait-for helpers with testutil.Wa…#1039
Fayupable wants to merge 1 commit into
Gitlawb:mainfrom
Fayupable:1023-shared-waitfor-helper

Conversation

@Fayupable

@Fayupable Fayupable commented Sep 9, 2026

Copy link
Copy Markdown

Fixes #1023

Summary

Extracts the duplicated poll-until helper functions scattered across internal/swarm and internal/daemon test files into a single shared testutil.WaitFor helper in a new internal/testutil package. All call sites (34 total, across 7 files, since the helper turned out to be used more widely than the 4 locations originally listed in the issue) were migrated to the shared function.

Note: I see #1037 is already open for this issue. I had this ready before noticing it, so I'm submitting it as an alternative in case it's useful for comparison.

Linked issue

Fixes #1023

Checklist

  • The linked issue already has the issue-approved label.
  • go build ./..., go vet ./..., and go test ./... pass locally.
  • gofmt clean.
  • Tests added/updated for the change (and run under -race where relevant).
  • UI changes include screenshots or a short recording where possible.

Summary by CodeRabbit

  • Tests
    • Improved asynchronous test reliability by standardizing condition-wait behavior across diagnostics, daemon, and swarm test coverage.
    • Added clearer wait descriptions and consistent timeout handling for readiness, scheduling, lifecycle, and queue-related checks.
    • Consolidated repeated polling logic into a shared testing utility, reducing duplication and making future test maintenance easier.

Copilot AI lite review requested due to automatic review settings September 9, 2026 09:16
@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR consolidates duplicated polling helpers into a shared internal/testutil.WaitFor utility while preserving the migrated test conditions.

  • Adds a shared polling helper with a five-second timeout and descriptive failure messages.
  • Migrates 34 call sites across agent, daemon, and swarm tests.
  • Removes the package-local helper implementations.

Confidence Score: 5/5

The PR appears safe to merge, with the shared helper preserving the relevant behavior of the removed test-local implementations.

The migrated predicates retain their original locking and state checks, while the shared helper provides an equal or longer timeout at every call site.

Important Files Changed

Filename Overview
internal/testutil/waitfor.go Introduces the shared polling helper with equivalent behavior and a more generous timeout.
internal/agent/async_diagnostics_test.go Replaces the local idle-wait helper while preserving mutex-protected state inspection.
internal/daemon/pool_test.go Migrates queue-depth polling to the shared helper without changing conditions.
internal/daemon/server_test.go Migrates status-file polling while retaining the original os.Stat success condition.
internal/daemon/session_test.go Migrates session-state polling to the shared helper.
internal/swarm/lifecycle_test.go Removes the package-local helper and migrates lifecycle synchronization checks.
internal/swarm/scheduler_test.go Migrates scheduler completion, spawn, skip, and retirement polling.
internal/swarm/tools_test.go Migrates asynchronous tool-state checks without changing their predicates.

Reviews (1): Last reviewed commit: "refactor(tests): replace duplicated wait..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is a straightforward, verified test-only refactor that removes duplication without altering production behavior.

Pull request overview

This PR consolidates multiple duplicated “poll-until” test helpers into a single shared helper (internal/testutil.WaitFor) to standardize asynchronous waiting behavior across test suites and make future timeout tuning centralized.

Changes:

  • Added internal/testutil.WaitFor (5s default deadline, 5ms poll interval, descriptive timeout failure).
  • Migrated existing tests in internal/swarm, internal/daemon, and internal/agent to use testutil.WaitFor.
  • Removed the now-redundant per-file wait helper implementations.
File summaries
File Description
internal/testutil/waitfor.go Introduces shared WaitFor polling helper for tests.
internal/swarm/tools_test.go Replaces local wait calls with testutil.WaitFor.
internal/swarm/scheduler_test.go Migrates scheduler polling waits to testutil.WaitFor.
internal/swarm/lifecycle_test.go Removes local helper; updates all waits to testutil.WaitFor.
internal/daemon/session_test.go Updates session state waits to testutil.WaitFor with descriptive labels.
internal/daemon/server_test.go Replaces file-appearance polling helper with testutil.WaitFor + os.Stat.
internal/daemon/pool_test.go Replaces pool queue-depth waits with testutil.WaitFor.
internal/agent/async_diagnostics_test.go Replaces idle-wait helper with testutil.WaitFor using a mutex-protected condition.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 8e662c1b-c1ef-40a1-9949-01eee5fd7905

📥 Commits

Reviewing files that changed from the base of the PR and between f30f550 and 45557ec.

📒 Files selected for processing (8)
  • internal/agent/async_diagnostics_test.go
  • internal/daemon/pool_test.go
  • internal/daemon/server_test.go
  • internal/daemon/session_test.go
  • internal/swarm/lifecycle_test.go
  • internal/swarm/scheduler_test.go
  • internal/swarm/tools_test.go
  • internal/testutil/waitfor.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The change adds a shared testutil.WaitFor helper and replaces local asynchronous polling helpers across agent, daemon, and swarm tests.

Changes

Shared asynchronous test polling

Layer / File(s) Summary
Shared polling helper
internal/testutil/waitfor.go
Adds WaitFor, which polls every 5ms for up to five seconds and reports the description on timeout.
Agent diagnostics migration
internal/agent/async_diagnostics_test.go
Replaces the local idle-wait helper with testutil.WaitFor.
Daemon test migration
internal/daemon/*_test.go
Replaces pool, server, and session polling helpers with testutil.WaitFor.
Swarm test migration
internal/swarm/*_test.go
Replaces lifecycle, scheduler, and tool polling helpers with testutil.WaitFor.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 45557

This change centralizes asynchronous test polling while preserving the existing conditions being awaited. It affects test behavior only, with no current merge-blocking risk identified.

Suggested reviewers: gnanam1990

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: replacing duplicated test wait helpers with shared testutil.WaitFor.
Linked Issues check ✅ Passed The PR satisfies issue #1023 by adding internal/testutil.WaitFor with a centralized 5-second deadline, 5-millisecond polling interval, and condition-specific failure reporting. It migrates the identif…
Out of Scope Changes check ✅ Passed All changes support the stated refactor. The additional migrated test helpers address the same duplicated polling pattern and do not introduce unrelated behavior changes.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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.

tests: four independent waitFor helpers, no shared poll-until helper

2 participants