Skip to content

[dotnet] Improve the debugging experience of tests #14755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

[dotnet] Improve the debugging experience of tests #14755

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a963d6f
[dotnet] Improve the debugging experience of tests
RenderMichael Nov 13, 2024
8f9b75e
Merge branch 'trunk' into dotnet-test-debugging
RenderMichael Nov 13, 2024
def2d56
remove unnecessary disposal
RenderMichael Nov 14, 2024
18d7795
Merge branch 'trunk' into dotnet-test-debugging
RenderMichael Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions dotnet/test/common/Environment/TestWebServer.cs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
using System.Net.Http;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading.Tasks;

namespace OpenQA.Selenium.Environment
{
Expand Down Expand Up @@ -178,20 +179,7 @@ public void Stop()
{
if (webserverProcess != null)
{
using (var httpClient = new HttpClient())
{
try
{
using (httpClient.GetAsync(EnvironmentManager.Instance.UrlBuilder.LocalWhereIs("quitquitquit")).GetAwaiter().GetResult())
{

}
}
catch (HttpRequestException)
{

}
}
QuitNoThrow().GetAwaiter().GetResult();

try
{
Expand All @@ -207,6 +195,15 @@ public void Stop()
webserverProcess = null;
}
}

static async Task QuitNoThrow()
{
using var httpClient = new HttpClient();

Task<HttpResponseMessage> getTask = httpClient.GetAsync(EnvironmentManager.Instance.UrlBuilder.LocalWhereIs("quitquitquit"));

await ((Task)getTask).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
}
}
}
}