Bidi Pass some network tests #3069
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables passing several network-related tests for the WebDriver BiDi implementation by fixing event handling issues and adjusting test expectations for known BiDi limitations.
- Refactored
SyncBrowsingContextsAsyncto use proper async/await and event handler cleanup - Added "once" behavior emulation to prevent duplicate Success/Error event firings in request handling
- Updated network tests to filter out favicon requests that can interfere with test assertions
- Added specific test expectations for known BiDi limitations (missing RemoteAddress, Firefox-specific issues)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/PuppeteerSharp/Bidi/Core/Browser.cs |
Converted SyncBrowsingContextsAsync to proper async method with try/finally event handler cleanup |
lib/PuppeteerSharp/Bidi/BidiHttpRequest.cs |
Added boolean flags to prevent duplicate Success/Error event firings during redirects |
lib/PuppeteerSharp/Bidi/BidiFrame.cs |
Added boolean flags to prevent duplicate Success/Error event firings for frame requests |
lib/PuppeteerSharp.Tests/NetworkTests/NetworkEventTests.cs |
Added favicon filtering to prevent spurious requests from affecting test assertions; removed debug console output |
lib/PuppeteerSharp.Nunit/TestExpectations/TestExpectations.local.json |
Removed blanket network test FAIL expectation; added specific FAIL expectations for known BiDi limitations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var successFired = false; | ||
| var errorFired = false; | ||
|
|
||
| request.Success += (_, _) => | ||
| { | ||
| // Emulate 'once' behavior - only fire once | ||
| if (successFired) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| successFired = true; |
There was a problem hiding this comment.
The boolean flags successFired and errorFired are not thread-safe. If the event handlers fire concurrently from different threads, both could read false before either sets it to true, causing the event to fire multiple times. Consider using Interlocked.CompareExchange or a thread-safe flag to ensure the "once" behavior is maintained in multi-threaded scenarios.
| var successFired = false; | ||
| var errorFired = false; | ||
|
|
||
| args.Request.Success += (o, eventArgs) => | ||
| { | ||
| // Emulate 'once' behavior - only fire once | ||
| if (successFired) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| successFired = true; |
There was a problem hiding this comment.
The boolean flags successFired and errorFired are not thread-safe. If the event handlers fire concurrently from different threads, both could read false before either sets it to true, causing the event to fire multiple times. Consider using Interlocked.CompareExchange or a thread-safe flag to ensure the "once" behavior is maintained in multi-threaded scenarios.
| // Emulate 'once' behavior - only fire once | ||
| if (errorFired) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| errorFired = true; |
There was a problem hiding this comment.
The boolean flag errorFired is not thread-safe. If the error event handler fires concurrently from different threads, both could read false before either sets it to true, causing the event to fire multiple times. Consider using Interlocked.CompareExchange or a thread-safe flag to ensure the "once" behavior is maintained in multi-threaded scenarios.
| // Emulate 'once' behavior - only fire once | ||
| if (errorFired) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| errorFired = true; |
There was a problem hiding this comment.
The boolean flag errorFired is not thread-safe. If the error event handler fires concurrently from different threads, both could read false before either sets it to true, causing the event to fire multiple times. Consider using Interlocked.CompareExchange or a thread-safe flag to ensure the "once" behavior is maintained in multi-threaded scenarios.
No description provided.