[dotnet] Improve the debugging experience of tests

[RenderMichael]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Use the new ConfigureAwait(ConfigureAwaitOptions) method introduced in .NET 8 to avoid first-chance exceptions in the test cleanup

Motivation and Context

When debugging through code, .NET provides first-chance exceptions, allowing users to see exceptions as they are thrown, even if they are handled or suppressed.

Unrelated exceptions can get in the way when testing unrelated code. This change makes the debugger break less when it is not necessary.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

enhancement

---

Description

• Introduced a new QuitNoThrow method to handle HTTP requests in a way that suppresses exceptions during test cleanup.
• Replaced existing inline HTTP client logic with the new QuitNoThrow method in the Stop method of TestWebServer.
• Utilized ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing) to prevent first-chance exceptions from interrupting the debugging process.

---

Changes walkthrough 📝

	Relevant files
Enhancement	TestWebServer.cs Improve test cleanup by suppressing exceptions using ConfigureAwait dotnet/test/common/Environment/TestWebServer.cs Added QuitNoThrow method to handle HTTP requests without throwing exceptions. Replaced inline HTTP client logic with QuitNoThrow in the Stop method. Utilized ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing) to suppress exceptions. +15/-14

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The QuitNoThrow method may silently swallow all exceptions, not just the expected HttpRequestException. Consider adding specific exception handling. Resource Cleanup The HttpResponseMessage is only disposed if the task completes successfully. Consider adding disposal in a finally block to ensure cleanup in all cases.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Enhancement	Add cancellation support to HTTP request for better resource cleanup Use CancellationToken to properly handle cancellation of the HTTP request during shutdown. dotnet/test/common/Environment/TestWebServer.cs [203] -Task<HttpResponseMessage> getTask = httpClient.GetAsync(EnvironmentManager.Instance.UrlBuilder.LocalWhereIs("quitquitquit")); +using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); +Task<HttpResponseMessage> getTask = httpClient.GetAsync(EnvironmentManager.Instance.UrlBuilder.LocalWhereIs("quitquitquit"), cts.Token); Apply this suggestion Suggestion importance[1-10]: 9 Why: Using CancellationToken provides the most robust solution for handling request timeouts, allowing proper cleanup of resources and preventing memory leaks during server shutdown.	9
Best practice	Add timeout to HTTP request to prevent indefinite waiting during shutdown Set a reasonable timeout for the HTTP request to prevent potential hangs during server shutdown. dotnet/test/common/Environment/TestWebServer.cs [203] -Task<HttpResponseMessage> getTask = httpClient.GetAsync(EnvironmentManager.Instance.UrlBuilder.LocalWhereIs("quitquitquit")); +Task<HttpResponseMessage> getTask = httpClient.GetAsync(EnvironmentManager.Instance.UrlBuilder.LocalWhereIs("quitquitquit"), TimeSpan.FromSeconds(5)); Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding a timeout to the HTTP request is crucial for preventing potential deadlocks during server shutdown, as the current implementation could hang indefinitely if the server is unresponsive.	8
	Set default timeout for HTTP client to prevent potential deadlocks Set HttpClient timeout at instance level to ensure all requests have a timeout, not just the quit request. dotnet/test/common/Environment/TestWebServer.cs [201] -using var httpClient = new HttpClient(); +using var httpClient = new HttpClient { Timeout = TimeSpan.FromSeconds(5) }; Apply this suggestion Suggestion importance[1-10]: 7 Why: Setting a default timeout at the HttpClient level is a good practice to ensure all potential requests have a timeout, though slightly less critical since the client is only used for a single request in this context.	7

💡 Need additional feedback ? start a PR chat

[nvborisenko]

So smart for test project. I vote for keeping simplicity, rather than being technically perfect.

[RenderMichael]

I realized we don't actually need to dispose the HTTP response, since we are disposing the client. I made it a little simpler, do you think this is acceptable?

[nvborisenko]

We have a lot of try/catch in codebase, how this one affects your debugging experience?

[RenderMichael]

I am not sure if I have something mis-configured, but the webserver Stop method always throws an exception for me, as shown in the PR description. This is something which causes a debugger break every time I am running tests.

It would be nice to avoid the debugger breaking on other exceptions which are well-handled, such as Runfiles.Create(). However, those are in synchronous methods, and there is no nice way to get that to happen.

Luckily, the quit method is an asynchronous operation and .NET 8 has an await modifier which avoids throwing the exception.

[RenderMichael]

In my opinion, this is a simplification. It requires extracting a method only because Stop is currently synchronous. Would it be better to make Stop async, and propagate that out to the callers? It would not be more complex, and it would avoid doing sync-over-async where it is not necessary

[nvborisenko]

It throws on my end, but my debugger doesn't hit it. If making Stop() method asynchronous will help you, then it is better. Thanks.

[nvborisenko]

Sorry, I cannot accept it. We have a lot of try/catch pattern in the codebase. Please understand your IDE why the debugger hits this exception, my debugger doesn't hit. Probably you have an active breakpoint in this class.

Again sorry, and thanks for understanding.