feat(no-action-timeout): disallow per-call timeouts on actions - #492
Open
unlikelyzero wants to merge 2 commits into
Open
feat(no-action-timeout): disallow per-call timeouts on actions#492unlikelyzero wants to merge 2 commits into
unlikelyzero wants to merge 2 commits into
Conversation
Playwright actions auto-wait, so a `timeout` option on click, fill, check,
and friends overrides the suite's action timeout for a single interaction.
That hides the underlying application problem, fragments timing policy away
from `use.actionTimeout`, and in the `{ timeout: 0 }` case removes the bound
entirely.
The rule reports a `timeout` property in the options object of the twenty
Locator/Page action methods that accept one, regardless of how the value is
written — the objection is the per-call override, not the literal. Waits,
navigations, assertions, queries, and test-level budgets are untouched.
An `allow` option exempts named methods for suites with one legitimately
long-running interaction, such as large uploads via setInputFiles. The rule
is not in the recommended config, since enabling it by default would break
most existing suites.
unlikelyzero
force-pushed
the
feat/no-action-timeout
branch
from
August 13, 2026 22:50
1d9a13e to
1e3b9a1
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.
Closes #491
Adds
no-action-timeout, which reports atimeoutoption passed to a Playwright action method.Playwright actions auto-wait —
click(),fill(), andcheck()already retry actionability checks until the element is ready or the action timeout expires, and that budget is configurable suite-wide viause.actionTimeout. A per-calltimeoutoverrides that for exactly one interaction, which hides the application problem that motivated it, fragments timing policy across spec files, and in the{ timeout: 0 }case removes the bound entirely so a broken test hangs until a much later timeout kills the run.Behavior
Reported — the twenty Locator/Page action methods that accept a
timeoutoption:blur,check,clear,click,dblclick,dispatchEvent,dragAndDrop,dragTo,fill,focus,hover,press,pressSequentially,selectOption,selectText,setChecked,setInputFiles,tap,type,uncheckAny value is reported, not only numeric literals. Naming the constant would not address the objection, since the problem is the override rather than how it is written.
Not reported, because the timeout belongs to something other than an interaction:
Options
allow: string[](default[]) — action methods permitted to specify atimeout, for suites with one legitimately long-running interaction:{ "playwright/no-action-timeout": ["error", { "allow": ["setInputFiles"] }] }Config
Not added to
recommended. Per-call action timeouts are common in existing suites, so enabling this by default would be a breaking change for most consumers — opt-in first, and it can move intorecommendedin a future major if that seems right.Guidance in the docs
Trying this against a mature suite turned up one violation, and fixing it needed no config change at all: the
waitFor()directly above had already established visibility, so the override was pure redundancy. That's worth saying out loud, because it inverts the obvious advice — the most common legitimate-looking case is often already redundant, and the fix is deletion rather than migration touse.actionTimeout.The docs now order the remedies accordingly:
waitFor()or web-first assertion above already established the condition the timeout was guarding.test.setTimeout()/test.slow(), keeping the budget in one visible place.actionTimeout, carefully — with an explicit warning that it is a bigger hammer than it looks, since it caps every action in the suite and slows down the failure of all of them.There's also a new section on deliberately short timeouts. The argument for this rule is strongest for values longer than the configured default; a short one is the opposite of the ratchet problem, because it makes a test stricter and faster to fail. That's a real pattern when probing for optional UI. A rule can't know statically whether a literal exceeds the configured default, so I haven't added an option for it — but the docs acknowledge the case and show the
isVisible()-then-act alternative, which should pre-empt the obvious objection in review.Notes
Detection follows the existing
no-force-optionapproach: match the method name on aMemberExpressioncallee, then look for atimeoutproperty in the trailing options object. Computed keys ({ ["timeout"]: 5000 },{ [`timeout`]: 5000 }) are handled viagetStringValue, and the report is anchored on the property itself rather than the whole call.Full test suite passes (3370 tests), along with lint and
tsc --noEmit.