Skip to content
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.

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

[dotnet] Propagate async throughout test setup and teardown #14775

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 18, 2024

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

Propagates async all the way to the top (where NUnit handles it) instead of GetAwaiter().GetResult()

Motivation and Context

Sync-over-async makes for bulkier code. It also prevents future SuppressThrowing optimizations.

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, tests


Description

  • Converted test setup and teardown methods to use async/await pattern across multiple test files.
  • Replaced synchronous start and stop methods with async versions in EnvironmentManager, RemoteSeleniumServer, and TestWebServer.
  • Improved code readability and potential performance by avoiding sync-over-async patterns.

Changes walkthrough 📝

Relevant files
Enhancement
11 files
ChromeSpecificTests.cs
Convert Chrome test teardown to async                                       

dotnet/test/chrome/ChromeSpecificTests.cs

  • Added async keyword to RunAfterAnyTests method.
  • Replaced synchronous stop method with StopAsync.
  • +3/-2     
    AssemblyFixture.cs
    Update AssemblyFixture setup and teardown to async             

    dotnet/test/common/AssemblyFixture.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous start and stop methods with async versions.
  • +7/-6     
    EnvironmentManager.cs
    Use async stop methods in EnvironmentManager destructor   

    dotnet/test/common/Environment/EnvironmentManager.cs

  • Replaced synchronous stop methods with async versions in destructor.
  • +2/-2     
    RemoteSeleniumServer.cs
    Convert RemoteSeleniumServer start and stop to async         

    dotnet/test/common/Environment/RemoteSeleniumServer.cs

  • Converted Start and Stop methods to async.
  • Replaced synchronous HTTP requests with async versions.
  • +5/-4     
    TestWebServer.cs
    Convert TestWebServer start and stop to async                       

    dotnet/test/common/Environment/TestWebServer.cs

  • Converted Start and Stop methods to async.
  • Replaced synchronous HTTP requests with async versions.
  • +5/-4     
    AssemblyTeardown.cs
    Update Edge assembly setup and teardown to async                 

    dotnet/test/edge/AssemblyTeardown.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous stop method with StopAsync.
  • +5/-4     
    AssemblyTeardown.cs
    Update Firefox assembly setup and teardown to async           

    dotnet/test/firefox/AssemblyTeardown.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous stop method with StopAsync.
  • +5/-4     
    AssemblyTeardown.cs
    Update IE assembly setup and teardown to async                     

    dotnet/test/ie/AssemblyTeardown.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous stop method with StopAsync.
  • +5/-4     
    AssemblyTeardown.cs
    Update Remote assembly setup and teardown to async             

    dotnet/test/remote/AssemblyTeardown.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous start and stop methods with async versions.
  • +7/-6     
    PopupWindowFinderTest.cs
    Update PopupWindowFinderTest setup and teardown to async 

    dotnet/test/support/UI/PopupWindowFinderTest.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous stop method with StopAsync.
  • +5/-4     
    SelectBrowserTests.cs
    Update SelectBrowserTests setup and teardown to async       

    dotnet/test/support/UI/SelectBrowserTests.cs

  • Added async keyword to setup and teardown methods.
  • Replaced synchronous stop method with StopAsync.
  • +5/-4     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Deadlock
    Using .Wait() on async tasks in the destructor could lead to deadlocks. Consider using a synchronous cleanup approach in the destructor instead.

    Error Handling
    Empty catch block silently swallows exceptions during server shutdown. Should at least log the error for debugging purposes.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Replace blocking Wait() calls in destructor with proper disposal pattern to prevent potential deadlocks

    Using .Wait() in a destructor can lead to deadlocks. Consider implementing
    IDisposable pattern to properly handle async cleanup, or use a synchronous
    alternative if async operations are not critical during shutdown.

    dotnet/test/common/Environment/EnvironmentManager.cs [198-209]

    -~EnvironmentManager()
    +public void Dispose()
     {
         if (remoteServer != null)
         {
    -        remoteServer.StopAsync().Wait();
    +        remoteServer.StopAsync().GetAwaiter().GetResult();
         }
         if (webServer != null)
         {
    -        webServer.StopAsync().Wait();
    +        webServer.StopAsync().GetAwaiter().GetResult();
         }
         CloseCurrentDriver();
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using Wait() in destructors is a dangerous practice that can lead to deadlocks. Implementing IDisposable pattern is a critical improvement for proper resource cleanup and application stability.

    9
    General
    Add ConfigureAwait(false) to async operations to prevent deadlocks and improve performance

    Add ConfigureAwait(false) to prevent potential deadlocks in ASP.NET contexts and
    improve performance by avoiding unnecessary context switches.

    dotnet/test/common/Environment/RemoteSeleniumServer.cs [71]

    -using var response = await httpClient.GetAsync("http://localhost:6000/wd/hub/status");
    +using var response = await httpClient.GetAsync("http://localhost:6000/wd/hub/status").ConfigureAwait(false);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding ConfigureAwait(false) is an important practice in .NET async code to prevent potential deadlocks in ASP.NET contexts and improve performance by avoiding unnecessary synchronization context switches.

    7

    💡 Need additional feedback ? start a PR chat

    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

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

    LGTM, thx Michael!

    @nvborisenko
    Copy link
    Member

    Thank you, @RenderMichael !

    @nvborisenko nvborisenko merged commit 0eb8393 into SeleniumHQ:trunk Nov 18, 2024
    10 checks passed
    @RenderMichael RenderMichael deleted the async-tests branch November 18, 2024 21:51
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants