e2e: replace fixed-sleep hook waits with event-driven waitForHook helper#1879
Open
TooTallNate wants to merge 1 commit intomainfrom
Open
e2e: replace fixed-sleep hook waits with event-driven waitForHook helper#1879TooTallNate wants to merge 1 commit intomainfrom
TooTallNate wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 8c293c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (1 failed)hono (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
workflow with 1 step💻 Local Development
workflow with 10 sequential steps💻 Local Development
workflow with 25 sequential steps💻 Local Development
workflow with 50 sequential steps💻 Local Development
Promise.all with 10 concurrent steps💻 Local Development
Promise.all with 25 concurrent steps💻 Local Development
Promise.all with 50 concurrent steps💻 Local Development
Promise.race with 10 concurrent steps💻 Local Development
Promise.race with 25 concurrent steps💻 Local Development
Promise.race with 50 concurrent steps💻 Local Development
workflow with 10 sequential data payload steps (10KB)💻 Local Development
workflow with 25 sequential data payload steps (10KB)💻 Local Development
workflow with 50 sequential data payload steps (10KB)💻 Local Development
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
stream pipeline with 5 transform steps (1MB)💻 Local Development
10 parallel streams (1MB each)💻 Local Development
fan-out fan-in 10 streams (1MB each)💻 Local Development
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates @workflow/core hook-related e2e tests to avoid fixed sleeps by introducing a shared waitForHook() polling helper, improving reliability on slow backends/runtimes and reducing unnecessary waiting on fast ones.
Changes:
- Added
waitForHook(token, { timeoutMs, intervalMs, runId })helper that pollsgetHookByToken()until the hook is available (optionally filtering byrunId). - Replaced multiple
setTimeout(5_000)-based waits withwaitForHook()across hook registration/resume e2e tests. - Standardized one remaining direct
setTimeoutusage tosleep(3_000).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+356
to
+358
| // Wait until the hook is registered (event-driven; faster than a | ||
| // fixed sleep on quick runtimes and tolerant of slow ones like the | ||
| // snapshot runtime on Vercel). |
There was a problem hiding this comment.
waitForHook is a polling loop (calls getHookByToken then sleeps), so describing it here as “event-driven” is misleading. Consider rewording this comment to “polling-based”/“retry loop” to match the actual mechanism and avoid confusion when debugging test flakiness.
Suggested change
| // Wait until the hook is registered (event-driven; faster than a | |
| // fixed sleep on quick runtimes and tolerant of slow ones like the | |
| // snapshot runtime on Vercel). | |
| // Wait until the hook is registered using a polling-based retry loop; | |
| // this is faster than a fixed sleep on quick runtimes and tolerant of | |
| // slow ones like the snapshot runtime on Vercel. |
Hook-related e2e tests (hookWorkflow, hookCleanupTestWorkflow,
hookDisposeTestWorkflow, hookWithSleepWorkflow, distributedAbortController)
previously slept a fixed 5 seconds before calling getHookByToken to wait
for the hook to be registered. On slower runtimes — notably the snapshot
runtime on Vercel where each workflow round-trip is several seconds longer
than replay — that fixed budget is too tight and the test fails with
HookNotFoundError. On faster runtimes it's unnecessarily slow.
Adds a waitForHook(token, { timeoutMs, intervalMs, runId }) helper that
polls until the hook resolves or the timeout (default 30s) expires, with
an optional runId filter for token-reuse tests where eventually-consistent
backends may briefly still report a stale hook. Each hook-wait site now
uses this helper. Non-hook fixed sleeps (workflow-progress polling for
sleepingWorkflow cancel tests, payload-processing waits in
hookWithSleepWorkflow) are unchanged.
TooTallNate
force-pushed
the
e2e-wait-for-hook
branch
from
3d9d5e3 to
8c293c9
Compare
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
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.
Summary
Replaces 8 fixed
setTimeout(5_000)waits in hook-related e2e tests with awaitForHook(token, { runId, timeoutMs, intervalMs })helper that pollsgetHookByTokenuntil it resolves or the timeout fires.Background
Hook-related e2e tests (
hookWorkflow,hookCleanupTestWorkflow,hookDisposeTestWorkflow,hookWithSleepWorkflow,distributedAbortController×3) all sleep a fixed 5 seconds before callinggetHookByToken, then assume the hook is registered. That budget is:createHookcall within 5s, and the test fails withHookNotFoundError.Fix
Adds a
waitForHookhelper at the top ofpackages/core/e2e/e2e.test.tsthat polls (default 250ms interval, 30s timeout) and exits early on success. The optionalrunIdfilter handles eventually-consistent backends where a stale lookup may still resolve to a previous run's hook for the same token (used byhookCleanupTestWorkflow/hookDisposeTestWorkflowtoken-reuse cases).Each affected call site replaces the
setTimeout(5_000) → getHookByToken(token)pair withwaitForHook(token, { runId: run.runId }). Non-hook fixed sleeps (thesleepingWorkflowcancel tests, payload-processing waits inhookWithSleepWorkflow) are untouched.Verification
The e2e file is exercised by the dedicated e2e CI pipeline against deployed worlds. Single-file change, 72 insertions / 49 deletions.
Extracted from PR #1300 (snapshot-runtime). The slowness issue was most visible on Vercel under the snapshot runtime where each round-trip is several seconds longer, but the fix makes the tests faster and more reliable on every runtime.