[dotnet] [bidi] Add browsing context screencast support - #17941
Open
nvborisenko wants to merge 3 commits into
Open
[dotnet] [bidi] Add browsing context screencast support#17941nvborisenko wants to merge 3 commits into
nvborisenko wants to merge 3 commits into
Conversation
Contributor
PR Summary by QodoAdd BiDi BrowsingContext screencast APIs for .NET
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Member
Author
|
Passing locally. So awaiting new FF version. |
Member
Author
|
Awaiting #17923 (FF 154), then it should pass. |
nvborisenko
commented
Aug 25, 2026
Comment on lines
+367
to
+369
| if (Path.Exists(stopScreencastResult?.Path)) | ||
| { | ||
| File.Delete(stopScreencastResult.Path); |
Contributor
There was a problem hiding this comment.
1. Screencast cleanup leaks on failure 📘 Rule violation ☼ Reliability
The test records the screencast path only after StopAsync() completes, so an assertion failure after startup or an exception during stopping leaves the screencast handle unavailable and only a path eligible for deletion. The active screencast and its artifact can therefore remain, leaking session resources and contaminating later test runs or the browser process.
Agent Prompt
## Issue description
The `finally` block deletes only `stopScreencastResult?.Path`, and the stop result is assigned only after `StopAsync()` completes. If startup succeeds but a later assertion fails or `StopAsync()` throws, the screencast handle is unavailable, no stop command is sent, and the screencast and its output artifact can remain active and leak resources into subsequent tests or the browser process.
## Issue Context
The screencast path is returned by `StartScreencastAsync` at lines 355-358, while the current cleanup depends on successful completion of `StopAsync()` at line 360. Keep the `Screencast` returned by `StartScreencastAsync` in a variable outside the `try` block; in `finally`, if that handle exists and stopping has not completed, attempt to stop it before deleting the resulting file. Cleanup should handle failures during both stopping and assertions without masking the original test failure.
## Fix Focus Areas
- dotnet/test/webdriver/BiDi/BrowsingContext/BrowsingContextTests.cs[353-371]
- dotnet/test/webdriver/BiDi/BrowsingContext/BrowsingContextTests.cs[351-371]
- dotnet/src/webdriver/BiDi/BrowsingContext/Screencast.cs[42-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Relying on start screencast. Fixed.
Contributor
|
Code review by qodo was updated up to the latest commit de4ef46 |
Contributor
|
Code review by qodo was updated up to the latest commit e3ca8bc |
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
2 participants
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.
StartSceencastStopScreencast💥 What does this PR do?
This pull request adds support for screencast functionality to the BiDi BrowsingContext in the .NET WebDriver. It introduces new APIs to start and stop screencasts, defines the necessary data structures, and includes a test to verify the new feature. The most important changes are grouped below:
Screencast Feature Implementation:
StartScreencastAsyncandStopScreencastAsyncto theBrowsingContextclass, enabling clients to start and stop screencasts on a browsing context.Screencasttype and related result/options/parameter types in new files:Screencast.cs,StartScreencast.cs, andStopScreencast.cs. These encapsulate screencast session management and options. [1] [2] [3]browsingContext.startScreencast,browsingContext.stopScreencast) and their parameters/results inBrowsingContextModule, and implemented the corresponding async methods. [1] [2] [3]IBrowsingContextModuleinterface to include the new screencast methods.Testing:
CanStartAndStopScreencastto verify the screencast start and stop functionality, currently ignored for Chrome and Edge due to lack of support.🔧 Implementation Notes
Following existing pattern.
🔄 Types of changes