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] add asynchronous methods to Navigation class #14051

Merged
merged 7 commits into from
Jun 6, 2024

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented May 29, 2024

User description

Description

  • This is a stripped down version of [dotnet] allow asynchronous command execution #13952 to focus on the bare minimum needed to get to the next step and compare BiDi implementations;
  • Execute() on CommandExecutor classes is already async, but has been made blocking; this PR now creates ExecuteAsync() to bubble up the await to WebDriver
  • Execute() methods remain blocking but now call ExecuteAsync()
  • Async methods implemented for all Navigation methods with existing sync methods calling the Async methods in a blocking fashion to maintain backwards compatibility
  • Note that this adds ConfigureAwait(false) inside all async lambdas executed in Task.Run() that include GetAwaiter().GetResult() at the end

Motivation and Context

This is an example of a partial implementation of #14067
We explicitly want to maintain both Sync and Async implementations for the time being. (possibly until Selenium 6)

Questions

  1. Is this the public facing API we want to expose to users? (I'm pretty sure the answer is yes, but I want to get an affirmative before starting next steps).

Follow on

  1. Add Async methods on all other classes
  2. Create helper classes to improve the maintainability of the implementation class as desired (I'll let the .NET experts figure this one out)
  3. AI thinks we shouldn't wrap things in Task.Run(), but I'll leave this for .NET experts to update as desired

Consider removing the unnecessary use of Task.Run() for wrapping synchronous calls in Back(), Forward(), GoToUrl(), and Refresh() methods. Instead, directly await the asynchronous methods. This change will reduce overhead and improve performance

  1. Same thing with Exception Handling
  2. Same thing with the CancellationToken recommendation

attn: @YevgeniyShunevych


PR Type

Enhancement, Tests


Description

  • Added asynchronous methods for navigation actions (Back, Forward, GoToUrl, Refresh) in EventFiringWebDriver, INavigation, and Navigator.
  • Introduced ExecuteAsync method to ICommandExecutor, DriverServiceCommandExecutor, and HttpCommandExecutor.
  • Synchronous methods now call their asynchronous counterparts to maintain backward compatibility.
  • Added ConfigureAwait(false) to all async calls to prevent deadlocks.
  • Added and updated tests to cover the new asynchronous methods.

Changes walkthrough 📝

Relevant files
Enhancement
7 files
EventFiringWebDriver.cs
Add asynchronous navigation methods to EventFiringWebDriver

dotnet/src/support/Events/EventFiringWebDriver.cs

  • Added asynchronous methods for navigation actions (Back, Forward,
    GoToUrl, Refresh).
  • Synchronous methods now call their asynchronous counterparts.
  • Introduced ConfigureAwait(false) for all async calls.
  • +62/-20 
    ICommandExecutor.cs
    Add asynchronous command execution to ICommandExecutor interface

    dotnet/src/webdriver/ICommandExecutor.cs

    • Added ExecuteAsync method to ICommandExecutor interface.
    +9/-0     
    INavigation.cs
    Add asynchronous navigation methods to INavigation interface

    dotnet/src/webdriver/INavigation.cs

  • Added asynchronous methods for navigation actions (Back, Forward,
    GoToUrl, Refresh).
  • +33/-0   
    Navigator.cs
    Implement asynchronous navigation methods in Navigator class

    dotnet/src/webdriver/Navigator.cs

  • Implemented asynchronous methods for navigation actions (Back,
    Forward, GoToUrl, Refresh).
  • Synchronous methods now call their asynchronous counterparts.
  • +59/-12 
    DriverServiceCommandExecutor.cs
    Add asynchronous command execution to DriverServiceCommandExecutor

    dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs

  • Added ExecuteAsync method to DriverServiceCommandExecutor.
  • Synchronous Execute method now calls the asynchronous version.
  • +12/-1   
    HttpCommandExecutor.cs
    Add asynchronous command execution to HttpCommandExecutor

    dotnet/src/webdriver/Remote/HttpCommandExecutor.cs

  • Added ExecuteAsync method to HttpCommandExecutor.
  • Synchronous Execute method now calls the asynchronous version.
  • +11/-1   
    WebDriver.cs
    Add asynchronous internal command execution to WebDriver 

    dotnet/src/webdriver/WebDriver.cs

  • Added InternalExecuteAsync method for internal command execution.
  • Synchronous methods now call their asynchronous counterparts.
  • +22/-3   
    Tests
    2 files
    NavigationTest.cs
    Add tests for asynchronous navigation methods                       

    dotnet/test/common/NavigationTest.cs

    • Added tests for asynchronous navigation methods.
    +70/-1   
    EventFiringWebDriverTest.cs
    Update tests for EventFiringWebDriver to use async methods

    dotnet/test/support/Events/EventFiringWebDriverTest.cs

    • Updated tests to verify asynchronous navigation methods.
    +7/-4     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    codiumai-pr-agent-pro bot commented May 29, 2024

    PR Review 🔍

    (Review updated until commit f50fd80)

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves significant changes across multiple files, including the introduction of asynchronous methods alongside existing synchronous methods. Reviewing these changes requires a deep understanding of asynchronous programming in C# and the existing architecture of the Selenium WebDriver. Additionally, the changes impact core navigation functionalities which are critical, thus requiring thorough testing and validation.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The use of Task.Run() to wrap synchronous code execution in asynchronous method calls might lead to unnecessary overhead and could potentially affect the performance. This pattern is seen across multiple methods like Back(), Forward(), GoToUrl(), and Refresh() in various classes.

    Thread Safety Concern: The current implementation may not be fully thread-safe. The use of asynchronous programming requires careful consideration of thread safety, especially in a library that might be used in a multi-threaded context.

    🔒 Security concerns

    No

    Code feedback:
    relevant filedotnet/src/support/Events/EventFiringWebDriver.cs
    suggestion      

    Consider removing the unnecessary use of Task.Run() for wrapping synchronous calls in Back(), Forward(), GoToUrl(), and Refresh() methods. Instead, directly await the asynchronous methods. This change will reduce overhead and improve performance. [important]

    relevant lineTask.Run(async () => await this.BackAsync().ConfigureAwait(false)).GetAwaiter().GetResult();

    relevant filedotnet/src/webdriver/Remote/HttpCommandExecutor.cs
    suggestion      

    Ensure proper exception handling in ExecuteAsync() method. Currently, the method catches HttpRequestException but does not rethrow it, which could lead to swallowing exceptions that should be visible to the caller. Consider adding a throw; statement in the catch block to rethrow the caught exception. [important]

    relevant lineresponseInfo = await this.MakeHttpRequest(requestInfo).ConfigureAwait(false);

    relevant filedotnet/src/webdriver/WebDriver.cs
    suggestion      

    Avoid using Task.Run() to call ExecuteAsync() within InternalExecuteAsync(). Since ExecuteAsync() is already asynchronous, you can directly await it without wrapping in Task.Run(), which is more efficient and clearer. [important]

    relevant linereturn Task.Run(() => this.ExecuteAsync(driverCommandToExecute, parameters)).GetAwaiter().GetResult();

    relevant filedotnet/src/webdriver/Navigator.cs
    suggestion      

    Refactor GoToUrlAsync(Uri url) to avoid calling ToString() on the URI, which is unnecessary and can be error-prone. Instead, use the Uri object directly in the asynchronous call to improve reliability and performance. [medium]

    relevant lineawait this.GoToUrlAsync(url.ToString()).ConfigureAwait(false);

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Use async and await directly in synchronous methods to avoid unnecessary thread pool usage

    Instead of using Task.Run to call the asynchronous methods from the synchronous ones,
    consider using async and await directly in the synchronous methods. This will avoid
    unnecessary thread pool usage and potential deadlocks.

    dotnet/src/support/Events/EventFiringWebDriver.cs [848-851]

    -public void Back()
    +public async void Back()
     {
    -    Task.Run(this.BackAsync).GetAwaiter().GetResult();
    +    await this.BackAsync().ConfigureAwait(false);
     }
     
    Suggestion importance[1-10]: 8

    Why: Using async and await directly instead of Task.Run can help prevent unnecessary thread pool usage and potential deadlocks, which is a significant improvement in asynchronous programming practices.

    8
    Best practice
    Add a CancellationToken parameter to the asynchronous method for better control over task execution

    To ensure that the asynchronous method ExecuteAsync is properly implemented, consider
    adding a CancellationToken parameter to allow the operation to be cancelled if needed.
    This is a common practice in asynchronous programming to provide more control over the
    task execution.

    dotnet/src/webdriver/ICommandExecutor.cs [50]

    -Task<Response> ExecuteAsync(Command commandToExecute);
    +Task<Response> ExecuteAsync(Command commandToExecute, CancellationToken cancellationToken = default);
     
    Suggestion importance[1-10]: 8

    Why: Adding a CancellationToken parameter to asynchronous methods is a best practice in C# to allow more control over task execution and to handle cancellations effectively. This suggestion is relevant and improves the API's usability and robustness.

    8
    Add exception handling in asynchronous methods to properly catch and handle exceptions

    Add exception handling in the asynchronous methods to ensure that any exceptions thrown
    during the asynchronous operations are properly caught and handled.

    dotnet/src/webdriver/Navigator.cs [53-56]

     public async Task BackAsync()
     {
    -    await this.driver.InternalExecuteAsync(DriverCommand.GoBack, null).ConfigureAwait(false);
    +    try
    +    {
    +        await this.driver.InternalExecuteAsync(DriverCommand.GoBack, null).ConfigureAwait(false);
    +    }
    +    catch (Exception ex)
    +    {
    +        // Handle exception
    +        throw;
    +    }
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding exception handling in asynchronous methods is a best practice that ensures exceptions are properly managed, enhancing the robustness of the code.

    7
    Maintainability
    Remove redundant synchronous wrapper methods to simplify the code and improve performance

    Remove the redundant InternalExecute method that wraps the asynchronous method with
    Task.Run, as it adds unnecessary complexity and potential performance overhead.

    dotnet/src/webdriver/WebDriver.cs [558-561]

    -internal Response InternalExecute(string driverCommandToExecute, Dictionary<string, object> parameters)
    -{
    -    return Task.Run(() => this.InternalExecuteAsync(driverCommandToExecute, parameters)).GetAwaiter().GetResult();
    -}
    +// Removed redundant InternalExecute method
     
    Suggestion importance[1-10]: 6

    Why: Removing redundant synchronous wrappers around asynchronous methods simplifies the codebase and can improve performance by reducing unnecessary complexity.

    6
    Test coverage
    Add verification for the RefreshAsync method in the test to ensure asynchronous refresh functionality is tested

    Add verification for the RefreshAsync method in the test to ensure that the refresh
    functionality is also tested asynchronously.

    dotnet/test/support/Events/EventFiringWebDriverTest.cs [66-69]

     mockNavigation.Verify(x => x.ForwardAsync(), Times.Once);
    +mockNavigation.Verify(x => x.RefreshAsync(), Times.Once);
     string normalizedActualLog =  log.ToString().Replace("\r\n", "\n").Replace("\r", "\n");
     Assert.AreEqual(normalizedExpectedLog, normalizedActualLog);
     
    Suggestion importance[1-10]: 5

    Why: Adding verification for the RefreshAsync method in unit tests ensures that all asynchronous functionality is properly tested, which is crucial for maintaining the reliability of the asynchronous operations in the system.

    5

    @titusfortner
    Copy link
    Member Author

    /help

    Copy link

    codiumai-pr-agent-pro bot commented Jun 3, 2024

    PR Agent Walkthrough 🤖

    Welcome to the PR Agent, an AI-powered tool for automated pull request analysis, feedback, suggestions and more.

    Here is a list of tools you can use to interact with the PR Agent:

    ToolDescriptionTrigger Interactively 💎

    DESCRIBE

    Generates PR description - title, type, summary, code walkthrough and labels
    • Run

    REVIEW

    Adjustable feedback about the PR, possible issues, security concerns, review effort and more
    • Run

    IMPROVE

    Code suggestions for improving the PR
    • Run

    UPDATE CHANGELOG

    Automatically updates the changelog
    • Run

    ADD DOCS 💎

    Generates documentation to methods/functions/classes that changed in the PR
    • Run

    TEST 💎

    Generates unit tests for a specific component, based on the PR code change
    • Run

    IMPROVE COMPONENT 💎

    Code suggestions for a specific component that changed in the PR
    • Run

    ANALYZE 💎

    Identifies code components that changed in the PR, and enables to interactively generate tests, docs, and code suggestions for each component
    • Run

    ASK

    Answering free-text questions about the PR

    [*]

    GENERATE CUSTOM LABELS 💎

    Generates custom labels for the PR, based on specific guidelines defined by the user

    [*]

    CI FEEDBACK 💎

    Generates feedback and analysis for a failed CI job

    [*]

    CUSTOM PROMPT 💎

    Generates custom suggestions for improving the PR code, derived only from a specific guidelines prompt defined by the user

    [*]

    SIMILAR ISSUE

    Automatically retrieves and presents similar issues

    [*]

    (1) Note that each tool be triggered automatically when a new PR is opened, or called manually by commenting on a PR.

    (2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the /ask tool, you need to comment on a PR: /ask "<question content>". See the relevant documentation for each tool for more details.

    Copy link

    PR Description updated to latest commit (f50fd80)

    Copy link

    Persistent review updated to latest commit f50fd80

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure the test method runs asynchronously by marking it as async and returning a Task

    The ShouldAcceptInvalidUrlsUsingUrisAsync test method should be marked as async and return
    a Task to ensure it runs asynchronously.

    dotnet/test/common/NavigationTest.cs [120-124]

     [Test]
    -public void ShouldAcceptInvalidUrlsUsingUrisAsync()
    +public async Task ShouldAcceptInvalidUrlsUsingUrisAsync()
     {
         INavigation navigation = driver.Navigate();
         Assert.That(async () => await navigation.GoToUrlAsync((Uri)null), Throws.InstanceOf<ArgumentNullException>());
     }
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies a critical issue where an asynchronous test method is not properly defined as async and does not return a Task. This is crucial for the correct execution of asynchronous tests.

    10
    Await asynchronous calls to ensure proper execution order and error handling

    The ShouldGoToUrlUsingUriAsync test method should await the asynchronous calls to
    GoToUrlAsync to ensure proper execution order and error handling.

    dotnet/test/common/NavigationTest.cs [139-146]

     [Test]
    -public void ShouldGoToUrlUsingUriAsync()
    +public async Task ShouldGoToUrlUsingUriAsync()
     {
         var navigation = driver.Navigate();
    -    navigation.GoToUrlAsync(new Uri(macbethPage));
    +    await navigation.GoToUrlAsync(new Uri(macbethPage));
         Assert.AreEqual(driver.Title, macbethTitle);
    -    navigation.GoToUrl(new Uri(simpleTestPage));
    +    await navigation.GoToUrlAsync(new Uri(simpleTestPage));
         Assert.AreEqual(simpleTestTitle, driver.Title);
     }
     
    Suggestion importance[1-10]: 10

    Why: The suggestion is accurate and addresses a significant issue where asynchronous methods are called without await, potentially leading to race conditions and incorrect test behavior.

    10
    Validate the URL format in GoToUrlAsync method to ensure it is a valid URL before proceeding

    In the GoToUrlAsync method, after checking for null, consider validating the URL format to
    ensure it is a valid URL before proceeding with the navigation.

    dotnet/src/webdriver/Navigator.cs [91-95]

     if (url == null)
     {
         throw new ArgumentNullException(nameof(url), "URL cannot be null.");
     }
    +if (!Uri.IsWellFormedUriString(url, UriKind.Absolute))
    +{
    +    throw new ArgumentException("The URL provided is not valid.", nameof(url));
    +}
     
    Suggestion importance[1-10]: 5

    Why: The suggestion to validate the URL format is a good practice to ensure the URL is correct before processing. However, the impact of this change is moderate as it primarily adds a validation step which might not be critical depending on the broader context of the application usage.

    5
    Maintainability
    Extract log normalization logic into a separate method for better readability and maintainability

    To improve readability and maintainability, consider extracting the log normalization
    logic into a separate method.

    dotnet/test/support/Events/EventFiringWebDriverTest.cs [61-69]

    -string normalizedExpectedLog = expectedLog.Replace("\r\n", "\n").Replace("\r", "\n");
    +string NormalizeLog(string log) => log.Replace("\r\n", "\n").Replace("\r", "\n");
     ...
    -string normalizedActualLog =  log.ToString().Replace("\r\n", "\n").Replace("\r", "\n");
    +string normalizedExpectedLog = NormalizeLog(expectedLog);
    +string normalizedActualLog = NormalizeLog(log.ToString());
     Assert.AreEqual(normalizedExpectedLog, normalizedActualLog);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves code maintainability and readability by extracting repeated logic into a separate method, which is a good practice, especially when dealing with string manipulations that are used multiple times.

    8
    Remove unnecessary Task.CompletedTask return statement in the asynchronous test method

    In the ShouldNotHaveProblemNavigatingWithNoPagesBrowsedAsync method, the
    Task.CompletedTask return statement is unnecessary and can be removed.

    dotnet/test/common/NavigationTest.cs [94-101]

     [Test]
     [NeedsFreshDriver(IsCreatedBeforeTest = true)]
    -public Task ShouldNotHaveProblemNavigatingWithNoPagesBrowsedAsync()
    +public async Task ShouldNotHaveProblemNavigatingWithNoPagesBrowsedAsync()
     {
         var navigation = driver.Navigate();
         Assert.DoesNotThrowAsync(async () => await navigation.BackAsync());
         Assert.DoesNotThrowAsync(async () => await navigation.ForwardAsync());
    -    return Task.CompletedTask;
     }
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies an unnecessary return statement in an asynchronous test method, improving the code's clarity and maintainability. However, it's a relatively minor improvement compared to the other issues.

    7
    Best practice
    Use async and await directly in synchronous methods instead of Task.Run for better error handling and to avoid potential deadlocks

    Instead of using Task.Run to call the asynchronous methods from the synchronous ones,
    consider using async and await directly in the synchronous methods. This will provide
    better error handling and avoid potential deadlocks.

    dotnet/src/support/Events/EventFiringWebDriver.cs [850]

    -Task.Run(async () => await this.BackAsync().ConfigureAwait(false)).GetAwaiter().GetResult();
    +public async void Back()
    +{
    +    await this.BackAsync().ConfigureAwait(false);
    +}
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to replace Task.Run with direct async and await usage in synchronous methods is valid and improves readability and error handling. However, the suggested code change to make Back() an async void method is not recommended due to potential issues with error handling and control flow.

    7
    Enhancement
    Add logging inside the catch blocks to capture exceptions for better debugging

    Add logging inside the catch blocks to capture exceptions. This will help in debugging and
    understanding the issues that occur during navigation.

    dotnet/src/support/Events/EventFiringWebDriver.cs [866]

     catch (Exception ex)
    +{
    +    // Log the exception
    +    Console.WriteLine($"Exception occurred: {ex.Message}");
    +    throw;
    +}
     
    Suggestion importance[1-10]: 6

    Why: Adding logging within catch blocks is a good practice for better error diagnostics and understanding operational issues. The suggestion is contextually correct and targets the new exception handling code introduced in the PR.

    6

    @nvborisenko
    Copy link
    Member

    Note that this adds ConfigureAwait(false) inside all async lambdas executed in Task.Run() that include GetAwaiter().GetResult() at the end.

    I think it is unnecessary in this context. I also would suggest to introduce kind of AsyncHelper who will be responsible to "resolve" sync-over-async problem. Later on we will be able to find all places across the repo too understand what is still "bad" in the code.

    @YevgeniyShunevych
    Copy link
    Contributor

    Agree with @nvborisenko. I would also keep sync-over-async implementation encapsulated in a class like this:

    internal static class AsyncHelper
    {
        internal static TResult RunSync<TResult>(Func<Task<TResult>> function) =>
            Task.Run(function).GetAwaiter().GetResult();
    
        internal static void RunSync(Func<Task> function) =>
            Task.Run(function).GetAwaiter().GetResult();
    }

    @titusfortner
    Copy link
    Member Author

    Abstracting out a helper class is out of scope for this PR. You can argue the best way to do that later; this is what the internet suggested I do (well, mostly) based on this particular execution.

    This is the user-facing API we want?

    I want to get something basic merged in before we start evaluating/comparing BiDi implementations

    @nvborisenko
    Copy link
    Member

    If you land it now, than nobody will return to it back later. My internet is different https://devblogs.microsoft.com/dotnet/configureawait-faq/#can-i-use-task-run-to-avoid-using-configureawaitfalse:

    image

    @titusfortner
    Copy link
    Member Author

    Nice, hadn't seen that, just the general advice that in a library every await should include a ConfigureAwait(false)

    @titusfortner
    Copy link
    Member Author

    than nobody will return to it back later

    That's fine too. That's a good indication that it doesn't actually matter.

    Really I just want to make sure that a different implementation won't require a different user-facing API, and that any changes you might want to make will work with this API.

    Copy link

    codiumai-pr-agent-pro bot commented Jun 6, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit e037c9f)

    Action: .NET / Browser Tests / Browser Tests

    Failed stage: Run Bazel [❌]

    Failed test name: ElementFindingTest-firefox

    Failure summary:

    The action failed due to the following reasons:

  • The protoc.exe command failed to execute successfully, causing the generation of proto_library and
    Descriptor Set proto_library to fail.
  • Specific errors include:
    - Generating proto_library
    @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto [for tool] failed: (Exit
    -1073741819): protoc.exe failed
    - Generating Descriptor Set proto_library
    @@protobuf~//:timestamp_proto [for tool] failed: (Exit -1073741819): protoc.exe failed
  • As a result, the build did not complete successfully, leading to the failure of the
    ElementFindingTest-firefox test to build.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Microsoft Windows Server 2022
    ...
    
    679:  �[32m[1,149 / 2,113]�[0m Building Python zip: //common/devtools:pdl_to_json [for tool]; 4s local ... (4 actions, 3 running)
    680:  �[32m[1,154 / 2,113]�[0m Building Python zip: //common/devtools:pdl_to_json [for tool]; 5s local ... (4 actions running)
    681:  �[32m[1,158 / 2,113]�[0m Building Python zip: //common/devtools:pdl_to_json [for tool]; 6s local ... (4 actions running)
    682:  �[32m[1,159 / 2,113]�[0m Building Python zip: //common/devtools:pdl_to_json [for tool]; 9s local ... (4 actions, 3 running)
    683:  �[32mINFO: �[0mFrom Compiling generator [for tool]:
    684:  warning CS1701: Assuming assembly reference 'System.Runtime, Version=4.2.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' used by 'Microsoft.Extensions.DependencyInjection' matches identity 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' of 'System.Runtime', you may need to supply runtime policy
    685:  �[32m[1,163 / 2,113]�[0m Compiling src/google/protobuf/compiler/cpp/message_field.cc [for tool]; 2s local ... (4 actions, 3 running)
    686:  �[32mINFO: �[0mFrom Building java/src/org/openqa/selenium/remote/libapi-class.jar (66 source files):
    687:  java\src\org\openqa\selenium\remote\ErrorHandler.java:46: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    688:  private final ErrorCodes errorCodes;
    689:  ^
    690:  java\src\org\openqa\selenium\remote\ErrorHandler.java:60: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    691:  this.errorCodes = new ErrorCodes();
    692:  ^
    693:  java\src\org\openqa\selenium\remote\ErrorHandler.java:68: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    694:  public ErrorHandler(ErrorCodes codes, boolean includeServerErrors) {
    695:  ^
    696:  java\src\org\openqa\selenium\remote\Response.java:97: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    697:  ErrorCodes errorCodes = new ErrorCodes();
    698:  ^
    699:  java\src\org\openqa\selenium\remote\Response.java:97: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    700:  ErrorCodes errorCodes = new ErrorCodes();
    701:  ^
    702:  java\src\org\openqa\selenium\remote\ProtocolHandshake.java:181: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    703:  response.setStatus(ErrorCodes.SUCCESS);
    704:  ^
    705:  java\src\org\openqa\selenium\remote\ProtocolHandshake.java:182: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    706:  response.setState(ErrorCodes.SUCCESS_STRING);
    707:  ^
    708:  java\src\org\openqa\selenium\remote\W3CHandshakeResponse.java:53: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    709:  new ErrorCodes().toStatus((String) rawError, Optional.of(tuple.getStatusCode())));
    710:  ^
    711:  java\src\org\openqa\selenium\remote\W3CHandshakeResponse.java:56: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    712:  new ErrorCodes().getExceptionType((String) rawError);
    713:  ^
    714:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:44: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    715:  private final ErrorCodes errorCodes = new ErrorCodes();
    716:  ^
    717:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:44: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    718:  private final ErrorCodes errorCodes = new ErrorCodes();
    719:  ^
    720:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:55: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    721:  int status = response.getStatus() == ErrorCodes.SUCCESS ? HTTP_OK : HTTP_INTERNAL_ERROR;
    722:  ^
    723:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:101: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    724:  response.setStatus(ErrorCodes.UNKNOWN_COMMAND);
    725:  ^
    726:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:103: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    727:  response.setStatus(ErrorCodes.UNHANDLED_ERROR);
    728:  ^
    729:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:124: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    730:  response.setStatus(ErrorCodes.SUCCESS);
    731:  ^
    732:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:125: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    733:  response.setState(errorCodes.toState(ErrorCodes.SUCCESS));
    734:  ^
    735:  java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:131: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    736:  response.setState(errorCodes.toState(ErrorCodes.SUCCESS));
    737:  ^
    738:  java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:70: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    739:  private final ErrorCodes errorCodes = new ErrorCodes();
    740:  ^
    741:  java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:70: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    742:  private final ErrorCodes errorCodes = new ErrorCodes();
    743:  ^
    744:  java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:93: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    745:  response.setStatus(ErrorCodes.UNKNOWN_COMMAND);
    746:  ^
    747:  java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:98: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    748:  response.setStatus(ErrorCodes.UNHANDLED_ERROR);
    749:  ^
    750:  java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:145: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal
    751:  response.setStatus(ErrorCodes.SUCCESS);
    ...
    
    838:  �[32m[1,302 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 6s local ... (4 actions running)
    839:  �[32m[1,304 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 7s local ... (4 actions running)
    840:  �[32m[1,305 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 10s local ... (4 actions, 3 running)
    841:  �[32m[1,307 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 12s local ... (4 actions running)
    842:  �[32m[1,308 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 13s local ... (4 actions, 3 running)
    843:  �[32m[1,311 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 14s local ... (3 actions, 2 running)
    844:  �[32m[1,311 / 2,113]�[0m Generating v124 DevTools Protocol bindings for .NET; 15s local ... (3 actions running)
    845:  �[32m[1,313 / 2,113]�[0m Calculating 712 JavaScript deps to javascript/atoms/deps.js; 1s local ... (4 actions running)
    846:  �[31m�[1mERROR: �[0mD:/_bazel/external/io_bazel_rules_closure/java/io/bazel/rules/closure/BUILD:64:14: Generating proto_library @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto [for tool] failed: (Exit -1073741819): protoc.exe failed: error executing GenProto command (from target @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto) 
    847:  cd /d D:/_bazel/execroot/_main
    848:  SET PATH=C:\Program Files\Git\bin;C:\Program Files\Git\usr\bin;C:\Windows;C:\Windows\System32;C:\Windows\System32\WindowsPowerShell\v1.0
    849:  bazel-out\x64_windows-opt-exec-ST-4eedf4a3b688\bin\external\protobuf~\protoc.exe --java_out=bazel-out/x64_windows-opt-exec-ST-4eedf4a3b688/bin/external/io_bazel_rules_closure/java/io/bazel/rules/closure/build_info_proto-speed-src.jar -Iexternal/io_bazel_rules_closure -I. external/io_bazel_rules_closure/java/io/bazel/rules/closure/build_info.proto
    850:  # Configuration: c4c5b213685058484698870969db21c43c1827cd924f94ca91a1e5ef606ef2ca
    851:  # Execution platform: @@local_config_platform//:host
    852:  �[31m�[1mERROR: �[0mD:/_bazel/external/protobuf~/BUILD.bazel:316:14: Generating Descriptor Set proto_library @@protobuf~//:timestamp_proto [for tool] failed: (Exit -1073741819): protoc.exe failed: error executing GenProtoDescriptorSet command (from target @@protobuf~//:timestamp_proto) 
    853:  cd /d D:/_bazel/execroot/_main
    854:  SET PATH=C:\Program Files\Git\bin;C:\Program Files\Git\usr\bin;C:\Windows;C:\Windows\System32;C:\Windows\System32\WindowsPowerShell\v1.0
    855:  bazel-out\x64_windows-opt-exec-ST-4eedf4a3b688\bin\external\protobuf~\protoc.exe --direct_dependencies google/protobuf/timestamp.proto --direct_dependencies_violation_msg=%s is imported, but @@protobuf~//:timestamp_proto doesn't directly depend on a proto_library that 'srcs' it. --descriptor_set_out=bazel-out/x64_windows-opt-exec-ST-4eedf4a3b688/bin/external/protobuf~/timestamp_proto-descriptor-set.proto.bin -Ibazel-out/x64_windows-opt-exec-ST-4eedf4a3b688/bin/external/protobuf~/_virtual_imports/timestamp_proto -I. bazel-out/x64_windows-opt-exec-ST-4eedf4a3b688/bin/external/protobuf~/_virtual_imports/timestamp_proto/google/protobuf/timestamp.proto
    856:  # Configuration: c4c5b213685058484698870969db21c43c1827cd924f94ca91a1e5ef606ef2ca
    857:  # Execution platform: @@local_config_platform//:host
    858:  �[32mINFO: �[0mElapsed time: 308.251s, Critical Path: 56.97s
    859:  �[32mINFO: �[0m1318 processes: 827 internal, 448 local, 43 worker.
    860:  �[31m�[1mERROR: �[0mBuild did NOT complete successfully
    861:  //dotnet/test/common:ElementFindingTest-chrome                        �[0m�[31m�[1mNO STATUS�[0m
    862:  //dotnet/test/common:ElementFindingTest-firefox                 �[0m�[31m�[1mFAILED TO BUILD�[0m
    863:  Executed 0 out of 2 tests: �[0m�[31m�[1m1 fails to build�[0m and �[0m�[35m1 was skipped�[0m.
    864:  �[0m
    865:  ##[error]Process completed with exit code 1.
    866:  Post job cleanup.
    867:  ##[group]Save cache for external-rules_dotnet~~rules_dotnet_nuget_packages_extension~nuget.netstandard.library.v2.0.3
    868:  [command]"C:\Program Files\Git\usr\bin\tar.exe" --posix -cf cache.tzst --exclude cache.tzst -P -C D:/a/selenium/selenium --files-from manifest.txt --force-local --use-compress-program "zstd -T0"
    869:  Failed to save: Unable to reserve cache with key setup-bazel-2-win32-external-rules_dotnet~~rules_dotnet_nuget_packages_extension~nuget.netstandard.library.v2.0.3-04e76b0f7a32645b4adb2fb87265df951412574d58a3904263ab6e131831a4cd, another job may be creating this cache. More details: Cache already exists. Scope: refs/pull/14051/merge, Key: setup-bazel-2-win32-external-rules_dotnet~~rules_dotnet_nuget_packages_extension~nuget.netstandard.library.v2.0.3-04e76b0f7a32645b4adb2fb87265df951412574d58a3904263ab6e131831a4cd, Version: 64e7856ef7807061948c4d0bd31e6d574b51e201a14b538096271148e3c7707b
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @titusfortner
    Copy link
    Member Author

    updated to use async delegate. Windows tests are failing due to whatever is going on with protoc.exe; Navigation tests are passing locally and on RBE.

    @titusfortner
    Copy link
    Member Author

    Anyone should feel free to create helpers/abstractions for this and/or other code already in the library if it makes sense to do so.

    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