Skip to content

test: reduce fixed waits in slow suites#2003

Merged
yottahmd merged 3 commits intomainfrom
test/speed-up-windows-unit-tests
Apr 16, 2026
Merged

test: reduce fixed waits in slow suites#2003
yottahmd merged 3 commits intomainfrom
test/speed-up-windows-unit-tests

Conversation

@yottahmd
Copy link
Copy Markdown
Collaborator

@yottahmd yottahmd commented Apr 16, 2026

Summary

  • Replace fixed sleep-based test delays with release-file gates in slow cmd and API tests.
  • Parallelize isolated stop command subtests.
  • Stop waiting for the full worker retry timeout once a retry is observed.
  • Bound worker shutdown waits and replace the mixed local/distributed integration sleep gate with a release-file gate.

Local timing

  • internal/cmd: 27.5s before, 15.3s after on this machine.
  • internal/service/worker: 14.1s before, 11.3s after on this machine.
  • internal/service/frontend/api/v1: 35.2s before, 23.9s after on this machine.
  • internal/intg/distr -run TestParallel_MixedLocalAndDistributed: 1.5s after replacing fixed sleeps.

Testing

  • go test -count=1 ./internal/cmd
  • go test -json -count=1 ./internal/service/worker > /tmp/worker-mainbranch.jsonl
  • go test -json -count=1 ./internal/service/frontend/api/v1 > /tmp/api-v1-mainbranch.jsonl
  • go test -count=1 ./internal/service/worker
  • go test -count=1 ./internal/intg/distr -run TestParallel_MixedLocalAndDistributed

Summary by CodeRabbit

  • Tests
    • Enhanced test reliability and consistency by implementing improved synchronization mechanisms across test suites. Reduced reliance on fixed timeouts and increased test execution efficiency.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 16, 2026

📝 Walkthrough

Walkthrough

This pull request replaces time-based synchronization (fixed sleep durations) with a deterministic hold-file mechanism across multiple test files in the internal/cmd and internal/service packages. Tests now create temporary files to coordinate timing instead of relying on static delays, enabling more reliable and controlled test execution.

Changes

Cohort / File(s) Summary
Test Synchronization Helpers
internal/cmd/test_helpers_test.go, internal/service/frontend/api/v1/dagruns_test.go
Introduced new helper functions newHoldFile() and releaseHoldFile() to create and manage temporary hold files, plus OS-specific command generation and status polling logic for synchronizing test timing.
Command Tests with Hold-File Mechanism
internal/cmd/cleanup_test.go, internal/cmd/restart_test.go, internal/cmd/status_test.go, internal/cmd/stop_test.go
Replaced static sleep commands with dynamically constructed DAG steps using hold-file synchronization; updated test flow to release hold files after assertions and wait for DAG completion; added t.Parallel() calls in stop_test and fmt imports where needed.
Service API Tests with Hold-File Mechanism
internal/service/frontend/api/v1/proc_liveness_test.go, internal/service/frontend/api/v1/singleton_test.go
Replaced fixed sleep commands with hold-file-based blocking in DAG specs; integrated waitForDAGRunStatus() polling and explicit file release calls to coordinate test lifecycle and status transitions.
Worker Test Synchronization
internal/service/worker/worker_test.go
Replaced context timeout-based waiting with explicit context cancellation and require.Eventually() polling to verify retry behavior; improved goroutine lifecycle control via done channel.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'test: reduce fixed waits in slow suites' directly and clearly summarizes the main change: replacing fixed sleep-based delays with dynamic synchronization mechanisms across test suites to improve test performance.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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 test/speed-up-windows-unit-tests

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/service/worker/worker_test.go`:
- Around line 587-589: The subtest can hang because w.Stop is called with
context.Background() and the test then blocks on <-done without timeout; change
this to create a context with timeout (e.g., context.WithTimeout) and pass that
to w.Stop, and replace the plain receive on done with a testify assertion that
the stop completed before the timeout (use require.Eventually or require.NoError
with a select/timer) so the test fails fast on regressions; update references to
Stop, cancel, and done in this subtest accordingly.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e551d33e-2e6d-4e74-ab44-23f46e23550b

📥 Commits

Reviewing files that changed from the base of the PR and between 2177b91 and b83d52a.

📒 Files selected for processing (9)
  • internal/cmd/cleanup_test.go
  • internal/cmd/restart_test.go
  • internal/cmd/status_test.go
  • internal/cmd/stop_test.go
  • internal/cmd/test_helpers_test.go
  • internal/service/frontend/api/v1/dagruns_test.go
  • internal/service/frontend/api/v1/proc_liveness_test.go
  • internal/service/frontend/api/v1/singleton_test.go
  • internal/service/worker/worker_test.go

Comment thread internal/service/worker/worker_test.go Outdated
@yottahmd yottahmd merged commit fce7797 into main Apr 16, 2026
10 checks passed
@yottahmd yottahmd deleted the test/speed-up-windows-unit-tests branch April 16, 2026 07:59
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