diff --git a/CLAUDE.md b/CLAUDE.md index eb1bd94b9..050090100 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,6 +28,14 @@ grep "^ Failed" /tmp/test-results.txt # list failures tail -5 /tmp/test-results.txt # summary ``` +Run a single class with `--filter "FullyQualifiedName~PageWaitForUrlTests"`. Every `dotnet test` invocation is a fresh test host, so run a timing-sensitive repro a few times, on `main` and on the fix. + +Test classes in `src/Playwright.Tests` mirror the upstream specs by name (`PageWaitForUrlTests` ↔ `tests/page/page-wait-for-url.spec.ts` in microsoft/playwright). Add a regression test to the matching class as a `[PlaywrightTest]` method; `PageTestEx` gives you `Page` and a local `Server` (`Server.SetRoute`, `Server.Prefix`, `Server.EmptyPage`). + +The test adapters are not covered by `Playwright.Tests`. They are tested from the Node harness in `src/Playwright.TestingHarnessTest`: `npm ci`, then `TEST_MODE=nunit npx playwright test nunit.spec.ts` (`TEST_MODE` is one of `nunit`, `mstest`, `mstest.v4`, `xunit`, `xunit.v3`). + +CI's style check is `dotnet format ./src/ --verify-no-changes`. + ## Architecture ### Generated vs hand-written code @@ -36,12 +44,39 @@ tail -5 /tmp/test-results.txt # summary - The generator skips generating interface files for types like `TimeoutException` and `IAsyncDisposable` that map to built-in .NET types. - Supplement interfaces (`src/Playwright/API/Supplements/`) are hand-written and extend the generated interfaces with .NET-specific overloads. - Internal implementations live in `src/Playwright/Core/` (namespace `Microsoft.Playwright.Core`). These implement both the generated and supplement interfaces. +- Protocol types in `src/Playwright/Transport/Protocol/Generated/` are generated by `../playwright/utils/generate_dotnet_channels.js`. Both generators run as part of `./build.sh --roll `; a wrong doc comment, option name or overload in the .NET API is an upstream change to `docs/src/api/` or the generator and reaches this repo with the next roll. + +### The client mirrors the JS client +- `src/Playwright/Core/*.cs` is a port of `packages/playwright-core/src/client/*.ts` in microsoft/playwright, method for method (`Frame.cs` ↔ `frame.ts`, `Waiter.cs` ↔ `waiter.ts`, `Connection.cs` ↔ `connection.ts`). Keep the same method boundaries, order of steps and log messages: reviews compare against the JS original, and rolls port upstream client commits one by one. +- Before restructuring a method, read its JS counterpart. If JS does the same thing, the change is compensating for something the .NET runtime does differently (usually threading, below); keep the deviation as small as the method allows and leave a comment naming the reason. +- If the same calls misbehave in the JS client, fix it upstream first. + +### Threading model +The JS client is single-threaded and the other ports keep that model. .NET does not: the library and the user's code run concurrently, which is where "lost event" and "collection was modified" reports come from (#2719, #3207, #3364). + +- Incoming messages are handled one at a time, in protocol order. `StdIOTransport.GetResponseAsync` reads the driver's stdout and calls `Connection.Dispatch` for each message; `Dispatch` runs `ChannelOwner.OnMessage` synchronously, so `Frame.OnNavigated`/`OnLoadState`, the `Url` and `_loadStates` updates and every event handler user code attached (`page.FrameNavigated += ...`) have finished before the next message is looked at. There is no fixed reader thread: the read loop is started as a long-running task but continues on whatever thread-pool thread completes each pipe read, so "the reader thread" below is a role, not a thread id. +- Replies to commands complete a `TaskCompletionSource` created with `RunContinuationsAsynchronously` (`Connection.cs`), which queues the awaiting code to the thread pool. Everything the reader did for messages *before* the reply is visible when `await page.GotoAsync(...)` returns: an event that precedes the reply in the protocol can never fire after the method has returned, and `GotoAsync` can rely on `_loadStates` containing `load`. Events that *follow* the reply race with the continuation. JS guarantees that a continuation runs before the next message is dispatched; .NET does not. +- Waits that complete through an event resume *inline on the reader thread*: `Waiter.GetWaitForEventTask` completes a plain `TaskCompletionSource`, so after `await page.WaitForLoadStateAsync()`, `WaitForNavigationAsync`, `WaitForURLAsync`, `WaitForEventAsync` or `RunAndWaitFor*Async` (when the event has not already happened) code runs inside `Connection.Dispatch` until its next incomplete `await`. Blocking there (`.Result`, `.Wait()`) stalls every later message, including the reply being waited for. It also inverts an ordering JS has: `Frame.OnLoadState` raises `Frame.LoadState` before `Page.Load`, so right after `await page.WaitForLoadStateAsync()` a `page.Load` handler has not run yet. +- A few messages are handed off on purpose and run concurrently with later messages: `route` and `webSocketRoute` (`_ = OnRouteAsync(...)`), `bindingCall`, locator handlers and screencast frames. Their handlers start on the reader thread and move to the pool at their first incomplete `await`; they have to call back into the server, so they cannot run inline. +- Outgoing messages are serialized through `Connection._queue` (a `TaskQueue` over a `SemaphoreSlim`). Incoming messages are not synchronized with user code. +- Sharing one `IPlaywright` across threads is not supported (#2560); use one instance per thread. + +Consequences for changes: +- "Check state, then subscribe to the event" is correct in JS and a race here: the reader thread can deliver the event between the check and the subscription, and the wait times out (#3364). Subscribe first, then check, and unsubscribe when the check already passes. +- A method may rely on state produced by events that precede its reply in the protocol, never on events that arrive after it. Library code that runs after an event-driven wait may be on the reader thread: it must not block, and must not assume that later events of the same dispatch (`Page.Load` after `Frame.LoadState`) have fired. +- Do not add `lock`. The client has none. For state the reader thread mutates and user code reads, take a snapshot (`_frames.ToList()` in #2724, `_contextsForSelectors.ToArray()` in #3208) or use a concurrent collection (`ConcurrentDictionary` in `RouteHandler`, #2887). +- Adding or removing an event handler can send `updateSubscription` to the server: `ChannelOwner.UpdateEventHandler` does so for the first and last handler of `console`, `dialog`, `fileChooser`, `request`, `response`, `requestFinished` and `requestFailed`. `Waiter` subscribes through reflection (`GetEvent` + `AddEventHandler`). ### Key patterns - All Playwright objects extend `ChannelOwner` and communicate via `SendMessageToServerAsync`. - `Connection.cs` has a factory switch that creates the right `ChannelOwner` subclass based on `ChannelOwnerType`. - New channel object types require: enum entry in `ChannelOwnerType.cs`, case in `Connection.cs`, initializer in `Transport/Protocol/Generated/`, and a `Core/` class. - Public APIs should use .NET standard types (e.g. `IAsyncDisposable`) not custom Playwright types. Internal helpers (e.g. `Disposable` class in `Core/`) stay internal. +- Public API methods in `Core/` carry `[MethodImpl(MethodImplOptions.NoInlining)]`. `Connection.WrapApiCallAsync` walks the stack once per outermost API call to attach the user's frame to call logs and traces, and inlining breaks that walk (#2620). Keep the attribute on new public methods. + +### Test adapters +- `Playwright.NUnit`, `Playwright.MSTest`, `Playwright.MSTest.v4`, `Playwright.Xunit` and `Playwright.Xunit.v3` each repeat the same hierarchy: `WorkerAwareTest` → `PlaywrightTest` → `BrowserTest` → `ContextTest` → `PageTest`. A change to one usually belongs in all five. +- Settings come from `.runsettings` through `Playwright.TestAdapter`: `PlaywrightSettingsProvider` is a VSTest `ISettingsProvider` for the `` section, and `PlaywrightSettingsXml` maps the XML onto the option classes by reflection. Microsoft.Testing.Platform does not run VSTest settings providers, which is behind most "runsettings are ignored" reports. ## Commit Convention