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.

Sign up for GitHub

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] implement high level API for WebDriver BiDi Log Events #14057

Closed
wants to merge 10 commits into from
Closed

[dotnet] implement high level API for WebDriver BiDi Log Events #14057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a6700fe
[dotnet] allow driver to execute commands asynchronously
titusfortner May 29, 2024
8872c88
[dotnet] implement navigation commands with async tasks
titusfortner May 29, 2024
aa484e9
[dotnet] add async navigation methods to interface and implement in e…
titusfortner May 29, 2024
4b42c7a
[dotnet] add webdriver bidi dependency
titusfortner May 16, 2024
5bcd3e9
[dotnet] tests should default to bidi disabled
titusfortner May 21, 2024
dc502b6
[dotnet] start a BiDi session if webSocketUrl is set
titusfortner May 29, 2024
38a94b6
[dotnet] implement navigation commands with WebDriver BiDi
titusfortner May 29, 2024
d66e0e4
[dotnet] implement appropriate waiting for bidi navigation based on p…
titusfortner May 29, 2024
5e29104
[dotnet] implement BiDi handlers for log events
titusfortner May 31, 2024
b38f405
[dotnet] create new test suite for running only BiDi tests
titusfortner May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 73 additions & 20 deletions dotnet/src/support/Events/EventFiringWebDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Drawing;
using System.Threading.Tasks;

namespace OpenQA.Selenium.Support.Events
{
Expand Down Expand Up @@ -332,6 +333,17 @@ public INavigation Navigate()
return new EventFiringNavigation(this);
}

/// <summary>
/// Provides access to WebDriverBiDi events in script and log domains.
/// </summary>
/// <returns>An <see cref="IScript"/> object allowing the user to access
/// WebDriverBiDi events.</returns>
[Obsolete("This method is in beta and may change in future releases.")]
public IScript Script()
{
throw new NotImplementedException();
}

/// <summary>
/// Instructs the driver to send future commands to a different frame or window.
/// </summary>
Expand Down Expand Up @@ -845,12 +857,21 @@ public EventFiringNavigation(EventFiringWebDriver driver)
/// Move the browser back
/// </summary>
public void Back()
{
Task.Run(this.BackAsync).GetAwaiter().GetResult();
}

/// <summary>
/// Move the browser back as an asynchronous task.
/// </summary>
/// <returns>A task object representing the asynchronous operation</returns>
public async Task BackAsync()
{
try
{
WebDriverNavigationEventArgs e = new WebDriverNavigationEventArgs(this.parentDriver);
this.parentDriver.OnNavigatingBack(e);
this.wrappedNavigation.Back();
await this.wrappedNavigation.BackAsync().ConfigureAwait(false);
this.parentDriver.OnNavigatedBack(e);
}
catch (Exception ex)
Expand All @@ -861,15 +882,24 @@ public void Back()
}

/// <summary>
/// Move the browser forward
/// Move a single "item" forward in the browser's history.
/// </summary>
public void Forward()
{
Task.Run(this.ForwardAsync).GetAwaiter().GetResult();
}

/// <summary>
/// Move a single "item" forward in the browser's history as an asynchronous task.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
public async Task ForwardAsync()
{
try
{
WebDriverNavigationEventArgs e = new WebDriverNavigationEventArgs(this.parentDriver);
this.parentDriver.OnNavigatingForward(e);
this.wrappedNavigation.Forward();
await this.wrappedNavigation.ForwardAsync().ConfigureAwait(false);
this.parentDriver.OnNavigatedForward(e);
}
catch (Exception ex)
Expand All @@ -880,16 +910,31 @@ public void Forward()
}

/// <summary>
/// Navigate to a url for your test
/// Navigate to a url.
/// </summary>
/// <param name="url">String of where you want the browser to go to</param>
public void GoToUrl(string url)
{
Task.Run(() => this.GoToUrlAsync(url)).GetAwaiter().GetResult();
}

/// <summary>
/// Navigate to a url as an asynchronous task.
/// </summary>
/// <param name="url">String of where you want the browser to go.</param>
/// <returns>A task object representing the asynchronous operation.</returns>
public async Task GoToUrlAsync(string url)
{
if (url == null)
{
throw new ArgumentNullException(nameof(url), "url cannot be null");
}

try
{
WebDriverNavigationEventArgs e = new WebDriverNavigationEventArgs(this.parentDriver, url);
this.parentDriver.OnNavigating(e);
this.wrappedNavigation.GoToUrl(url);
await this.wrappedNavigation.GoToUrlAsync(url).ConfigureAwait(false);
this.parentDriver.OnNavigated(e);
}
catch (Exception ex)
Expand All @@ -900,38 +945,46 @@ public void GoToUrl(string url)
}

/// <summary>
/// Navigate to a url for your test
/// Navigate to a url.
/// </summary>
/// <param name="url">Uri object of where you want the browser to go to</param>
public void GoToUrl(Uri url)
{
Task.Run(() => this.GoToUrlAsync(url)).GetAwaiter().GetResult();
}

/// <summary>
/// Navigate to a url as an asynchronous task.
/// </summary>
/// <param name="url">Uri object of where you want the browser to go.</param>
/// <returns>A task object representing the asynchronous operation.</returns>
public async Task GoToUrlAsync(Uri url)
{
if (url == null)
{
throw new ArgumentNullException(nameof(url), "url cannot be null");
}

try
{
WebDriverNavigationEventArgs e = new WebDriverNavigationEventArgs(this.parentDriver, url.ToString());
this.parentDriver.OnNavigating(e);
this.wrappedNavigation.GoToUrl(url);
this.parentDriver.OnNavigated(e);
}
catch (Exception ex)
{
this.parentDriver.OnException(new WebDriverExceptionEventArgs(this.parentDriver, ex));
throw;
}
await this.GoToUrlAsync(url.ToString()).ConfigureAwait(false);
}

/// <summary>
/// Refresh the browser
/// Reload the current page.
/// </summary>
public void Refresh()
{
Task.Run(this.RefreshAsync).GetAwaiter().GetResult();
}

/// <summary>
/// Reload the current page as an asynchronous task.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
public async Task RefreshAsync()
{
try
{
this.wrappedNavigation.Refresh();
await this.wrappedNavigation.RefreshAsync().ConfigureAwait(false);
}
catch (Exception ex)
{
Expand Down
9 changes: 9 additions & 0 deletions dotnet/src/webdriver/ICommandExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// </copyright>

using System;
using System.Threading.Tasks;

namespace OpenQA.Selenium
{
Expand All @@ -39,5 +40,13 @@ public interface ICommandExecutor : IDisposable
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A response from the browser</returns>
Response Execute(Command commandToExecute);


/// <summary>
/// Executes a command as an asynchronous task.
/// </summary>
/// <param name="commandToExecute">The command you wish to execute</param>
/// <returns>A task object representing the asynchronous operation</returns>
Task<Response> ExecuteAsync(Command commandToExecute);
}
}
33 changes: 33 additions & 0 deletions dotnet/src/webdriver/INavigation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// </copyright>

using System;
using System.Threading.Tasks;

namespace OpenQA.Selenium
{
Expand All @@ -31,12 +32,24 @@ public interface INavigation
/// </summary>
void Back();

/// <summary>
/// Move back a single entry in the browser's history as an asynchronous task.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
Task BackAsync();

/// <summary>
/// Move a single "item" forward in the browser's history.
/// </summary>
/// <remarks>Does nothing if we are on the latest page viewed.</remarks>
void Forward();

/// <summary>
/// Move a single "item" forward in the browser's history as an asynchronous task.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
Task ForwardAsync();

/// <summary>
/// Load a new web page in the current browser window.
/// </summary>
Expand All @@ -52,6 +65,13 @@ public interface INavigation
/// </remarks>
void GoToUrl(string url);

/// <summary>
/// Navigate to a url as an asynchronous task.
/// </summary>
/// <param name="url">String of where you want the browser to go.</param>
/// <returns>A task object representing the asynchronous operation.</returns>
Task GoToUrlAsync(string url);

/// <summary>
/// Load a new web page in the current browser window.
/// </summary>
Expand All @@ -67,9 +87,22 @@ public interface INavigation
/// </remarks>
void GoToUrl(Uri url);

/// <summary>
/// Navigate to a url as an asynchronous task.
/// </summary>
/// <param name="url">Uri object of where you want the browser to go.</param>
/// <returns>A task object representing the asynchronous operation.</returns>
Task GoToUrlAsync(Uri url);

/// <summary>
/// Refreshes the current page.
/// </summary>
void Refresh();

/// <summary>
/// Reload the current page as an asynchronous task.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
Task RefreshAsync();
}
}
57 changes: 57 additions & 0 deletions dotnet/src/webdriver/IScript.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// <copyright file="IScript.cs" company="WebDriver Committers">
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Threading.Tasks;
using WebDriverBiDi.Log;

namespace OpenQA.Selenium
{
/// <summary>
/// Defines an interface providing access to WebDriverBiDi events in script and log domains.
/// </summary>
[Obsolete("This class is in beta and may change in future releases.")]
public interface IScript
{
/// <summary>
/// Add and remove handlers for console messages.
/// </summary>
[Obsolete("This event is in beta and may change in future releases.")]
event EventHandler<EntryAddedEventArgs> ConsoleMessageHandler;

/// <summary>
/// Add and remove handlers for console messages.
/// </summary>
[Obsolete("This event is in beta and may change in future releases.")]
event EventHandler<EntryAddedEventArgs> JavaScriptErrorHandler;

/// <summary>
/// Asynchronously starts monitoring for console and JavaScript log entries.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
[Obsolete("This task is in beta and may change in future releases.")]
Task StartMonitoringLogEntries();
Copy link
Member

Choose a reason for hiding this comment

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

What's the need for exposing start/stop API? I would probably just start when the first event handler is added and stop when the last one is removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it seems less intuitive to me that you need to register and then explicitly call to start monitoring, but apparently that's what is expected in the .NET world 🤷

Copy link
Member

Choose a reason for hiding this comment

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

This thread promises to be interesting, and this thread is very important.

but apparently that's what is expected in the .NET world

No, native events is a standard implementation of pub/sub pattern, or observable pattern, or any other notification based pattern. Events born at the beginning of .net. Technically you are able to know when somebody subscribes/unsubscribes on the event.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/add - pretty easy.

Now, in 2024, let's think about it deeper:

  • add method is sync, but we need to call another async method. Sync-over-async problem.
  • OK, we know how to call async methods from sync context even when Microsoft doesn't recommend it.
  • What about async event handlers? User wants to call async methods in theirs event handlers. This is async void problem. Again, Microsoft doesn't recommend it. Ah, this is exceptional case, async event handlers... move on.
  • Now, when user is able to define sync/async event handlers, we want to make that all events are dispatched to users, and users handlers are finished. The problem here is how to await async void? Kind of impossible. There is tricky trick, but this trick is very rare in .net world. Nobody wants to code workarounds.

How to be?

await driver.OnConsoleLogEntryAsync(e => Console.WriteLine(e));

await driver.OnBeforeRequestAsync(async req => await req.ContinueAsync());

Why? It is fully manageable!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my first implementation had private variables for the handlers and then reference them to add/remove

 public event EventHandler<EntryAddedEventArgs> ConsoleMessageHandler
        {
            add
            {
                bool startMonitoring = consoleMessageHandler == null;
                consoleMessageHandler += value;
                if (startMonitoring && !isMonitoringStarted)
                {
                    Task.Run(StartMonitoring);
                }
            }
            remove => consoleMessageHandler -= value;
        }

But from what I've seen, .NET practices leans toward explicitness and clarity which means not assuming that adding or removing events will start and stop monitoring. (ChatGPT agrees with this 😁)

I'm not following all of your comment, though (and I'm not well-versed in async approach in general). Is there a use case for where we want to make one of these events manage async actions?

Copy link
Member

@nvborisenko nvborisenko May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For user it doesn't matter what you do behind: starting, stopping, laughing. He just subscribe on the event, what means he wants to get notifications every time when the event is occurred.

Use cases for async event handlers?.. My initial example can be considered as use case:

await driver.OnBeforeRequestAsync(async req => await req.ContinueAsync());

To be more precise:

await context.OnBeforeRequestSentAsync("https://**", async req =>
{
    await req.ContinueAsync("POST");
});

await context.NavigateAsync("https://selenium.dev");

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess my question is whether there is something that can't be done with a sync method, or if it's just that people want to do everything async on principle?

Copy link
Member Author

Choose a reason for hiding this comment

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

With respect to explicit start/stop:

So I took a look at PuppeteerCSharp and Playwright .NET implementations; I don't want to take what they are doing as necessary indications of proper, idiomatic .NET behavior, but, neither of them require an extra step to start monitoring.

I generally see our priorities as:

  1. Language idiomatic
  2. Matches ease of use in alternative projects
  3. Consistent across Selenium bindings

I agree that explicit stop/starts follows the convention of explicitness, but that neither other project requires it and none of the other bindings will do it this way, I'm leaning toward having adding the event start it, but keep the methods for starting and stopping public in case the user wants more control.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as the async implementation, I did some reading, and yes, we need to support it, I'll see about updating this code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, looks like Async works out of the box...

            driver.Script.ConsoleMessageHandler += async (_, msg) =>
            {
                if (msg.Level == WebDriverBiDi.Log.LogLevel.Info)
                {
                    logs.Add(msg);
                }
                await Task.Delay(1000);
            };

Copy link
Member

Choose a reason for hiding this comment

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

Your example is known async void problem. Just try slightly modified handler:

await Task.Delay(1000);
if (msg.Level == WebDriverBiDi.Log.LogLevel.Info)
{
  logs.Add(msg);
}


/// <summary>
/// Asynchronously stops monitoring for console and JavaScript log entries.
/// </summary>
/// <returns>A task object representing the asynchronous operation.</returns>
[Obsolete("This task is in beta and may change in future releases.")]
Task StopMonitoringLogEntries();
}
}
8 changes: 8 additions & 0 deletions dotnet/src/webdriver/IWebDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ public interface IWebDriver : ISearchContext, IDisposable
/// the browser's history and to navigate to a given URL.</returns>
INavigation Navigate();

/// <summary>
/// Provides access to WebDriverBiDi events for Script and Logs.
/// </summary>
/// <returns>An <see cref="IScript"/> object allowing the user to access
/// WebDriverBiDi events.</returns>
[Obsolete("This method is in beta and may change in future releases.")]
IScript Script();

/// <summary>
/// Instructs the driver to send future commands to a different frame or window.
/// </summary>
Expand Down
Loading