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] Modernize exception handling in tests #14776

Merged
merged 16 commits into from
Nov 21, 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

This uses modern methods when making assertions or suppressions of exceptions.

Motivation and Context

Modernization and avoiding first-chance exceptions.

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

  • Modernized exception handling across multiple test files by replacing traditional try-catch blocks with modern assertion methods using Assert.That.
  • Improved code readability and conciseness by removing redundant exception handling and using task suppression where applicable.
  • Reformatted large JavaScript strings for better readability.
  • Enhanced test reliability by using more precise exception expectations.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
AlertsTest.cs
Modernize exception handling in AlertsTest                             

dotnet/test/common/AlertsTest.cs

  • Replaced try-catch blocks with modern assertion methods.
  • Used Assert.That with exception expectations.
  • +7/-14   
    NetworkEventsTest.cs
    Update exception handling in NetworkEventsTest                     

    dotnet/test/common/BiDi/Network/NetworkEventsTest.cs

  • Replaced try-catch with task suppression for navigation.
  • Used ConfigureAwaitOptions.SuppressThrowing.
  • +2/-5     
    CorrectEventFiringTest.cs
    Simplify exception handling in CorrectEventFiringTest       

    dotnet/test/common/CorrectEventFiringTest.cs

  • Simplified exception handling with assertions.
  • Removed unnecessary try-catch blocks.
  • +4/-12   
    ElementAttributeTest.cs
    Refactor exception handling in ElementAttributeTest           

    dotnet/test/common/ElementAttributeTest.cs

  • Replaced try-catch blocks with Assert.That for exceptions.
  • Improved readability and conciseness.
  • +6/-14   
    RemoteSeleniumServer.cs
    Enhance HTTP request handling in RemoteSeleniumServer       

    dotnet/test/common/Environment/RemoteSeleniumServer.cs

  • Used task suppression for HTTP requests.
  • Removed redundant exception handling.
  • +7/-16   
    TestWebServer.cs
    Simplify HTTP request handling in TestWebServer                   

    dotnet/test/common/Environment/TestWebServer.cs

  • Simplified HTTP request handling with task suppression.
  • Removed unnecessary try-catch blocks.
  • +2/-11   
    ExecutingJavascriptTest.cs
    Modernize exception handling in ExecutingJavascriptTest   

    dotnet/test/common/ExecutingJavascriptTest.cs

  • Used Assert.That for exception expectations.
  • Reformatted large JavaScript string for readability.
  • +54/-13 
    FrameSwitchingTest.cs
    Update frame switching exception handling                               

    dotnet/test/common/FrameSwitchingTest.cs

  • Replaced try-catch blocks with modern assertions.
  • Improved frame switching tests.
  • +7/-20   
    TargetLocatorTest.cs
    Refactor exception handling in TargetLocatorTest                 

    dotnet/test/common/TargetLocatorTest.cs

  • Replaced try-catch blocks with Assert.That.
  • Improved test readability and conciseness.
  • +17/-28 
    WindowSwitchingTest.cs
    Modernize exception handling in WindowSwitchingTest           

    dotnet/test/common/WindowSwitchingTest.cs

  • Used Assert.That for exception handling.
  • Simplified window switching tests.
  • +21/-46 

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The suppression of navigation errors may hide important failure cases that should be handled explicitly

    Error Handling
    Suppressing HTTP request exceptions without any logging or fallback behavior could mask server connectivity issues

    Code Smell
    The AlertToBePresent method has empty try-catch blocks that could be simplified or removed

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 18, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Method ignores provided locator parameter and uses hardcoded selector instead

    The ElementToBePresent method returns null when element is not found, but doesn't
    properly handle the locator parameter. The method should use the provided locator
    instead of hardcoding By.Id.

    dotnet/test/common/AlertsTest.cs [523]

    -return driver.FindElement(By.Id("open-page-with-onunload-alert"));
    +return driver.FindElement(locator);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The method ignores the locator parameter and uses a hardcoded selector, which is a significant bug that could cause incorrect behavior in tests using this method with different locators.

    8
    General
    Add delay between server status check retries to prevent overwhelming the server

    The server status check should include a delay between retries to avoid overwhelming
    the server with rapid requests. Add a small delay in the polling loop.

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

     while (!isRunning && DateTime.Now < timeout)
     {
         var statusTask = httpClient.GetAsync("http://localhost:6000/wd/hub/status");
         await ((Task)statusTask).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
    +    await Task.Delay(100);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a delay between retries is a good practice to prevent excessive server load and potential resource exhaustion, though the current implementation would still work without it.

    6
    Broaden exception handling to catch all WebDriver exceptions when checking for alerts

    The AlertToBePresent method returns null when no alert is present but doesn't
    properly handle exceptions other than NoAlertPresentException. Add handling for
    other potential exceptions.

    dotnet/test/common/WindowSwitchingTest.cs [478-485]

     try
     {
         return driver.SwitchTo().Alert();
     }
    -catch (NoAlertPresentException)
    +catch (WebDriverException)
     {
         return null;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While catching WebDriverException would make the code more robust, the current implementation focusing on NoAlertPresentException is functionally correct for most use cases.

    4

    💡 Need additional feedback ? start a PR chat

    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko Feedback has been addressed, this PR is ready for another look

    The feedback so far could be used to improve already-existing tests, but I wanted to keep this PR within reason. Maybe that can be a follow-up

    @RenderMichael
    Copy link
    Contributor Author

    Test failures are for tests I did not touch, such as shadow root exceptions not being thrown on firefox

    image

    @nvborisenko
    Copy link
    Member

    Thank you, @RenderMichael ! CI is failing due to unrelated changes (ShadowRoot).

    @nvborisenko nvborisenko merged commit 0954cca into SeleniumHQ:trunk Nov 21, 2024
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the exception-test branch November 21, 2024 05:04
    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.

    3 participants