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] remove desired capabilities argument for Webkitgtk #14128

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jun 12, 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.

according to #14087
i removed capabilities and add default Options same as in safari.webdriver class

Description

Motivation and Context

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

Bug fix


Description

  • Removed the desired_capabilities argument from the WebDriver constructor in webkitgtk module.
  • Updated the constructor to use options directly, simplifying the initialization process.
  • Removed the logic for merging desired_capabilities with options capabilities.

Changes walkthrough 📝

Relevant files
Bug fix
webdriver.py
Remove desired_capabilities argument and simplify WebDriver
initialization.

py/selenium/webdriver/webkitgtk/webdriver.py

  • Removed desired_capabilities argument from the WebDriver constructor.
  • Updated the constructor to use options directly.
  • Simplified the initialization process by removing capability merging
    logic.
  • +8/-18   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    remove capabilities and add default Options
    
    same  in safari.webdriver
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    2

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Simplification and Refactoring:
    The PR simplifies the constructor by removing the desired_capabilities argument and refactoring the handling of options. Ensure that the new implementation correctly handles all scenarios previously covered by the combination of options and desired_capabilities.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add exception handling to ensure the Service object is terminated properly in case of initialization failure

    Ensure that the Service object is properly terminated in case of an exception during
    initialization to avoid resource leaks.

    py/selenium/webdriver/webkitgtk/webdriver.py [54]

    -self.service.start()
    +try:
    +    self.service.start()
    +except Exception as e:
    +    self.service.stop()
    +    raise e
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion is highly beneficial as it addresses proper resource management and error handling, which is crucial for maintaining system stability and avoiding resource leaks.

    8
    Possible issue
    Add validation for the executable_path parameter to ensure it is not None or empty

    Add a check to ensure that executable_path is not None or an empty string to avoid
    potential issues when starting the service.

    py/selenium/webdriver/webkitgtk/webdriver.py [52]

    +if not executable_path:
    +    raise ValueError("Executable path must be provided")
     self.service = Service(executable_path, port=port, log_path=service_log_path)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion is valid as it prevents potential runtime errors when executable_path is not provided, enhancing the robustness of the service initialization.

    7

    @titusfortner
    Copy link
    Member

    This does fix the problem described in the issue, but this class is problematic in several ways, still

    It's hard to write this code when we can't have tests because we aren't managing the drivers.

    But I think it needs to look more like:
    https://github.com/SeleniumHQ/selenium/blob/trunk/py/selenium/webdriver/wpewebkit/webdriver.py

    @iampopovich
    Copy link
    Contributor Author

    iampopovich commented Jun 13, 2024

    But I think it needs to look more like

    I'm trying to understand the differences between the classes if I make wpewebkit and webkitgtk identical. I removed parameters such as port, executable_path, and log path because there are default parameters in the Service class, and other driver binds similarly do not use them. However, in this case, the two classes become identical, and I'm not sure if I correctly understood what is required in the comment above.

    classes in the latest commit
    Screenshot 2024-06-13 at 22 32 35
    classes after I applied changes described above
    Screenshot 2024-06-13 at 22 51 28

    @titusfortner
    Copy link
    Member

    It makes sense to me that they are identical if they aren't actually doing anything other than starting/stopping the driver.

    @titusfortner
    Copy link
    Member

    @iampopovich the python linter is failing on some of these changes, can you take a look?

    @iampopovich
    Copy link
    Contributor Author

    @titusfortner Running the script format.sh did not make any corrections, but during the linting stage, isort showed an error. It might be a good idea to add the isort run to the format.sh script.

    @titusfortner
    Copy link
    Member

    From root run ./go py:lint

    The format script is supposed to work without any additional software installed, but we haven't gotten that figured out for python. If we add the python bit, everyone who doesn't have a python dev environment will get failures. There's an open PR for part of it, we need to dig into it again.

    @titusfortner titusfortner merged commit 8b56711 into SeleniumHQ:trunk Jun 20, 2024
    14 checks passed
    @iampopovich iampopovich deleted the remove-desired-capabilities-webkitgtk branch June 20, 2024 13:08
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    …4128)
    
    * initial changes for capabilities removing
    
    remove capabilities and add default Options
    
    same  in safari.webdriver
    
    * make webkitgtk.webdriver same as wpewebkit.webdriver
    
    * fix formatting with isort
    
    * applying formatting with tox
    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.

    2 participants