[dotnet] add asynchronous methods to Navigation class by titusfortner · Pull Request #14051 · SeleniumHQ/selenium

titusfortner · 2024-05-29T21:07:21Z

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

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

Add Async methods on all other classes
Create helper classes to improve the maintainability of the implementation class as desired (I'll let the .NET experts figure this one out)
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

Same thing with Exception Handling
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

…vent firing webdriver

qodo-code-review · 2024-05-29T21:08:07Z

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.
⚡ Possible issues	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 file	dotnet/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 line	Task.Run(async () => await this.BackAsync().ConfigureAwait(false)).GetAwaiter().GetResult();

relevant file	dotnet/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 line	responseInfo = await this.MakeHttpRequest(requestInfo).ConfigureAwait(false);

relevant file	dotnet/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 line	return Task.Run(() => this.ExecuteAsync(driverCommandToExecute, parameters)).GetAwaiter().GetResult();

relevant file	dotnet/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 line	await this.GoToUrlAsync(url.ToString()).ConfigureAwait(false);

qodo-code-review · 2024-05-29T21:08:37Z

PR Code Suggestions ✨

Category	Suggestion	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
Best practice	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 · 2024-06-03T17:13:32Z

/help

qodo-code-review · 2024-06-03T17:13:38Z

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:

Tool	Description	Trigger 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.

qodo-code-review · 2024-06-03T17:17:54Z

PR Description updated to latest commit (f50fd80)

Copy walkthrough table to "Files Changed" Tab

qodo-code-review · 2024-06-03T17:18:17Z

Persistent review updated to latest commit f50fd80

qodo-code-review · 2024-06-03T17:18:30Z

PR Code Suggestions ✨

Category	Suggestion	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
Maintainability	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 · 2024-06-03T18:29:00Z

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 · 2024-06-03T18:53:02Z

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 · 2024-06-03T19:09:25Z

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 · 2024-06-03T19:33:11Z

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:

titusfortner · 2024-06-03T19:56:01Z

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

titusfortner · 2024-06-03T20:00:07Z

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.

qodo-code-review · 2024-06-06T17:22:02Z

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 · 2024-06-06T19:26:07Z

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 · 2024-06-06T19:27:38Z

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.

* allow driver to execute commands asynchronously * implement navigation commands with async tasks * dd async navigation methods to interface and implement in event firing webdriver * add explicit tests for navigation async methods * use async delegate with Task.Run instead of ConfigureAwait

titusfortner added 3 commits May 29, 2024 15:58

[dotnet] allow driver to execute commands asynchronously

a6700fe

[dotnet] implement navigation commands with async tasks

8872c88

[dotnet] add async navigation methods to interface and implement in e…

aa484e9

…vent firing webdriver

titusfortner added C-dotnet .NET Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels May 29, 2024

titusfortner requested a review from nvborisenko May 29, 2024 21:07

qodo-code-review bot added P-enhancement PR with a new feature tests Review effort [1-5]: 3 labels May 29, 2024

titusfortner mentioned this pull request May 29, 2024

[dotnet] implement WebDriver BiDi with WebDriverBiDi dependency #14052

Closed

titusfortner added 2 commits June 3, 2024 10:21

all async executions inside Task need to set configure await to false

2153ba2

add explicit tests for navigation async methods

f50fd80

titusfortner mentioned this pull request Jun 3, 2024

[🚀 Feature]: Transition .NET to Async library #14067

Open

titusfortner requested a review from jimevans June 3, 2024 17:15

qodo-code-review bot added Review effort [1-5]: 4 and removed Review effort [1-5]: 3 labels Jun 3, 2024

use async delegate with Task.Run instead of ConfigureAwait

44f94bc

Merge branch 'trunk' into dotnet_async_new

e037c9f

titusfortner merged commit 55f7a1d into trunk Jun 6, 2024

titusfortner deleted the dotnet_async_new branch June 6, 2024 19:28

seedysoft mentioned this pull request Jun 21, 2024

Bump Selenium.WebDriver from 4.21.0 to 4.22.0 seedysoft/all-my-stuff#53

Merged

MiguelPelegrina mentioned this pull request Jul 12, 2024

[Snyk] Upgrade selenium-webdriver from 4.21.0 to 4.22.0 MiguelPelegrina/java_school_online_store_frontend#19

Merged

aravindb26 mentioned this pull request Jul 16, 2024

[Snyk] Upgrade selenium-webdriver from 4.16.0 to 4.22.0 aravindb26/spark#3

Open

x-way mentioned this pull request Jul 24, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.22.0 x-way/cal-heatmap#503

Closed

x-way mentioned this pull request Aug 10, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.23.0 x-way/cal-heatmap#544

Closed

radmorecameron mentioned this pull request Aug 22, 2024

Switch to async methods for Selenium navigation mitchHay/StageZero#125

Merged

Uh oh!

Conversation

titusfortner commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Questions

Follow on

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-code-review bot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review 🔍

(Review updated until commit f50fd80)

Uh oh!

qodo-code-review bot commented May 29, 2024

PR Code Suggestions ✨

Uh oh!

titusfortner commented Jun 3, 2024

Uh oh!

qodo-code-review bot commented Jun 3, 2024 • edited by titusfortner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Agent Walkthrough 🤖

Uh oh!

qodo-code-review bot commented Jun 3, 2024

Uh oh!

qodo-code-review bot commented Jun 3, 2024

Uh oh!

qodo-code-review bot commented Jun 3, 2024

PR Code Suggestions ✨

Uh oh!

nvborisenko commented Jun 3, 2024

Uh oh!

YevgeniyShunevych commented Jun 3, 2024

Uh oh!

titusfortner commented Jun 3, 2024

Uh oh!

nvborisenko commented Jun 3, 2024

Uh oh!

titusfortner commented Jun 3, 2024

Uh oh!

titusfortner commented Jun 3, 2024

Uh oh!

qodo-code-review bot commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Failure Feedback 🧐

(Checks updated until commit e037c9f)

Configuration options

Uh oh!

titusfortner commented Jun 6, 2024

Uh oh!

titusfortner commented Jun 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

titusfortner commented May 29, 2024 •

edited

Loading

qodo-code-review bot commented May 29, 2024 •

edited

Loading

(Review updated until commit `f50fd80`)

qodo-code-review bot commented Jun 3, 2024 •

edited by titusfortner

Loading

qodo-code-review bot commented Jun 6, 2024 •

edited

Loading

(Checks updated until commit `e037c9f`)