[docs] decision: BiDi events are awaited with expect_* context managers#17671
[docs] decision: BiDi events are awaited with expect_* context managers#17671AutomatedTester wants to merge 3 commits into
Conversation
AutomatedTester
force-pushed
the
adr-bidi-expect-events
branch
from
e708379 to
0c1288e
Compare
…le-object decision (0008)
titusfortner
left a comment
titusfortner
left a comment
There was a problem hiding this comment.
I really do not want to expand the API to include expect_* methods for everything in all bindings.
My proposal in #17685 is to allow the user to declare when an expected thing has happened inside the callable by setting a complete status. This prevents needing to coordinate external data structures and allows us to continue to separate concerns and use existing Selenium Wait implementations and external test runner assertions on results.
Rename 0001 -> 17671 and point all cross-references (to 17672/17676/17681) at their PR-number filenames.
diemol
left a comment
diemol
left a comment
There was a problem hiding this comment.
In general, I like the concept as it resembles the Playwright design, which I think is well thought out.
To be on the same page, this is not a high-level API, right? This seems to be a middle-level API that could help users address use cases not included in the high-level APIs.
Also, this ADR is addressing the observability use case, correct? If I understand it correctly, @titusfortner's ADR, is more about network interception and handling behavior when something happens, not only observing.
The way I see it, ¿could both ADRs live together?
However, the problem I see is that neither ADR references the other's internal implementation model. This is a risk because each binding could end up reinventing the same queue/wait mechanism in incompatible ways.
| @@ -0,0 +1,144 @@ | |||
| # 17671. BiDi events are awaited with `expect_*` context managers | |||
There was a problem hiding this comment.
This ARD is very Python-idiomatic, since expect_* is what Playwright is doing for Python. Could we name this ADR so that the concept applies to all language bindings and avoid keywords specific to a particular programming language?
| Playwright solved this with `with page.expect_event(...) as info: action()`, where the | ||
| listener is armed on `__enter__` (before the action) and `info.value` blocks on | ||
| `__exit__` until a matching event arrives or the timeout elapses. This is the single most | ||
| common asynchronous pattern in browser automation, and Selenium has no equivalent. |
There was a problem hiding this comment.
Is there a way to make this section more general? This is specific to Python. Below, in the section of the options considered, the pattern waitFor* is discarded, but that is the convention Playwright uses for this concept. I would like to see, if possible, all the patterns we could use for all bindings. I don't mind resembling the ones from Playwright.
| Every binding exposes an **`expect_*` family of context-manager (or block) helpers** that | ||
| arm a one-shot, predicate-filtered subscription before the user action and resolve to the | ||
| captured event after it. |
| Concrete shorthands each binding SHOULD provide (built on the generic primitive): | ||
| `expect_request`, `expect_response`, `expect_console_message`, `expect_navigation` | ||
| (see [17676](17676-navigation-awaited-with-expect-helpers.md)), `expect_user_prompt` | ||
| (see [17672](17672-user-prompts-handled-through-typed-handler-api.md)), `expect_download`, | ||
| and — for tabs/windows the page opens itself — `expect_page` / `expect_popup` over | ||
| `browsingContext.contextCreated`, returning the new context as a handle object | ||
| (see [17681](17681-browsing-contexts-exposed-as-handle-objects.md)). |
There was a problem hiding this comment.
I would prefer to define this naming for all bindings as part of this ADR. This would mean we are all on the same page. As I mentioned before, I do not mind using the same naming Playwright uses.
| - **A future/promise returned from a `waitForEvent(...)` call** — viable in async | ||
| bindings, but in synchronous bindings it reintroduces the race (the call to start | ||
| waiting happens after the action) unless wrapped in a block anyway. Rejected as the | ||
| primary shape; bindings MAY offer it as a secondary convenience where idiomatic. |
There was a problem hiding this comment.
This ADR's section is the confusing part for me. My understanding is to follow the idea from Playwright, specifically the naming they use.
| - Each binding gains a small reusable `Subscription` primitive; implementing it surfaces | ||
| and forces fixes to event-dispatch thread-safety (callback maps must be lock-guarded, | ||
| subscribe/unsubscribe I/O must not be held under a dispatch lock). |
There was a problem hiding this comment.
Would it be necessary to specify how concurrency is handled? What happens if two events that match enter at the same time? If we don't specify it, we could end up with inconsistent behavior across language binding implementations.
3 participants
💥 What does this PR do?
Proposes a design decision record: BiDi events are awaited with
expect_*context managers.Today the only way to react to a BiDi event is a fire-and-forget callback. To wait for an event triggered by a user action, users must subscribe, act, then wait — which races (the event can fire before the wait starts) and forces everyone to re-implement capture-and-filter boilerplate. This record proposes a race-free, predicate-filtered
expect_*family (arm-before-action context managers) over a small reusableSubscriptionprimitive, consistent across all five bindings, as the foundation the other event-driven decisions (navigation, downloads, user prompts) build on.🔧 Implementation Notes
docs/decisions/process and template introduced in [docs] add design decision record process and template #17665 — depends on [docs] add design decision record process and template #17665 landing (this PR adds onlydocs/decisions/0001-*.md; no conflict with the template/README files).expect_*+Subscriptionwork is implemented and a code PR is pending).🤖 AI assistance
💡 Additional Considerations
Part of a set of BiDi ergonomics decision records proposed together (events, user prompts, storage/cookies, network body+timing, emulation, navigation). This one should be considered first since the others build on the
expect_*primitive it defines. Cross-binding convergence is tracked in the record's binding-status table.🔄 Types of changes