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] Support GetLog command by Remote Web Driver #14549

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 1, 2024

User description

Description

Now it is possible to driver.Manage().Logs.GetLog("browser") where driver is remote webdriver.

Motivation and Context

Fixes #14545

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Added support for the GetLog command in the Remote WebDriver, allowing retrieval of logs such as "browser" logs.
  • Implemented the GetAvailableLogTypes command to fetch available log types.

Changes walkthrough 📝

Relevant files
Enhancement
W3CWireProtocolCommandInfoRepository.cs
Add support for log retrieval commands in Remote WebDriver

dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs

  • Added support for the GetLog command in the Remote WebDriver.
  • Added support for the GetAvailableLogTypes command.
  • +2/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 1, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Compatibility Issue
    The new commands (GetLog and GetAvailableLogTypes) are added under a comment stating they are not part of the W3C specification. This might lead to compatibility issues with other WebDriver implementations.

    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 1, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling for command registration to catch issues early

    Consider adding error handling or validation for the TryAddCommand method calls.
    This could help catch any issues with command registration early and provide more
    informative error messages.

    dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs [136-137]

    -this.TryAddCommand(DriverCommand.GetLog, new HttpCommandInfo(HttpCommandInfo.PostCommand, "/session/{sessionId}/se/log"));
    -this.TryAddCommand(DriverCommand.GetAvailableLogTypes, new HttpCommandInfo(HttpCommandInfo.GetCommand, "/session/{sessionId}/se/log/types"));
    +if (!this.TryAddCommand(DriverCommand.GetLog, new HttpCommandInfo(HttpCommandInfo.PostCommand, "/session/{sessionId}/se/log")))
    +{
    +    throw new InvalidOperationException($"Failed to add command {DriverCommand.GetLog}");
    +}
    +if (!this.TryAddCommand(DriverCommand.GetAvailableLogTypes, new HttpCommandInfo(HttpCommandInfo.GetCommand, "/session/{sessionId}/se/log/types")))
    +{
    +    throw new InvalidOperationException($"Failed to add command {DriverCommand.GetAvailableLogTypes}");
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the TryAddCommand method calls is a valuable enhancement. It helps catch potential issues early and provides informative error messages, improving robustness and debugging.

    8
    Best practice
    Use constants for endpoint paths to improve maintainability and reduce the risk of errors

    Consider using constants for the endpoint paths instead of hardcoding them directly
    in the method calls. This would improve maintainability and reduce the risk of typos
    in the URLs.

    dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs [136-137]

    -this.TryAddCommand(DriverCommand.GetLog, new HttpCommandInfo(HttpCommandInfo.PostCommand, "/session/{sessionId}/se/log"));
    -this.TryAddCommand(DriverCommand.GetAvailableLogTypes, new HttpCommandInfo(HttpCommandInfo.GetCommand, "/session/{sessionId}/se/log/types"));
    +const string LogEndpoint = "/session/{sessionId}/se/log";
    +const string LogTypesEndpoint = "/session/{sessionId}/se/log/types";
    +this.TryAddCommand(DriverCommand.GetLog, new HttpCommandInfo(HttpCommandInfo.PostCommand, LogEndpoint));
    +this.TryAddCommand(DriverCommand.GetAvailableLogTypes, new HttpCommandInfo(HttpCommandInfo.GetCommand, LogTypesEndpoint));
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using constants for endpoint paths enhances maintainability and reduces the likelihood of typos or errors in URLs. This is a good practice for improving code quality and readability.

    7
    Documentation
    Add a comment explaining the inclusion of non-W3C specification commands

    Consider adding a comment explaining why these commands are not part of the W3C
    specification but are still included. This would provide valuable context for future
    maintainers.

    dotnet/src/webdriver/Remote/W3CWireProtocolCommandInfoRepository.cs [131-133]

     // Commands below here are not included in the W3C specification,
     // but are required for full fidelity of execution with Selenium's
     // local-end implementation of WebDriver.
    +// The GetLog and GetAvailableLogTypes commands are included here to support
    +// logging functionality in the Remote WebDriver, which is essential for
    +// debugging and monitoring WebDriver sessions.
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a comment to explain the inclusion of non-W3C specification commands provides valuable context for future maintainers. It enhances documentation and understanding of the codebase.

    6

    💡 Need additional feedback ? start a PR chat

    Copy link
    Contributor

    @pujagani pujagani left a comment

    Choose a reason for hiding this comment

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

    LGTM. Thank you!

    @pujagani
    Copy link
    Contributor

    pujagani commented Oct 3, 2024

    There a bunch of failing tests though I am not sure if they are related to the changes in this PR.

    @nvborisenko
    Copy link
    Member Author

    Failed tests are not related to this PR. Probably I will be able to look at this later (as separate PR).

    @nvborisenko
    Copy link
    Member Author

    Tests are failing because of Edge 128 is used, when 129 is expected. #14411 should fix it.

    @nvborisenko nvborisenko merged commit 10d5f8c into SeleniumHQ:trunk Oct 3, 2024
    8 of 10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-remotedriver branch October 3, 2024 18:38
    @sponnusamyneogov
    Copy link

    Thank you so much!@nvborisenko

    @nvborisenko
    Copy link
    Member Author

    @sponnusamyneogov, you will be able to test the changes in upcoming nightly build, don't forget to share results.

    @hatemkahil
    Copy link

    Hi, unfortunately it looks like this change has broken the existing functionality of driver.Manage().Logs.GetLog("browser").

    We were using this for the last 2 years and it was working fine for both our Chrome and Safari (through testingbot.com) tests. After the 4.26 update, we started getting the below exception when we use it on the Safari (through testingbot.com) tests. It's working fine on Chrome.

    System.NotImplementedException: The command 'POST /session/ef0dd34ce41e-3063933af011-5b4c8dca3c44-173394094693-47367542/se/log' was not found. at OpenQA.Selenium.WebDriver.UnpackAndThrowOnError(Response errorResponse, String commandToExecute) at OpenQA.Selenium.WebDriver.ExecuteAsync(String driverCommandToExecute, Dictionary2 parameters)
    at OpenQA.Selenium.WebDriver.InternalExecute(String driverCommandToExecute, Dictionary2 parameters) at OpenQA.Selenium.Logs.GetLog(String logKind)

    Please let me know if I can provide more info or if we need to change anything to make this work.

    Thanks.

    @nvborisenko
    Copy link
    Member Author

    @hatemkahil you said: "it was working fine for both our Chrome and Safari". It was incorrect, now it is correct. Now it throws exception, instead of returning empty list of log messages. Please submit new issue if you still think it is incorrect.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Console logs returned as zero using RemoteDriver (SeleniumGrid) .NET Selenium
    4 participants