Repository navigation
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 18 files
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
View guided diff | Re-trigger cubic
…out, and refine latency docs (#1099)
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
okwasniewski
left a comment
There was a problem hiding this comment.
Thanks for picking this up, the mobile ReadBudget plumbing and the sparse-retry stop are useful pieces. A few things before this can land:
-
#1099 asks for the poll to give the last read extra time past the deadline, so a slow read still finishes. This PR keeps the read cut at the deadline and only changes the error it reports. On main a slow last read that finds the element passes (the deleted "lets a locate outlive its budget" test covered that); here it becomes
LOCATOR_NOT_FOUND. Could the last read's budget beremaining + latency(capped by the phase budget) instead? -
A fixed 2,000 ms constant is a guess that will break on slower CI devices or bigger trees (a 3 s read starting with 2.5 s left is labelled hung again). We'd rather the poll measure it: track how long completed reads take in that poll, and use that for both the extra time and the cut-off check. That also drops the
readLatencyengine field entirely. -
If an engine field stays: newer
@e2e-dev/mobilewith an oldere2e(peer is>=0.15.0) fails withunknown key "readLatency", so the peer floor would need raising. An engine contract change also needs a@e2e-dev/webchangeset (see CONTRIBUTING, Engine contract). -
Budgets: each snapshot/screenshot creates a fresh
ReadBudget, so observe can take up to 3xtimeoutMs. Could we create one budget per operation inengine.ts(likeInputBudget) and pass it through, instead of optionaltimeoutMs?params? That also covers the keyboard-dismiss snapshots, which are still unbounded, and letsReadBudgetfold intoInputBudget(remainingMs()is unused). -
Tests: please add a mobile +
LocatorEnginetest for slow-but-valid last read ->LOCATOR_NOT_FOUND, hung read with time to spare ->ACTION_FAILED, and a late match that still resolves. The new locator test relies on 100 ms real-timer margins; fake timers would make it deterministic. -
Docs/changeset: keep them to what users see, without internal names (
pollCondition, "read latency tolerance"). The changeset says the latency is granted past the deadline, which this version doesn't do.
Happy to help shape the measured-latency version if useful.
…to input budget (#1099)
|
@okwasniewski Thanks for the detailed review! All points have been addressed in the latest update:
|
There was a problem hiding this comment.
All reported issues were addressed across 20 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
okwasniewski
left a comment
There was a problem hiding this comment.
Thanks for the quick and thorough turnaround, @Totopo27! Dropping the engine field, measuring latency, sharing one budget per operation and moving to fake timers are all the right moves. A few things before this can land:
- Assertions and
waitForstill cut the last read. The grace only applies inresolveCounted,performWithinandscrollUntilVisible.pollCondition(expect(locator),locator.waitFor,toHaveScreenshot) measures latency but still reads withoperationWithin(deadline). Mobile now enforces that bound, soexpect(screen.getByText('Saved')).toBeVisible()on iOS, where the text shows up during the last snapshot, passes on main and fails here withASSERTION_FAILED. - One helper instead of four copies. The latency/readTimeout/cut-off logic is in 4 places (
engine.ts:231,engine.ts:422,screen.ts:277,time.ts:128), andresolveOnce's retry path still uses the old 100ms rule. Could this be one small helper ininternal/time.ts, withpollCondition'sevaluatereceiving the readtimeoutMs? That fixes (1) too. While there: time only the engine call (the sample currently includes retry sleeps), don't hand each retry the fullreadTimeoutagain, and keep theMath.max(1, ...)floor on an explicittimeoutMs. - Peer floor. New
@e2e-dev/mobilewith an oldere2e(peer>=0.15.0) bounds reads but gets no grace from the old core, so a slow last read isACTION_FAILEDagain, which is the original #1099 failure. The peer floor needs to move to thee2eversion that ships this. InputBudget.read()should honorsent, likecheck()does. After the keyboard-dismiss swipe goes out, a snapshot that times out should beACTION_MAY_HAVE_COMMITTED, notOPERATION_TIMEOUT, so a replay doesn't send the input again.- Docs.
docs/reference/engine.mdx: please restore the caveat thatsignaldoes not abort attimeoutMs. That is still true, and engine authors rely on it.docs/mobile.mdxand the changeset should only claim the grace where it applies. A late match on an action still leavesperformwith no budget, so "avoiding timeouts when elements appear late" overstates it today. - Tests. "slow-but-valid last resolve gives
LOCATOR_NOT_FOUND" also passes on main, because the fake engine ignorestimeoutMs. Could it enforce the timeout it's given? The shared-observe-budget test times out in the viewport probe, so it needs viewport geometry on the snapshot. A mobile-level test for a slow last read that still finishes would close the loop.
Smaller: capture() can still send a second snapshot after the budget has abandoned the read; the InputBudget doc still says "input only"; and the path.posix.join change looks unrelated to this PR.
This was an automated review.
…-budget # Conflicts: # packages/mobile/src/surface.ts
Address review: one ReadWindow helper for every wait (pollCondition, locator waits, scrollUntilVisible, toHaveScreenshot), InputBudget.read honors sent, no snapshot retry after an abandoned capture, sparse retries stop when the budget cannot fit one, mobile peer e2e >=0.21.2. Co-Authored-By: Claude <noreply@anthropic.com>
|
Automated review (Claude Code on behalf of @okwasniewski) Since there were no updates after our last review, we finished this ourselves and pushed to your branch. We merged main, no force push.
Open: |
Co-Authored-By: Claude <noreply@anthropic.com>
|
Automated review (Claude Code on behalf of @okwasniewski) Follow-up in 21aeaa9: |
There was a problem hiding this comment.
All reported issues were addressed across 21 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Re-trigger cubic
Addressed (maintainer finished the remaining changes or author did). Automated review.
…ercise ReadWindow cap
|
Automated review (Claude Code on behalf of @okwasniewski) Thanks @Totopo27. I checked aaac086 and 2018295 and pushed a small follow-up on top (no force push):
|
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
|
Thanks @Totopo27. Re-checked after the two main merges (02ad4dd). The merge resolution is right: the I pushed f7f9584 with two small fixes:
Left for you (small)
This was an automated review. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
View guided diff | Re-trigger cubic
…99-mobile-reads-budget
|
Thanks @Totopo27. Re-checked f9edf91 (a main merge on top of our f7f9584). Our peer floor ( The merge dropped main's Nits
This was an automated review. |
# Conflicts: # packages/e2e/tests/unit/locator-engine.test.ts # packages/mobile/src/surface.ts
Only tests passed a Deadline; drop the instanceof branch.
What and why
@e2e-dev/mobilea snapshot or screenshot that hung held the step until the test timeout (Bound mobile reads by the operation budget without turning a slow last poll into a timeout #1099). Every device read now runs under the operation's budget (InputBudget.read,packages/mobile/src/support.ts), so a hung read fails the step asOPERATION_TIMEOUT(orACTION_MAY_HAVE_COMMITTEDonce an input of that operation went out).waitFor/expect/toHaveScreenshotgot cut at the deadline and failed as a hang instead ofLOCATOR_NOT_FOUND.ReadWindow(packages/e2e/src/internal/time.ts). Each read of one wait gets the time left plus the slowest read that wait already finished, capped by the running phase's deadline. A read cut off within that tail ends the wait as a normal timeout; a read that hung with more time left is still the failure.@e2e-dev/mobilenow needse2e >=0.21.4(this PR's e2e patch changeset ships it): on 0.21.3 core, mobile reads would end at the deadline with no last-read grace.ReadWindowis internal;OperationContext.timeoutMsdocs note the last read can get more than the deadline leaves.Issue
Fixes #1099
Verified
Ran it locally: no device run. Unit evidence on the head merged with main (
f4e8c8e1a+ nit7f1d05ed8):readWithin-> direct call) fails 6 mobile tests, e.g. "bounds a snapshot by the operation budget" and "takes no second snapshot for a capture the budget abandoned".ReadWindow.timeoutMs()fails 5 core tests (time, locator-engine, locator-wait-for), e.g. "lets a slow last resolve finish past the deadline: a late match resolves".ReadWindow.cutOff()fails "ends on a read cut off within the longest read time of the deadline, and fails on one that hung with more left".pnpm checkpasses; e2e (3171), mobile (420), web (290) unit suites pass.expectfails as a hangLOCATOR_NOT_FOUND/ASSERTION_FAILEDChecklist
pnpm changeset) for any change to a published package, or the change is testbed, docs, or CI only.!, the changeset body names what breaks and what replaces it, and the deprecation warning landed a window earlier (stability policy in CONTRIBUTING.md).docs/**/*.mdxpage for the behavior, andskills/e2e/if the skill describes it..d.tsreviewed,tests/types/sdk-types.tsupdated; wire change edits the schema and both fixtures.e2e/engine): changesets fore2e,@e2e-dev/web, and@e2e-dev/mobile.