Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dotnet] allow RemoteWebDriver to access Selenium logs #10671

Merged
merged 1 commit into from
May 25, 2022

Conversation

titusfortner
Copy link
Member

There are 3 options I can see for quickly fixing #8229, none of them are ideal.

  1. Require users to subclass RemoteWebDriver and add implementation for ISupportsLog which I *think should "just work" (at least it did in my quick test of it).
public class RemoteChromeDriver : RemoteWebDriver, ISupportsLogs
  1. This PR, which would allow a user to do this:
var customCommandDriver = driver as ICustomDriverCommandExecutor;
customCommandDriver.RegisterCustomDriverCommands(ChromeDriver.CustomCommandDefinitions);

ReadOnlyCollection<string> logTypes = driver.Manage().Logs.AvailableLogTypes;
driver.Url = "https://selenium.dev/selenium/web/errors.html";
driver.FindElement(By.CssSelector("input")).Click();
ReadOnlyCollection<LogEntry> results = driver.Manage().Logs.GetLog("browser");
  1. Can remove some of the uglier parts of this PR and put more onus on user:
IReadOnlyDictionary<string, CommandInfo> logCommands = new Dictionary<string, CommandInfo>
{
    {
        DriverCommand.GetAvailableLogTypes,
        new HttpCommandInfo(HttpCommandInfo.GetCommand, "/session/{sessionId}/se/log/types")
    },
    {
        DriverCommand.GetLog,
        new HttpCommandInfo(HttpCommandInfo.PostCommand, "/session/{sessionId}/se/log")
    }
};

var customCommandDriver = driver as ICustomDriverCommandExecutor;
customCommandDriver.RegisterCustomDriverCommands(logCommands);

ReadOnlyCollection<string> logTypes = driver.Manage().Logs.AvailableLogTypes;
driver.Url = "https://selenium.dev/selenium/web/errors.html";
driver.FindElement(By.CssSelector("input")).Click();
ReadOnlyCollection<LogEntry> results = driver.Manage().Logs.GetLog("browser");

The existing code allows driver classes for drivers that haven't implemented the logging endpoints, to call the log methods, but it won't actually send the commands. No error that the method isn't actually doing anything.

This PR would always send the command but will swallow the not-implemented exception if the driver commands were not added. So same end result, but slightly less efficient, but if a user is calling for something that never returns anything anyway, maybe that doesn't matter from an implementation standpoint.

Any feedback from anyone would be great on this, thanks.

@titusfortner titusfortner requested a review from jimevans May 20, 2022 05:38
@titusfortner titusfortner marked this pull request as draft May 24, 2022 15:04
@titusfortner
Copy link
Member Author

It's easier to add things than take them away, so I want to do the easier code/harder user scenario for this and go from there....

@titusfortner
Copy link
Member Author

Scaled down the PR for minimal change
Existing code prevents executing log methods unless the instance implements the interface.
Only Chromium implements the interface, and Remote can't implement it, so even using the Custom Driver Command Executor to add the commands, they were blocked.
So, this removes the block, and if it is a local Chromium browser, the methods will be automatically added and it will work; and if it is a Remote driver and the user has added the commands, it will work; and if neither of those things is true, the code swallows the error (which effectively current behavior).

@titusfortner titusfortner marked this pull request as ready for review May 25, 2022 02:37
@titusfortner titusfortner merged commit 9e53690 into SeleniumHQ:trunk May 25, 2022
elgatov pushed a commit to elgatov/selenium that referenced this pull request Jun 27, 2022
@titusfortner titusfortner deleted the supports_logs branch December 4, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant