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

[py] add execute_cdp_cmd to Remote #14809

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Nov 26, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Implements execute_cdp_cmd for Remote WebDriver

Motivation and Context

Implements #14799 as it should be accessible via Remote driver as well since Grid provides the endpoints and the underlying command executor

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

  • Introduced the execute_cdp_cmd method to the Remote WebDriver class, enabling the execution of Chrome DevTools Protocol commands.
  • This enhancement allows users to execute specific browser commands directly through the Remote WebDriver, similar to the existing functionality in the Chrome WebDriver.
  • The method takes a command name and a dictionary of command arguments, returning the result of the command execution.

Changes walkthrough 📝

Relevant files
Enhancement
webdriver.py
Add `execute_cdp_cmd` method to Remote WebDriver                 

py/selenium/webdriver/remote/webdriver.py

  • Added execute_cdp_cmd method to the Remote WebDriver class.
  • The method allows execution of Chrome DevTools Protocol commands.
  • Provides a way to pass command name and arguments.
  • Returns the result of the command execution.
  • +20/-0   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14799 - Fully compliant

    Compliant requirements:

    • Added execute_cdp_cmd method to Remote WebDriver
    • Method uses execute("executeCdpCommand") internally
    • Implementation matches Chrome WebDriver's functionality
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The execute_cdp_cmd method should include error handling for invalid commands or parameters

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add input validation to prevent runtime errors from invalid arguments

    Add error handling to validate that cmd is a non-empty string and cmd_args is a
    dictionary before execution, to prevent runtime errors.

    py/selenium/webdriver/remote/webdriver.py [364-382]

     def execute_cdp_cmd(self, cmd: str, cmd_args: dict):
    +    if not cmd or not isinstance(cmd, str):
    +        raise ValueError("Command must be a non-empty string")
    +    if not isinstance(cmd_args, dict):
    +        raise ValueError("Command arguments must be a dictionary")
         return self.execute("executeCdpCommand", {"cmd": cmd, "params": cmd_args})["value"]
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial input validation to prevent potential runtime errors and provides clear error messages, which is especially important for a public API method that interfaces with Chrome DevTools Protocol.

    8

    💡 Need additional feedback ? start a PR chat

    @VietND96
    Copy link
    Member

    Do you think we should add a test for it?

    @Delta456
    Copy link
    Contributor Author

    I don't see any tests of execute_cdp_cmd for ChromiumRemoteConnection. I don't know how will this be tested too.

    @VietND96
    Copy link
    Member

    VietND96 commented Dec 6, 2024

    @p0deje, do you think this is fine to merge?

    @p0deje
    Copy link
    Member

    p0deje commented Dec 6, 2024

    @VietND96 Doesn't this create a duplication as we already have the same method in ChromiumDriver? I am not sure how to re-use it, but maybe we can at least remove it from ChromiumDriver.

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 6, 2024

    Let's add a deprecation warning before removing it?

    @VietND96
    Copy link
    Member

    VietND96 commented Dec 6, 2024

    @Delta456, I saw class ChromiumDriver(RemoteWebDriver):, do you think can make Chromium can access and reuse the same method from Remote?

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 8, 2024

    I will try

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 8, 2024

    @Delta456, I saw class ChromiumDriver(RemoteWebDriver):, do you think can make Chromium can access and reuse the same method from Remote?

    Yes, we can use the same method from Remote. I think we can merge this for now.

    We can think of a deprecation warning afterwards.

    @VietND96 VietND96 merged commit 4bb5353 into SeleniumHQ:trunk Dec 9, 2024
    16 of 17 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Dec 9, 2024

    Thank you, @Delta456 !

    @Delta456 Delta456 deleted the remote_cdp branch December 9, 2024 09:56
    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.

    3 participants