[dotnet] [test] Introduce fake BiDi transport

[nvborisenko]

And use it for some testing scenarios. Might be not elegant api, but it is OK for now.

Also changed bidi options to factory instead of provided instance. It resolves ownership problems.

💥 What does this PR do?

This pull request refactors and improves the BiDi (Bidirectional) WebDriver test suite by introducing a new in-process fake transport for more deterministic and isolated unit testing. It also moves several tests from integration-style tests to new unit tests, and updates the transport configuration API for greater flexibility.

Testing improvements

• Added a new FakeTransport class that implements ITransport for in-process, controllable, and deterministic unit testing of BiDi functionality without requiring a real browser or network connection. (dotnet/test/webdriver/BiDi/FakeTransport.cs)
• Moved and expanded several tests from SessionTests to a new SessionUnitTests class, now using FakeTransport to test command timeouts, cancellation, disposal, error handling, and command ID sequencing in isolation. (dotnet/test/webdriver/BiDi/SessionUnitTests.cs)
• Removed timeout and cancellation token tests from SessionTests, as these are now covered in the new unit tests. (dotnet/test/webdriver/BiDi/Session/SessionTests.cs)
• Removed the idempotent disposal test from SessionTests, as it is now covered in the unit tests. (dotnet/test/webdriver/BiDi/Session/SessionTests.cs)

API improvements

• Changed the UseTransport method in BiDiOptionsBuilder to accept a factory function for creating ITransport instances, improving flexibility and ensuring correct ownership and disposal of transports. (dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs)

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated:
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Cleanup (formatting, renaming)

[qodo-code-review]

Review Summary by Qodo

Introduce fake BiDi transport and refactor to factory-based API

✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Introduce FakeTransport for deterministic unit testing without real browser
• Change UseTransport API to accept factory function instead of instance
• Move timeout, cancellation, and disposal tests to new SessionUnitTests
• Add comprehensive unit tests for command sequencing and error handling

Diagram

flowchart LR
  A["BiDiOptionsBuilder"] -->|"UseTransport factory"| B["ITransport instance"]
  C["FakeTransport"] -->|"implements"| B
  D["SessionTests"] -->|"moves tests to"| E["SessionUnitTests"]
  E -->|"uses"| C
  F["Unit tests"] -->|"test timeout/cancellation"| E

Loading

File Changes

1. dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs ✨ Enhancement +8/-5

Change UseTransport to accept factory function

• Changed UseTransport method signature from accepting ITransport instance to Func factory
• Updated documentation to clarify BiDi takes ownership of transport and will dispose it
• Factory function is invoked to create transport instance, enabling proper ownership management

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs

---

2. dotnet/test/webdriver/BiDi/FakeTransport.cs 🧪 Tests +126/-0

Add FakeTransport for controllable unit testing

• New in-process ITransport implementation for deterministic unit testing
• Provides SentMessages list to capture outgoing BiDi commands
• Includes helper methods EnqueueSuccess, EnqueueError, EnqueueEvent for scripting responses
• Implements WaitForSentMessagesAsync to wait for commands with timeout
• Uses Channel for blocking receive behavior to enable timeout/cancellation testing

dotnet/test/webdriver/BiDi/FakeTransport.cs

---

3. dotnet/test/webdriver/BiDi/Session/SessionTests.cs 🧪 Tests +0/-25

Remove unit tests moved to SessionUnitTests

• Removed ShouldHaveIdempotentDisposal test (moved to unit tests)
• Removed ShouldRespectTimeout test (moved to unit tests)
• Removed ShouldRespectCancellationToken test (moved to unit tests)
• Kept integration test CanGetStatus that requires real browser connection

dotnet/test/webdriver/BiDi/Session/SessionTests.cs

---

View more (1)

4. dotnet/test/webdriver/BiDi/SessionUnitTests.cs 🧪 Tests +115/-0

Add comprehensive unit tests with FakeTransport

• New unit test class using FakeTransport for isolated testing
• Tests command timeout, cancellation token, and idempotent disposal
• Tests correct method name in status command and error response handling
• Tests sequential command ID generation across multiple commands
• Configured for parallel execution with instance-per-test lifecycle

dotnet/test/webdriver/BiDi/SessionUnitTests.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Null transport factory result ✓ Resolved 🐞 Bug ≡ Correctness

Description

BiDiOptionsBuilder.UseTransport(Func<ITransport>) does not validate that the factory returns a
non-null ITransport, so a null return will flow into BiDi.ConnectAsync and crash later when Broker
calls SendAsync/ReceiveAsync on the transport.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R56-63]

+    public BiDiOptionsBuilder UseTransport(Func<ITransport> factory)
    {
-        ArgumentNullException.ThrowIfNull(transport);
+        ArgumentNullException.ThrowIfNull(factory);

        return UseTransport((_, ct) => ct.IsCancellationRequested
            ? Task.FromCanceled<ITransport>(ct)
-            : Task.FromResult(transport));
+            : Task.FromResult(factory()));
    }

Evidence

The new overload returns Task.FromResult(factory()) without checking for null, and the resulting
transport is used without null checks by BiDi/Broker.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[48-69]
dotnet/src/webdriver/BiDi/BiDi.cs[56-73]
dotnet/src/webdriver/BiDi/Broker.cs[58-66]
dotnet/src/webdriver/BiDi/Broker.cs[125-131]
dotnet/src/webdriver/BiDi/Broker.cs[315-327]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`UseTransport(Func<ITransport>)` validates the `factory` delegate itself, but not the object it returns. If `factory()` returns `null`, `BiDi.ConnectAsync` will pass it to `Broker`, leading to a `NullReferenceException` at runtime.

### Issue Context
`BiDi.ConnectAsync` awaits `TransportFactory` and immediately constructs `Broker(transport, bidi)`.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[56-63]

### Suggested fix
Update the wrapper delegate to validate the result:
- Call `var transport = factory();`
- `ArgumentNullException.ThrowIfNull(transport);`
- Return `Task.FromResult(transport)`

Optionally, wrap exceptions from `factory()` into a faulted `Task<ITransport>` for consistency with async factory patterns.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. UseTransport signature breaking change 📘 Rule violation ⚙ Maintainability

Description

BiDiOptionsBuilder.UseTransport now requires a Func<ITransport> instead of an ITransport,
which will break downstream callers that pass an existing transport instance. The removed API is not
kept as an overload nor deprecated, so consumers have no guided migration path.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R49-63]

+    /// Configures the BiDi connection to use a transport created by the specified factory.
    /// </summary>
-    /// <param name="transport">The transport instance to use.</param>
+    /// <remarks>
+    /// BiDi takes ownership of the transport instance returned by the factory and will dispose it.
+    /// </remarks>
+    /// <param name="factory">A factory function that creates the <see cref="ITransport"/> instance.</param>
    /// <returns>The current <see cref="BiDiOptionsBuilder"/> instance for chaining.</returns>
-    public BiDiOptionsBuilder UseTransport(ITransport transport)
+    public BiDiOptionsBuilder UseTransport(Func<ITransport> factory)
    {
-        ArgumentNullException.ThrowIfNull(transport);
+        ArgumentNullException.ThrowIfNull(factory);

        return UseTransport((_, ct) => ct.IsCancellationRequested
            ? Task.FromCanceled<ITransport>(ct)
-            : Task.FromResult(transport));
+            : Task.FromResult(factory()));
    }

Evidence

The compliance checklist requires public API changes to be backward-compatible and, if removal is
necessary, to be deprecated first with a clear alternative. The updated public method signature
shows the API now only accepts Func<ITransport>, which is not source-compatible with the previous
ITransport-parameter shape and has no deprecation mechanism shown in the updated code.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
AGENTS.md: Deprecate Before Removing Public Functionality and Provide Alternatives
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-63]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A public API breaking change was introduced by changing `BiDiOptionsBuilder.UseTransport` from accepting an `ITransport` instance to accepting only a `Func<ITransport>` factory.

## Issue Context
This breaks existing consumer code at compile time and provides no deprecation/migration period.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[49-63]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. UseTransport factory ignores cancellation 📘 Rule violation ☼ Reliability

Description

UseTransport(Func<ITransport>) only checks ct.IsCancellationRequested before calling
factory(), but cannot pass cancellation/timeout into transport creation, so a blocking factory
(e.g., network connect) can hang ConnectAsync indefinitely. This reduces deterministic
cancellation behavior for async connection setup.

Code

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[R60-70]

+        return UseTransport((_, ct) =>
+        {
+            if (ct.IsCancellationRequested)
+            {
+                return Task.FromCanceled<ITransport>(ct);
+            }
+
+            var transport = factory() ?? throw new InvalidOperationException("The transport factory must return a non-null ITransport instance.");

-        return UseTransport((_, ct) => ct.IsCancellationRequested
-            ? Task.FromCanceled<ITransport>(ct)
-            : Task.FromResult(transport));
+            return Task.FromResult(transport);
+        });

Evidence

PR Compliance ID 14 requires async waits/network operations to have explicit cancellation/timeout
paths. The new implementation calls factory() without any way to propagate ct, meaning transport
creation cannot be canceled once started.

dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[60-70]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`UseTransport(Func<ITransport>)` wraps a synchronous factory and cannot pass the `CancellationToken` into transport creation. If the factory performs blocking work (like establishing a connection), `BiDi.ConnectAsync(..., cancellationToken)` may not be able to cancel promptly.

## Issue Context
`ConnectAsync` calls `builder.TransportFactory(uri, cancellationToken)`. The `UseTransport(Func<ITransport>)` overload currently only short-circuits when cancellation is already requested, but does not support cancellation during transport creation.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[60-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Enqueue silently ignores failure 🐞 Bug ☼ Reliability

Description

FakeTransport.Enqueue ignores the boolean result of ChannelWriter.TryWrite, so if the channel is
completed (e.g., after disposal) the response is silently dropped and tests can hang/time out
waiting for a message that will never arrive.

Code

dotnet/test/webdriver/BiDi/FakeTransport.cs[R73-76]

+    /// <summary>Enqueues a raw JSON string to be returned by the next <see cref="ReceiveAsync"/> call.</summary>
+    public void Enqueue(string json)
+        => _incoming.Writer.TryWrite(json);
+

Evidence

If TryWrite fails (writer completed), ReceiveAsync will keep waiting for messages; because Enqueue
discards the failure, the caller has no signal that the response will never arrive.

dotnet/test/webdriver/BiDi/FakeTransport.cs[73-76]
dotnet/test/webdriver/BiDi/FakeTransport.cs[108-113]
dotnet/test/webdriver/BiDi/FakeTransport.cs[121-125]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Enqueue()` calls `TryWrite()` and ignores the return value. For an unbounded channel this mainly fails when the writer is completed, but when it does fail it causes very confusing behavior: tests think they queued a response, but `ReceiveAsync` continues blocking.

### Issue Context
`ReceiveAsync` blocks on `_incoming.Reader.ReadAsync(...)` until a message is available.

### Fix Focus Areas
- dotnet/test/webdriver/BiDi/FakeTransport.cs[73-76]
- dotnet/test/webdriver/BiDi/FakeTransport.cs[108-113]

### Suggested fix
Make `Enqueue` fail loudly:
- If `TryWrite(json)` returns `false`, throw `InvalidOperationException` with a clear message (e.g., "FakeTransport is completed/disposed").

Optionally add an `IsCompleted` flag set during `DisposeAsync` to improve error messaging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. FakeTransport SentMessages races 🐞 Bug ☼ Reliability

Description

FakeTransport stores sent messages in a List<string> that is mutated in SendAsync and
enumerated/snapshotted in WaitForSentMessagesAsync without synchronization, so concurrent commands
can corrupt the list or throw during enumeration.

Code

dotnet/test/webdriver/BiDi/FakeTransport.cs[R58-71]

+    public async Task<IReadOnlyList<string>> WaitForSentMessagesAsync(
+        int count = 1,
+        CancellationToken cancellationToken = default)
+    {
+        using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+        cts.CancelAfter(TimeSpan.FromSeconds(5));
+
+        while (SentMessages.Count < count)
+        {
+            await Task.Delay(5, cts.Token).ConfigureAwait(false);
+        }
+
+        return [.. SentMessages];
+    }

Evidence

Broker is written to be concurrency-friendly and can call SendAsync concurrently; FakeTransport uses
an unsynchronized List plus snapshot enumeration ([.. SentMessages]), which is unsafe under
concurrent Add/enumeration.

dotnet/test/webdriver/BiDi/FakeTransport.cs[51-71]
dotnet/test/webdriver/BiDi/FakeTransport.cs[115-119]
dotnet/src/webdriver/BiDi/Broker.cs[42-47]
dotnet/src/webdriver/BiDi/Broker.cs[77-87]
dotnet/src/webdriver/BiDi/Broker.cs[125-131]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SentMessages` is a mutable `List<string>` accessed from potentially multiple concurrent command executions (multiple `SendAsync` calls) and from test code (count checks + snapshot enumeration). `List<T>` is not thread-safe and can throw or become corrupted under concurrent access.

### Issue Context
`Broker.ExecuteAsync` supports concurrent use (uses `ConcurrentDictionary` and `Interlocked.Increment`) and calls `_transport.SendAsync(...)` per command.

### Fix Focus Areas
- dotnet/test/webdriver/BiDi/FakeTransport.cs[51-71]
- dotnet/test/webdriver/BiDi/FakeTransport.cs[115-119]

### Suggested fix
Use a thread-safe collection or locking:
- Replace `List<string>` with `ConcurrentQueue<string>` (or `ConcurrentBag<string>`) and expose snapshots via `ToArray()`.
- Or keep `List<string>` but guard *all* reads/writes with a private `lock` and return snapshots (`ToArray()`) rather than the live list.

Also ensure `WaitForSentMessagesAsync` snapshots in a way that cannot throw due to concurrent modification.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Unit tests hardcode IDs 🐞 Bug ⚙ Maintainability

Description

SessionUnitTests enqueue responses with hard-coded command IDs (1, 2) instead of using
FakeTransport.LastSentCommandId(), making the tests brittle if BiDi ever sends connect-time commands
or changes command-id initialization.

Code

dotnet/test/webdriver/BiDi/SessionUnitTests.cs[R69-75]

+    [Test]
+    public async Task StatusCommandSendsCorrectMethod()
+    {
+        var task = _bidi.StatusAsync();
+        await _transport.WaitForSentMessagesAsync(1);
+        _transport.EnqueueSuccess(1, """{"ready":true,"message":"running"}""");
+

Evidence

FakeTransport provides a helper to derive IDs from actual sent messages, but the tests reply with
fixed IDs in multiple places.

dotnet/test/webdriver/BiDi/FakeTransport.cs[97-106]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[69-82]
dotnet/test/webdriver/BiDi/SessionUnitTests.cs[97-109]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The tests assume the first two commands will have IDs 1 and 2. That is an implementation detail that can change (e.g., if BiDi begins sending a handshake command during `ConnectAsync`).

### Issue Context
`FakeTransport` already provides `LastSentCommandId()` to build matching responses.

### Fix Focus Areas
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[69-82]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[97-114]

### Suggested fix
Replace hard-coded IDs with values parsed from sent messages:
- After `await _transport.WaitForSentMessagesAsync(...)`, call `var id = _transport.LastSentCommandId();`
- Use `EnqueueSuccess(id, ...)` / `EnqueueError(id, ...)`.

For sequential-ID assertions, parse IDs from the captured sent messages (as you already do) but enqueue responses using the parsed IDs rather than assuming 1/2.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1a5c307