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

[🚀 Feature] [py]: Better compatibility with Appium-python #14587

Merged
merged 18 commits into from
Oct 21, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Oct 11, 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

Fixes #13959

This PR implements extensibility points that would be useful in Appium-python client.

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, Tests


Description

  • Introduced extensibility for custom locator strategies and web element classes in Selenium WebDriver.
  • Enhanced RemoteConnection to support extra headers, custom user-agent, and additional HTTP commands.
  • Implemented LocatorConverter for flexible locator conversion logic.
  • Added comprehensive tests for new features, including custom locators, elements, and connection settings.

Changes walkthrough 📝

Relevant files
Enhancement
by.py
Add support for custom locator strategies                               

py/selenium/webdriver/common/by.py

  • Added methods to register, retrieve, and clear custom finders.
  • Introduced a dictionary to store custom finders.
  • +16/-0   
    locator_converter.py
    Introduce LocatorConverter for custom locator conversion 

    py/selenium/webdriver/remote/locator_converter.py

  • Introduced a new LocatorConverter class.
  • Implemented default conversion logic for locators.
  • +28/-0   
    remote_connection.py
    Enhance RemoteConnection with custom headers and commands

    py/selenium/webdriver/remote/remote_connection.py

  • Added support for extra headers and custom user-agent.
  • Enabled ignoring certificates in connection manager.
  • Allowed registration of extra HTTP commands.
  • +28/-3   
    webdriver.py
    Enhance WebDriver with custom locators and elements           

    py/selenium/webdriver/remote/webdriver.py

  • Integrated LocatorConverter for element finding.
  • Added support for custom web element classes.
  • +10/-20 
    Tests
    driver_element_finding_tests.py
    Add tests for custom locator strategies                                   

    py/test/selenium/webdriver/common/driver_element_finding_tests.py

  • Added tests for registering and retrieving custom finders.
  • Included tests for clearing custom finders.
  • +18/-0   
    custom_element_tests.py
    Add tests for custom web element classes                                 

    py/test/selenium/webdriver/remote/custom_element_tests.py

  • Added tests for using custom web element classes.
  • Verified default and custom element class behavior.
  • +61/-0   
    remote_custom_locator_tests.py
    Add tests for custom locator conversion                                   

    py/test/selenium/webdriver/remote/remote_custom_locator_tests.py

  • Added tests for custom locator conversion.
  • Verified custom locator logic in element finding.
  • +54/-0   
    remote_connection_tests.py
    Add tests for RemoteConnection enhancements                           

    py/test/unit/selenium/webdriver/remote/remote_connection_tests.py

  • Added tests for custom commands and headers in RemoteConnection.
  • Verified user-agent override and certificate ignoring.
  • +62/-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:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The ignore_certificates option in the RemoteConnection class (line 248 of py/selenium/webdriver/remote/remote_connection.py) allows bypassing SSL certificate verification. This could potentially expose the application to man-in-the-middle attacks if used inappropriately. It's crucial to ensure that this option is only used in controlled environments and never in production settings.

    ⚡ Recommended focus areas for review

    Security Concern
    The ignore_certificates option allows bypassing SSL certificate verification, which could potentially lead to security vulnerabilities if used improperly.

    Performance Impact
    The addition of extra headers and custom commands might slightly increase the overhead for each request. The impact on performance should be evaluated, especially for high-volume scenarios.

    Code Complexity
    The introduction of LocatorConverter and custom web element classes adds flexibility but also increases the complexity of the element finding process. This might make debugging and maintenance more challenging.

    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 11, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Implement a more secure approach for certificate verification instead of disabling it entirely

    Consider using a more secure method for handling certificate verification. Instead
    of disabling certificate verification entirely, you could implement a custom
    certificate verification function or use a trusted certificate authority.

    py/selenium/webdriver/remote/remote_connection.py [247-249]

     if self._ignore_certificates:
    -    pool_manager_init_args["cert_reqs"] = "CERT_NONE"
    -    urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
    +    pool_manager_init_args["cert_reqs"] = "CERT_REQUIRED"
    +    pool_manager_init_args["cert_file"] = "/path/to/custom/cert.pem"
    +    pool_manager_init_args["assert_hostname"] = False
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a significant security concern by proposing a more secure method for handling certificate verification, which is crucial for maintaining secure connections. This change enhances the security of the application by avoiding the complete disabling of certificate verification.

    9
    Enhancement
    Implement more robust error handling and logging in the execute method to improve debugging and error reporting

    Consider implementing a more robust error handling mechanism for the execute method,
    such as custom exceptions for different types of errors or a logging system for
    better debugging.

    py/selenium/webdriver/remote/remote_connection.py [308-323]

     def execute(self, command, params):
         """Send a command to the remote server.
     
         Any path substitutions required for the URL mapped to the command should be
         included in the command parameters.
     
         :Args:
          - command - A string specifying the command to execute.
          - params - A dictionary of named parameters to send with the command as
            its JSON payload.
    +    :Raises:
    +     - UnknownCommandError: If the command is not recognized.
    +     - CommandExecutionError: If there's an error during command execution.
         """
    -    command_info = self._commands.get(command) or self.extra_commands.get(command)
    -    assert command_info is not None, f"Unrecognised command {command}"
    -    path_string = command_info[1]
    -    path = string.Template(path_string).substitute(params)
    +    try:
    +        command_info = self._commands.get(command) or self.extra_commands.get(command)
    +        if command_info is None:
    +            raise UnknownCommandError(f"Unrecognised command {command}")
    +        path_string = command_info[1]
    +        path = string.Template(path_string).substitute(params)
    +        # ... rest of the method implementation
    +    except Exception as e:
    +        logging.error(f"Error executing command {command}: {str(e)}")
    +        raise CommandExecutionError(f"Error executing {command}: {str(e)}") from e
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves error handling by introducing custom exceptions and logging, which aids in debugging and provides clearer error reporting, enhancing the robustness of the code.

    8
    Implement dedicated methods for managing custom headers to improve flexibility and maintainability

    Consider using a more robust method for handling custom headers, such as a dedicated
    method for adding and removing headers, instead of directly modifying the
    extra_headers class variable.

    py/selenium/webdriver/remote/remote_connection.py [147]

    -extra_headers = None
    +@classmethod
    +def add_header(cls, key, value):
    +    if cls.extra_headers is None:
    +        cls.extra_headers = {}
    +    cls.extra_headers[key] = value
     
    +@classmethod
    +def remove_header(cls, key):
    +    if cls.extra_headers and key in cls.extra_headers:
    +        del cls.extra_headers[key]
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code maintainability and flexibility by proposing dedicated methods for managing custom headers, which is a more organized approach than directly modifying class variables.

    7
    ✅ Implement a more flexible system for managing custom commands to improve extensibility
    Suggestion Impact:The commit changed the `add_command` method to store commands in a dictionary format, aligning with the suggestion to use a more flexible system for managing custom commands.

    code diff:

    -    @classmethod
    -    def add_command(cls, name, method, url):
    +    def add_command(self, name, method, url):
             """Register a new command."""
    -        cls.extra_commands[name] = (method, url)
    +        self._commands[name] = (method, url)

    Consider using a more flexible approach for custom command registration, such as a
    dictionary-based system that allows for easier management and modification of
    commands.

    py/selenium/webdriver/remote/remote_connection.py [303-306]

     @classmethod
     def add_command(cls, name, method, url):
         """Register a new command."""
    -    cls.extra_commands[name] = (method, url)
    +    cls.extra_commands[name] = {"method": method, "url": url}
     
    +@classmethod
    +def remove_command(cls, name):
    +    """Remove a registered command."""
    +    if name in cls.extra_commands:
    +        del cls.extra_commands[name]
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion enhances the extensibility of the code by recommending a dictionary-based system for managing custom commands, which allows for easier modifications and management of commands.

    6

    💡 Need additional feedback ? start a PR chat

    @navin772
    Copy link
    Contributor Author

    @p0deje @KazuCocoa what do you think of the changes, would love to get your feedback on this!

    @p0deje
    Copy link
    Member

    p0deje commented Oct 11, 2024

    @navin772 The changes look good to me, I only wonder if we need to make _web_element_cls public. The rest is good as long as @KazuCocoa confirms it's fine for Appium.

    Copy link
    Contributor

    @KazuCocoa KazuCocoa left a comment

    Choose a reason for hiding this comment

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

    Left a couple of comments.
    I tried to integrate roughly a bit as https://github.com/appium/python-client/compare/selenium?expand=1

    Thank you!

    py/selenium/webdriver/remote/webdriver.py Show resolved Hide resolved
    py/selenium/webdriver/remote/webdriver.py Show resolved Hide resolved
    py/selenium/webdriver/remote/remote_connection.py Outdated Show resolved Hide resolved
    @navin772
    Copy link
    Contributor Author

    @KazuCocoa Thanks for the review, I have updated the PR with the requested changes.

    Copy link
    Contributor

    @KazuCocoa KazuCocoa left a comment

    Choose a reason for hiding this comment

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

    Thank you. I left two comments.

    I have also updated appium/python-client#1045 with this PR (and init_args_for_pool_manager modification in this comment)

    py/selenium/webdriver/remote/remote_connection.py Outdated Show resolved Hide resolved
    @navin772
    Copy link
    Contributor Author

    @KazuCocoa updated the PR with the requested changes.

    Copy link
    Contributor

    @KazuCocoa KazuCocoa left a comment

    Choose a reason for hiding this comment

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

    Thank you! LGTM
    cc @p0deje

    @KazuCocoa
    Copy link
    Contributor

    Lgtm

    @VietND96 VietND96 merged commit 1345193 into SeleniumHQ:trunk Oct 21, 2024
    13 checks passed
    @VietND96
    Copy link
    Member

    Thank you, @navin772 !

    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.

    [🚀 Feature]: Better compatibility with Appium in Python
    6 participants