test: assert the specific error code instead of any failure (#1781 B4) - #1790
Merged
Conversation
Converts the 20 test assertions across the repo that accepted ANY
failure (bare `expect(...).toThrow()`, bare `assert.throws(fn)`, bare
`assert.rejects(p)`) into assertions on the specific AppError `code`
each test is actually about, or — where the propagated error is
genuinely opaque (a mocked upstream failure whose identity, not its
shape, is the point) — identity assertions with a comment explaining
why.
Added a synchronous `assertThrowsAppError(fn, {code, message?})`
sibling to the existing `assertRejectsAppError` helper in
src/__tests__/test-utils/app-error.ts, exported via the test-utils
index, for the two src/ sites that needed it.
packages/provider-limrun and packages/provider-webdriver have no
test-utils dir and cannot import from src/, so those sites use
vitest's `expect(...).toThrow(expect.objectContaining({ code }))` or
an inline `assert.rejects(p, matcherFn)` instead.
Sites converted:
- packages/provider-limrun/src/app-log-runtime.test.ts:153-155
(bare `.toThrow()` x3 -> `UNSUPPORTED_OPERATION`)
- src/daemon/__tests__/app-log.test.ts:39 (bare `.toThrow()` ->
message match; plain Error, not AppError, from verified-file's
identity check)
- src/daemon/__tests__/resumable-upload-range.test.ts:13 (bare
`assert.throws(fn)` -> `INVALID_ARGS`); also fixed line 19's
`assert.throws(fn, value)`, a documented Node.js gotcha where a
string second argument is the failure message, not a matcher, so
it was equally bare in effect
- packages/provider-webdriver/src/webdriver-client.test.ts:229 (bare
`assert.rejects(p)` -> asserts the raw AbortSignal.timeout()
rejection's `name`, since the transport re-throws it unwrapped)
- src/daemon/handlers/__tests__/session-device-claims.test.ts:129,
151, 174 (bare `assert.rejects(p)` x3 -> identity assertions; each
test's point is device-claim rollback/retention around an opaque
mocked upstream failure)
- src/platforms/android/__tests__/settings.test.ts:109 (bare
`assert.rejects(p)` -> `UNSUPPORTED_OPERATION`)
- src/platforms/android/__tests__/snapshot.test.ts:1071, 1342 (bare
`assert.rejects(p)` x2 -> `COMMAND_FAILED` + message)
- src/platforms/android/__tests__/touch-helper-session.test.ts:526
(bare `assert.rejects(p)` -> `COMMAND_FAILED`, wrong-protocol
message)
- src/platforms/apple/core/__tests__/runner-command-retry.test.ts:472,
527, 550, 762, 881, 1016 (bare `assert.rejects(p)` x6 ->
`COMMAND_FAILED` with the recovery-path-specific details/message)
- src/platforms/apple/core/__tests__/runner-transport.test.ts:61
(bare `assert.rejects(p)` -> identity assertion; fetchWithTimeout
does not wrap fetch() failures into an AppError)
No repo-wide scanner/lint rule added (explicitly out of scope per
#1781); no test loosened.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
marked this pull request as ready for review
August 17, 2026 17:39
Member
Author
|
Request changes before merge:
The changed assertions themselves appear honest on review. CI is green for the relevant test gates; the only failure is CodeQL Java/Kotlin failing while GitHub returned 503 during SARIF upload, which is infrastructure-unrelated to this TypeScript-only test change. |
14 tasks
Member
Author
|
Both points addressed in the PR body — no code changes were needed, so no new commit. 1. Count. You're right, and the fix is arithmetic, not scope. It's 21, made of 20 + 1:
The old body reported the scan's 20 as if it were the conversion count and buried the +1 in a per-site bullet. Body now states 21 up front with the breakdown, and carries your 4/2/15 tally explicitly. 2. Red evidence. Done properly — I made each matcher class reject its wrong failure rather than just one code and one identity, since the classes fail differently. Nine single-site mutations, each applied alone and reverted before the next; full table with quoted output in the body. Summary:
The identity mutation is deliberately the mean one: a same-message replacement error. That pins the propagation path itself, not the wording — which is the only thing an identity exemption can honestly claim to pin. I also added the half your request implies but doesn't ask for: proof the old assertions were not load-bearing. One product-side perturbation, applied to
Green on main, red here — the pins are new, not decorative. All mutations reverted, CI: re-checked on the current run — |
|
thymikee
added a commit
that referenced
this pull request
Aug 18, 2026
…et probe main's #1790 tightened this test to expect the raw TimeoutError DOMException, which this PR intentionally normalizes into AppError{reason: webdriver_request_timeout}. On the merge ref the two met and Coverage went red. The regression now asserts the structured contract and that the second request's budget is the shared remainder (~118ms of 200 after an 80ms first call).
thymikee
added a commit
that referenced
this pull request
Aug 18, 2026
…eaking billed sessions (#1782) * fix(webdriver): give cloud session creation its own budget and stop leaking billed sessions Cloud lease allocation ran under the generic 30s/1-retry request policy, so BrowserStack iOS real-device session creation (45-90s) aborted client-side at ~60s on most runs. Each timed-out POST /session still completed server-side and, being non-idempotent, was retried — leaving two billed provider sessions per failed open with no id to release them. - POST /session is its own phase: a 180s create budget (default), zero retries, and no request-bound abort, so the daemon always learns the session id. - lease_allocate carries a 300s allocation budget surfaced to providers as LeaseLifecycleContext.deadline, and a matching 330s client envelope that preserves the daemon on timeout (a reset would SIGKILL mid-create and orphan every billed session the daemon held). - The request's cancellation signal is ownership evidence: a session that completes after the requester left is released, not registered; a create that the transport gives up on surfaces typed evidence (provider + lease) so an operator can find and stop the maybe-orphaned session. Closes #1774 * refactor: one canceled-request error, and tighten the #1774 shapes Review pass over the session-create fix: - The canceled-request error had nine hand-rolled copies (src/request/cancel, maestro shared, exec, retry, install-source x2, and the new provider one). It now has one definition in @agent-device/kernel/errors: createRequestCanceledError(details?, cause?) + isRequestCanceledError + REQUEST_CANCELED_REASON. Callers add evidence or a sharper hint; the reason itself is not overridable, so nothing can build one the predicate misses. - lease_allocate's timeout bundle moves beside INSTALL_TIMEOUT_POLICY in the registry (same {...DEFAULT, envelopeMs, onTimeout} shape); the request timeout constant stays exported from timeout-policy like its siblings. - Transport: fetch helper returns Response's own ok/status; the timeout reason const is private behind isWebDriverRequestTimeout. - Client: one-use options type inlined; the two deadline helpers share one floor. - Session-manager tests: shared makeRuntime/jsonResponse/afterEach restore. Net -29 lines with the feature in. * chore: keep the canceled-request reason private to the kernel * fix: typed cancellation everywhere + own the AWS remote-access ARN through startup Second-order follow-ups the #1774 refactor made cheap: - markRequestCanceled aborts the request signal WITH the kernel's typed canceled error as its reason. Every signal.throwIfAborted(), aborted fetch, and 'throw signal.reason' in the daemon (20+ sites) now surfaces a canceled request as such instead of a bare DOMException that normalized to UNKNOWN — and no site has to know the factory exists. - AWS Device Farm prepareSession owns the remote-access ARN from the moment create-remote-access-session answers: a startup timeout, the allocation deadline, or a canceled request now stops it before the failure surfaces (previously a timed-out startup left a RUNNING billed session behind — the same leak class as the WebDriver session, one phase earlier). The startup wait is capped by LeaseLifecycleContext.deadline and wakes on cancellation. - BrowserStack's pre-session local app upload honors the request signal (an upload is not billed, so plain abort is right there). - lease_heartbeat/lease_release share lease_allocate's preserve-daemon policy: the rationale — the daemon owns billed sessions; a reset orphans them all — applies verbatim. Each AWS ownership test proven red without the guard (3/3). * refactor: dedupe billed-resource cleanup and lease-signal wiring Shrink pass — same behavior, less duplication: - releaseOnFailure(primaryError, release) in webdriver-utils replaces the two identical 'best-effort stop the billed resource, attach cleanupError to the primary AppError' helpers (WebDriver session + AWS remote-access ARN); shared errorMessage too. - The lease handler pulls the request signal from getRequestSignal(requestId) like every sibling handler, instead of threading a requestSignal arg through LeaseHandlerArgs and the request-handler chain. Drops the field, the wiring, and five mechanical test edits; the handler test now proves the request-bound signal (abort it, watch the provider's signal flip) rather than arg identity. - Inlined the one-use requestHeaders back into fetchWebDriver. Handler-signal test proven red without the wiring. * fix(lease): the daemon releases a lease allocated for a gone requester; honest release evidence Review follow-up. The provider was doing the daemon's job: it treated the request signal as 'ownership evidence, not an interrupt' and needed three paragraphs to say so. The daemon owns the request, so it now decides — generically, for every provider — what happens to a lease that finished allocating after its requester left: release it (provider + registry) and answer with the canceled error. - lease.ts: after allocate returns, isRequestCanceled(requestId) → releaseAllocationForGoneRequester(). Release evidence is claimed ONLY on a clean release (no warnings, no throw); a WEBDRIVER_SESSION_DELETE_FAILED release is reported released:false with providerSessionId + a stop-by-hand hint (thymikee's finding: the previous evidence was success-shaped even when DELETE failed). - WebDriverSessionManager: the createOwnedSession/releaseCanceledSession trio is gone; allocate is plain 'create with a budget; on failure clean up' again. - LeaseLifecycleContext.signal is just cancellation, like everywhere else; the ownership-semantics comments on the contract, client, registry, AWS prepare and utils shrink to what the code no longer says itself. - Tests: the two provider-level cancellation tests move to the daemon handler (where the logic now lives), plus the failing-DELETE regression; both proven red without the post-allocate check. * fix(aws): the allocation deadline bounds remote-access startup, not the 120s default Live iOS real-device run: startup needed ~128s and hit the standalone 120s default while the daemon's 300s allocation budget still had room — the new ownership guard correctly stopped the ARN, but the open failed for no reason. When the daemon supplies a deadline it is the bound; the default only applies standalone. Rerun: open in 112s, snapshot, clean close, session STOPPING. * test(aws): pin that the allocation deadline outlives the 120s startup default; drop empty import Review follow-ups on 7f9d148: a virtual-clock test (Date.now advanced 10s per poll, RUNNING at 150s, deadline 300s) that fails on the old min(default, deadline) logic and passes now; and the empty 'import {} from kernel/errors' left in maestro/shared.ts is removed. * refactor: finish the dedupe — one release path, kernel errorMessage, AWS on releaseOnFailure Code-quality review at 7f9d148: 1. aws-device-farm.ts still carried its own copy of releaseOnFailure (the dedupe commit's script aborted before reaching it and I mis-verified). Now uses the shared helper; private copy deleted. 2. Empty 'import {} from kernel/errors' in maestro/shared.ts removed (2738700). 3. errorMessage() lives in @agent-device/kernel/errors; the two copies this PR had added (lease.ts, webdriver-utils.ts) import it. Sweeping the pre-existing copies is a follow-up. 4. lease.ts has ONE release path: releaseLease(registry, provider, lease, request, ctx) → { released (registry), provider } used by both the lease_release case (wire shape unchanged) and the gone-requester branch, which folds a throwing provider release into releaseError. 'released' now means the same thing in both; the provider verdict is a separate 'providerReleased' (warnings-free, no throw) that drives the stop-by-hand hint. -~35 lines. 5. sessionCreateTimeoutMs is Omit-ed at the WebDriverTransportOptions boundary instead of Pick-ed back out internally. * fix(lease): 'released' on a canceled allocation means the billed session is confirmed gone Re-review at 3665ea0: unifying the release path had made the cancellation error report released:true from the daemon's registry record while the provider DELETE had failed — success-shaped again, with the operator verdict demoted to a second key. Fixed at the source of the ambiguity: - LeaseReleaseOutcome names its bookkeeping field registryReleased. - On the canceled error, 'released' is true only when registryReleased AND the provider released without warnings AND without throwing; the registry record is exposed as 'registryReleased'. The stop-by-hand hint keys on 'released'. - lease_release keeps its existing wire field ('released' = registry; provider cleanup rides in 'provider'), unchanged. - Regressions: failed DELETE and throwing release both pin released:false / registryReleased:true (+ providerSessionId, warnings|releaseError, hint); both proven red on registry-only semantics. * ci: retrigger default-setup CodeQL Run 32051017472 is wedged on GitHub's side: status=completed with Analyze (python) still queued and Analyze (java-kotlin) failed only at SARIF upload (503, 'No server is currently available'). It can be neither cancelled nor rerun, and default-setup CodeQL has no dispatchable workflow, so a new push is the only way to get a fresh run. No source change. * test(webdriver): assert the typed timeout contract on the shared-budget probe main's #1790 tightened this test to expect the raw TimeoutError DOMException, which this PR intentionally normalizes into AppError{reason: webdriver_request_timeout}. On the merge ref the two met and Coverage went red. The regression now asserts the structured contract and that the second request's budget is the shared remainder (~118ms of 200 after an 80ms first call).
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
1 participant
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
Umbrella #1781, item B4 "assert the right reason". The repo's typed-error rule: failures are
AppErrorwith acodefrom the closedKNOWN_APP_ERROR_CODESset (packages/kernel/src/errors.ts). This PR converts the 21 test assertions across the repo that instead accepted ANY failure — bareexpect(...).toThrow(), bareassert.throws(fn), bareassert.rejects(p)— into assertions on the specific code (or, where the propagated error is genuinely opaque, an identity assertion with a comment explaining why).Count breakdown (correcting an earlier revision of this body, which said 20 and disagreed with its own site list): 20 + 1 = 21.
.not.toThrow()excluded), and it is the number the earlier body quoted.resumable-upload-range.test.ts:19'sassert.throws(fn, value). A string second argument to node:assert'sthrows/rejectsis the assertion failure message, never an error matcher — a documented Node.js gotcha — so that site accepted any error too. It is converted here as well, which is why the per-site list below totals 21 rather than 20.Added a synchronous
assertThrowsAppError(fn, {code, message?})sibling to the existingassertRejectsAppErrorhelper insrc/__tests__/test-utils/app-error.ts, exported via the test-utils index.packages/provider-limrunandpackages/provider-webdriverhave no test-utils dir and cannot import fromsrc/, so those sites use vitest'sexpect(...).toThrow(expect.objectContaining({ code }))or an inlineassert.rejects(p, matcherFn)instead — no cross-package imports, no new re-export surface.Sites converted
packages/provider-limrun/src/app-log-runtime.test.ts:153-155— bare.toThrow()x3 →UNSUPPORTED_OPERATIONsrc/daemon/__tests__/app-log.test.ts:39— bare.toThrow()→ message match (plainError, notAppError, from verified-file's identity check; message differs by operation)src/daemon/__tests__/resumable-upload-range.test.ts:13— bareassert.throws(fn)→INVALID_ARGS. Also line 19'sassert.throws(fn, value)— the string-second-argument gotcha described above, the +1 in the count.packages/provider-webdriver/src/webdriver-client.test.ts:229— bareassert.rejects(p)→ asserts the rawAbortSignal.timeout()rejection'sname, since the transport re-throws it unwrapped (not anAppError)src/daemon/handlers/__tests__/session-device-claims.test.ts:129,151,174— bareassert.rejects(p)x3 → identity assertions; each test's point is device-claim rollback/retention behavior around an opaque mocked upstream failure, not any particular error shapesrc/platforms/android/__tests__/settings.test.ts:109— bareassert.rejects(p)→UNSUPPORTED_OPERATIONsrc/platforms/android/__tests__/snapshot.test.ts:1071,1342— bareassert.rejects(p)x2 →COMMAND_FAILED+ messagesrc/platforms/android/__tests__/touch-helper-session.test.ts:526— bareassert.rejects(p)→COMMAND_FAILED, wrong-protocol messagesrc/platforms/apple/core/__tests__/runner-command-retry.test.ts:472,527,550,762,881,1016— bareassert.rejects(p)x6 →COMMAND_FAILEDwith the recovery-path-specific details/messagesrc/platforms/apple/core/__tests__/runner-transport.test.ts:61— bareassert.rejects(p)→ identity assertion;fetchWithTimeoutdoes not wrapfetch()failures into anAppErrorTotals by matcher: 4
.toThrow, 2assert.throws, 15assert.rejects= 21.No repo-wide scanner/lint rule added (explicitly out of scope per #1781); no test loosened.
Red evidence
docs/agents/testing.mdrequires red evidence for a regression pin. Every matcher class introduced here was made to face its wrong failure and observed to reject it. Each mutation was applied alone and reverted before the next.assertRejectsAppError(p, {code})android/__tests__/settings.test.ts:109UNSUPPORTED_OPERATION→INVALID_ARGSAssertionError: Expected values to be strictly equal:+ 'UNSUPPORTED_OPERATION'- 'INVALID_ARGS'— 1 failed | 14 passed (15)assertThrowsAppError(fn, {code, message})(new)daemon/__tests__/resumable-upload-range.test.ts:14INVALID_ARGS→COMMAND_FAILED+ 'INVALID_ARGS'- 'COMMAND_FAILED'— 1 failed | 1 passed (2).toThrow(expect.objectContaining({code}))provider-limrun/src/app-log-runtime.test.ts:153UNSUPPORTED_OPERATION→INVALID_ARGSAssertionError: expected error to match asymmetric matcher— 1 failed | 9 passed (10)assert.rejects(p, fn)asserting AppError code+detailsapple/core/__tests__/runner-command-retry.test.ts:478COMMAND_FAILED→INVALID_ARGS+ 'COMMAND_FAILED'- 'INVALID_ARGS'— 1 failed | 37 passed (38)assert.rejects(p, fn)asserting AppError code+messageandroid/__tests__/touch-helper-session.test.ts:537COMMAND_FAILED→INVALID_ARGS+ 'COMMAND_FAILED'- 'INVALID_ARGS'— 1 failed | 8 passed (9).toThrow(regex)message matchdaemon/__tests__/app-log.test.ts:39markbranch expects/must not be a symbolic link/expected [Function] to throw error matching /must not be a symbolic link/ but got 'Final path must be a regular file: …'— 2 failed | 2 passed (4)(error) => error === Xdaemon/handlers/__tests__/session-device-claims.test.ts:129AssertionError: The validation function is expected to return "true". Received false/Error: device not ready— 1 failed | 8 passed (9)(error) => error === Xapple/core/__tests__/runner-transport.test.ts:61fetchthrows a different instance, same messageThe validation function is expected to return "true". Received false/Error: request timed out after reaching runner— 1 failed | 5 passed (6)AppErrorname assertionprovider-webdriver/src/webdriver-client.test.ts:229nameTimeoutError→AbortErrorThe validation function is expected to return "true". Received false/TimeoutError: The operation was aborted due to timeout— 1 failed | 12 passed (13)F1 and F2 are the ones worth reading closely: the identity exemptions reject a same-message replacement error, so they pin the propagation path itself, not the wording.
Before/after A/B — the assertions are newly load-bearing
The table above shows the new assertions reject a wrong expectation. This pair shows the old ones did not, by applying one product-side perturbation to
origin/main's file and to this branch's file:origin/main(bare)unknown commandtoerror: device offline, so the code becomesCOMMAND_FAILEDinstead ofUNSUPPORTED_OPERATION(settings.test.ts)assert.rejectsaccepts the wrong failure+ 'COMMAND_FAILED'- 'UNSUPPORTED_OPERATION'— 1 failed | 14 passed (15)session-device-claims.test.ts)assert.rejectsaccepts itThe validation function is expected to return "true". Received false/Error: completely unrelated failure— 1 failed | 8 passed (9)Restored → green
All mutations reverted (
git statusclean); the full touched set re-run:Follow-up surfaced
src/utils/app-log-files.ts:43andsrc/utils/verified-file.ts:35,110throw plainErrorinstead ofAppError, which is whyapp-log.test.ts:39had to become a message match instead of a code assertion. This is a typed-error-rule gap in product code, out of scope for this PR. Filing as its own issue, referencing #1781 B4.Test plan
npx vitest runon all 10 touched test files — 153 tests passedorigin/main, red herepnpm run typecheck— cleannpx oxlint --deny-warningson touched files — cleanCI note
Analyze (java-kotlin)(GitHub's CodeQL default-setup scan) is red on an unrelated infra failure: the SARIF-upload step hitsHTTP 503: No server is currently availablewhile POSTing results, the same transient outage that hitBundle Sizeearlier (which has since passed on rerun). This PR touches zero Java/Kotlin files. It's a GitHub-managed "dynamic" default-setup scan (not an in-repo workflow), sogh run reruncan't retry it — GitHub reruns it on the next push automatically. It is also not a required check:gh api repos/callstack/agent-device/rules/branches/mainshows norequired_status_checksrule formain. Safe to ignore / merge past.Re-checked on the current run (
32049027824, job95443646924): same failure mode, unchanged —##[error]No server is currently available to service your requestat the upload step, preceded by the buildless-extraction jar fetch warning. Every other check on the PR is green.