Repository navigation
test(web): accept remaining timeout budget in diagnostics - #1690
Conversation
github-actions
Bot
added
the
size:XS
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
[Codex] responding on behalf of shamilovtim Head GitHub denied adding |
okwasniewski
left a comment
okwasniewski
left a comment
There was a problem hiding this comment.
[claude-opus-5-5] automated review on behalf of @okwasniewski
The diagnosis is right. clickControl starts its Deadline before the first evaluateHandle, so 1999 ms is a valid value and the exact 2000ms match was the bug. Fixing the test is the right fix.
One change is needed before merge:
toBeGreaterThan(0)can never fail, becauseremaining()is clamped to at least 1 (actions.ts:172). As written, the test accepts any timeout up to 2000. To check, I changedactions.ts:185to{ timeout: Math.max(1, remaining() / 2) }, so the tap gives up at half its budget. This branch stays green (the whole file passes 83/83), while main's assertion fails. The test is named "within one deadline", so it should pin the full budget. The drift before the first read is a few ms, so a floor liketoBeGreaterThan(1500)stays stable and still catches this.
Nit: if the regex stops matching, Number(undefined) fails with "expected NaN to be greater than 0", which hides the real message. Keep the prefix check first:
expect(error?.message).toMatch(/^tap did not become actionable in time: .*Timeout \d+ms exceeded/);
const timeoutMs = Number(/Timeout (\d+)ms exceeded/.exec(error?.message ?? '')?.[1]);
expect(timeoutMs).toBeGreaterThan(1500);
expect(timeoutMs).toBeLessThanOrEqual(2000);Require a remaining timeout above 1500 ms and no greater than 2000 ms, and check the diagnostic prefix before parsing. This preserves tolerance for deadline drift while catching a tap that receives only half its budget.
|
[Codex] responding on behalf of shamilovtim Fixed both points from the review in 82076ff. The remaining timeout must now be above 1500 ms and no greater than 2000 ms, and the anchored prefix assertion runs before parsing. The Verified the same integer half-budget mutation against both assertions: the old file passed 83/83; the revised file passed 82 and failed only this case with All current-head CI gates passed. The earlier formal change request remains pending maintainer re-review. |
okwasniewski
left a comment
okwasniewski
left a comment
There was a problem hiding this comment.
[claude-opus-5-5] automated review on behalf of @okwasniewski
Re-checked at 82076ff. Both asks are done:
- The floor is now
> 1500. I re-ran the half-budget mutant (actions.ts:185->remaining() / 2), and it now fails withexpected 1000 to be greater than 1500. On the PR head,check-actions.test.tspasses 83/83. - The prefix assertion runs before parsing.
CI is green. Approving and queueing.
NOT_ACTIONABLE/ nonretryable result and 3500 ms elapsed-time bound. Only the existing test changes.Issue
Fixes #1689.
Verified
Ran it locally: yes
The original main assertion failed on a valid 1999 ms diagnostic in CI. The revised assertion passed the complete web package suite in real Chromium:
For a negative control, changed only the locator
evaluateHandletimeout toMath.max(1, Math.floor(remaining() / 2)):expected 1000 to be greater than 1500Restored production code and rebuilt; the revised case passed.
pnpm checkalso passed.