Add C# example for unhandledPromptBehavior option#2724
Conversation
PR Summary by QodoAdd C# unhandledPromptBehavior example and wire docs snippet
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
✅ Deploy Preview for selenium-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
10 rules 1. Edge docs #L30-L31 stale
|
On Windows, msedgedriver can still hold its log file open for a moment after driver.Quit(), so reading the log right after Quit() intermittently threw IOException in LogsToFile, LogsLevel, ConfigureDriverLogs, and DisableBuildCheck. Mirrors the Python and Ruby examples, which read the log while the service is still running instead of after quitting (see 4decac3 for the equivalent Python fix). Also guards the log file deletion in Cleanup() against the same race, tolerating IOException like Python's contextlib.suppress(OSError) and Ruby's FileUtils.rm_f. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 1623d5c |
CI showed the log file is locked for the entire life of the Edge driver service on Windows, not just briefly after Quit() -- moving the read before Quit() (previous commit) failed immediately since the driver process was still running. Reading after Quit() is correct, but the OS still needs a moment to release the handle once the process exits, so wrap the read in a short bounded retry instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 897e537 |
If driver creation throws before the field is assigned, Cleanup()'s unconditional driver.Quit() throws NullReferenceException and masks the real test failure. ChromeTest.cs already uses this driver?.Quit() pattern in its own cleanup; apply it here too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 7831356 |
The previous 2s retry budget (10 x 200ms) wasn't enough -- CI still failed with the same IOException after exhausting all attempts. EdgeDriverService.LogPath is fed by DriverService's async OutputDataReceived event handler (per the Selenium.WebDriver XML docs), so the log FileStream can stay open for several seconds after Quit() returns while the last buffered output drains. Widen the budget to ~9.5s (20 x 500ms) to reliably outlast that tail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit d49f495 |
ReadLogLines was retrying on any IOException, so a genuine failure (missing file, bad path, permissions) would silently eat up to ~10s of sleeping before surfacing, slowing down and obscuring real failures. Restrict the retry to the specific Win32 sharing/lock violation HRESULTs the Windows file-lock race actually produces; everything else propagates immediately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 0ff5613 |
Drop the retry loop and try forcing a real synchronous teardown instead: EdgeDriverService.LogPath is fed by DriverService's async OutputDataReceived handler, so driver.Quit() alone may return before that stream is fully flushed and closed. Holding the service reference and calling service.Dispose() explicitly should block until the log file is actually released, letting us read it with a plain File.ReadAllLines and no polling. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| var lines = File.ReadLines(GetLogLocation()); | ||
| driver.Quit(); | ||
| service.Dispose(); // Force the service log file to fully close before reading | ||
| var lines = File.ReadAllLines(GetLogLocation()); |
There was a problem hiding this comment.
1. Eager log reads 🐞 Bug ➹ Performance
EdgeTest now uses File.ReadAllLines() to read driver logs even though each test only needs to find a single matching line, forcing unnecessary full-file I/O and memory usage (especially with verbose logging enabled). This can slow the test suite and scales poorly if the driver emits large logs.
Agent Prompt
## Issue description
`EdgeTest` was changed from `File.ReadLines(...)` to `File.ReadAllLines(...)` in multiple log assertions. These tests only need to locate one matching line (via `FirstOrDefault`), so reading the entire log into memory is unnecessary work.
## Issue Context
This is most relevant in `ConfigureDriverLogs`, where verbose logging is enabled and logs can be larger.
## Fix Focus Areas
- examples/dotnet/SeleniumDocs/Browsers/EdgeTest.cs[103-168]
## Suggested change
Replace `File.ReadAllLines(GetLogLocation())` with `File.ReadLines(GetLogLocation())` (or inline the search: `Assert.IsNotNull(File.ReadLines(...).FirstOrDefault(...))`) in:
- `LogsToFile`
- `LogsLevel`
- `ConfigureDriverLogs`
- `DisableBuildCheck`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 28cb0c2 |
The explicit-Dispose-only approach didn't work: CI showed the same IOException immediately after driver.Quit() + service.Dispose(), so DriverService's async OutputDataReceived teardown isn't fully synchronous even through an explicit Dispose() call. Restore the retry (narrowed to actual sharing/lock-violation HRESULTs, ~9.5s budget), and keep the Dispose() call alongside it since it's still correct resource cleanup and may shrink the typical wait. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| service.Dispose(); // Close the Service log file before reading | ||
| var lines = ReadLogLines(GetLogLocation()); |
There was a problem hiding this comment.
1. Service dispose not guaranteed 🐞 Bug ☼ Reliability
In EdgeTest log tests, EdgeDriverService.Dispose() is only called on the success path; if EdgeDriver construction or driver.Quit() throws, the service is never disposed and can leave the driver process/log file handle open. This can cause follow-on test flakiness and persistent log file locks in the suite.
Agent Prompt
### Issue description
`EdgeDriverService` is disposed only on the happy path in multiple tests. If `new EdgeDriver(service, options)` or `driver.Quit()` throws, `service.Dispose()` is skipped and may leak resources (including log file handles), leading to downstream flakiness.
### Issue Context
These tests intentionally manage the service log file lifecycle (`service.Dispose(); // Close the Service log file before reading`) and then immediately read the log file. This makes guaranteed disposal particularly important.
### Fix Focus Areas
- examples/dotnet/SeleniumDocs/Browsers/EdgeTest.cs[98-169]
### Suggested fix
Wrap each test’s driver/service lifecycle with `try/finally` (or `using` + `try/finally`) so `service.Dispose()` always runs, even if driver construction or `Quit()` fails. For example:
```csharp
var service = EdgeDriverService.CreateDefaultService();
try
{
service.LogPath = GetLogLocation();
driver = new EdgeDriver(service, options);
driver.Quit();
}
finally
{
// Ensure service is released even if constructor/Quit throws
service.Dispose();
}
var lines = ReadLogLines(GetLogLocation());
```
(Apply similarly in `LogsLevel`, `ConfigureDriverLogs`, and `DisableBuildCheck`.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 50f4678 |
This is a doc code example, not production code -- the narrowed sharing/lock-violation HResult check added unnecessary verbosity, and comments like "Close the Service log file before reading" get confusing once the file is translated. Keep the retry and service.Dispose(), drop the rest.
|
Code review by qodo was updated up to the latest commit e18a147 |
PR #2724 wired the CSharp tab's unhandledPromptBehavior example into options.en.md but missed the ja/pt-br/zh-cn translations, which still showed the placeholder badge. Apply the same gh-codeblock reference there.
Thanks for contributing to the Selenium site and documentation!
A PR well described will help maintainers to review and merge it quickly
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, and help reviewers by making them as simple and short as possible.
Description
Adds the missing C# test for the
unhandledPromptBehavioroption and points the docs at it, replacing the placeholder badge in the CSharp tab.Motivation and Context
The CSharp tab for
unhandledPromptBehaviorinoptions.en.mdshowed a placeholder badge instead of a code example, unlike Java, Python, and Ruby. This combines the code from #2611 and the doc update from #2612 (both by @kimtg) into one PR, fixing the line-range format in the doc reference along the way.Types of changes
Checklist