[dotnet] add asynchronous methods to Navigation class

titusfortner commented 1 month ago

User description

Description

This is a stripped down version of #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

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Review 🔍

(Review updated until commit https://github.com/SeleniumHQ/selenium/commit/f50fd80e37c0edffac3502b6d8d28d99d63093bf)

⏱️ 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);

codiumai-pr-agent-pro[bot] commented 1 month ago

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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-ecfb7c6b8ab17df6046130d592902620e9baddc4595b3140d1ebd7446da2d4d3R848-R851) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-4b9443bafd5a20a4713027f0f29bd91e6f83649cf92d3cf54f401a1a06639ef7R50-R50) ```diff -Task ExecuteAsync(Command commandToExecute); +Task 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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-13e558c131ed2ff6ab4315550e45f9bcaa3e0ae76c70a34fcd77cc4fa71a2152R53-R56) ```diff 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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-ef1874c09b1dca0d62ea1f0ead685355944b9cb7ae63df60ad9b32abdf90c8dcR558-R561) ```diff -internal Response InternalExecute(string driverCommandToExecute, Dictionary 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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-8a1f6089996b8c42dea999e7e0043f7820d548fe71b6a71a359a70c126da9443R66-R69) ```diff 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 commented 1 month ago

/help

codiumai-pr-agent-pro[bot] commented 1 month ago

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 :gem:
[DESCRIBE](https://pr-agent-docs.codium.ai/tools/describe/)	Generates PR description - title, type, summary, code walkthrough and labels	- [x] Run
[REVIEW](https://pr-agent-docs.codium.ai/tools/review/)	Adjustable feedback about the PR, possible issues, security concerns, review effort and more	- [x] Run
[IMPROVE](https://pr-agent-docs.codium.ai/tools/improve/)	Code suggestions for improving the PR	- [x] Run
[UPDATE CHANGELOG](https://pr-agent-docs.codium.ai/tools/update_changelog/)	Automatically updates the changelog	- [ ] Run
[ADD DOCS](https://pr-agent-docs.codium.ai/tools/documentation/) 💎	Generates documentation to methods/functions/classes that changed in the PR	- [ ] Run
[TEST](https://pr-agent-docs.codium.ai/tools/test/) 💎	Generates unit tests for a specific component, based on the PR code change	- [ ] Run
[IMPROVE COMPONENT](https://pr-agent-docs.codium.ai/tools/improve_component/) 💎	Code suggestions for a specific component that changed in the PR	- [ ] Run
[ANALYZE](https://pr-agent-docs.codium.ai/tools/analyze/) 💎	Identifies code components that changed in the PR, and enables to interactively generate tests, docs, and code suggestions for each component	- [ ] Run
[ASK](https://pr-agent-docs.codium.ai/tools/ask/)	Answering free-text questions about the PR	[*]
[GENERATE CUSTOM LABELS](https://pr-agent-docs.codium.ai/tools/custom_labels/) 💎	Generates custom labels for the PR, based on specific guidelines defined by the user	[*]
[CI FEEDBACK](https://pr-agent-docs.codium.ai/tools/ci_feedback/) 💎	Generates feedback and analysis for a failed CI job	[*]
[CUSTOM PROMPT](https://pr-agent-docs.codium.ai/tools/custom_prompt/) 💎	Generates custom suggestions for improving the PR code, derived only from a specific guidelines prompt defined by the user	[*]
[SIMILAR ISSUE](https://pr-agent-docs.codium.ai/tools/similar_issues/)	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.

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/f50fd80e37c0edffac3502b6d8d28d99d63093bf)

[ ] Copy walkthrough table to "Files Changed" Tab

codiumai-pr-agent-pro[bot] commented 1 month ago

Persistent review updated to latest commit https://github.com/SeleniumHQ/selenium/commit/f50fd80e37c0edffac3502b6d8d28d99d63093bf

codiumai-pr-agent-pro[bot] commented 1 month ago

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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-949f3e2193640efef49f4dc096d01e37851df0edac65eeb9c0db24b4262edd63R120-R124) ```diff [Test] -public void ShouldAcceptInvalidUrlsUsingUrisAsync() +public async Task ShouldAcceptInvalidUrlsUsingUrisAsync() { INavigation navigation = driver.Navigate(); Assert.That(async () => await navigation.GoToUrlAsync((Uri)null), Throws.InstanceOf()); } ``` 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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-949f3e2193640efef49f4dc096d01e37851df0edac65eeb9c0db24b4262edd63R139-R146) ```diff [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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-13e558c131ed2ff6ab4315550e45f9bcaa3e0ae76c70a34fcd77cc4fa71a2152R91-R95) ```diff 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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-8a1f6089996b8c42dea999e7e0043f7820d548fe71b6a71a359a70c126da9443R61-R69) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-949f3e2193640efef49f4dc096d01e37851df0edac65eeb9c0db24b4262edd63R94-R101) ```diff [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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-ecfb7c6b8ab17df6046130d592902620e9baddc4595b3140d1ebd7446da2d4d3R850-R850) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14051/files#diff-ecfb7c6b8ab17df6046130d592902620e9baddc4595b3140d1ebd7446da2d4d3R866-R866) ```diff 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

titusfortner commented 1 month ago

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.

codiumai-pr-agent-pro[bot] commented 1 month ago

CI Failure Feedback 🧐

(Checks updated until commit https://github.com/SeleniumHQ/selenium/commit/e037c9ff3adc560a8bebae397c847b94acb1bead)

**Action:** .NET / Browser Tests / Browser Tests

**Failed stage:** [Run Bazel](https://github.com/SeleniumHQ/selenium/actions/runs/9405043690/job/25910310817) [❌]

**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:

```yaml 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](https://pr-agent-docs.codium.ai/tools/ci_feedback/).

titusfortner commented 1 month ago

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 commented 1 month ago

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.

SeleniumHQ / selenium