[py] remove desired capabilities argument for Webkitgtk

iampopovich commented 2 weeks ago

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

[x] 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

[x] 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.
[x] 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

codiumai-pr-agent-pro[bot] commented 2 weeks ago

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`.

codiumai-pr-agent-pro[bot] commented 2 weeks ago

PR Code Suggestions ✨

Category	Suggestion	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]](https://github.com/SeleniumHQ/selenium/pull/14128/files#diff-0d84f899f993a1342a3a2e75a3ef749fb36e08dd022b607a7ab81beda9cfcbfcR54-R54) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14128/files#diff-0d84f899f993a1342a3a2e75a3ef749fb36e08dd022b607a7ab81beda9cfcbfcR52-R52) ```diff +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 commented 2 weeks ago

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 commented 2 weeks ago

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

classes after I applied changes described above

titusfortner commented 2 weeks ago

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

titusfortner commented 1 week ago

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

iampopovich commented 1 week ago

@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 commented 1 week ago

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.

SeleniumHQ / selenium