[py] Feature 12760 for python service properties

[iampopovich]

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

according to feature request #12760
I did the work as in the pull request #12767.
Unfortunately, that pull request was closed due to inactivity and conflicts with the trunk branch.
I checked the build results and test execution here: https://github.com/iampopovich/selenium/actions/runs/9928825190/job/27425721756.

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

Enhancement

---

Description

• Refactored service classes for Chromium, Edge, Firefox, IE, Safari, WebKitGTK, and WPEWebKit to use properties for service arguments.
• Added type hints for List, Mapping, Optional, and Sequence in the service classes.
• Replaced service_args with _service_args and added property methods for better encapsulation.
• Updated command_line_args methods to use _service_args in all service classes.

---

Changes walkthrough 📝

Relevant files

Enhancement

service.py Refactor Chromium service to use properties for service arguments py/selenium/webdriver/chromium/service.py Added type hints for List, Mapping, Optional, and Sequence. Replaced service_args with _service_args and added property methods. Updated command_line_args method to use _service_args. +20/-7    service.py Refactor Edge service to use properties for service arguments py/selenium/webdriver/edge/service.py Added type hints for Mapping, Optional, and Sequence. Replaced service_args with _service_args and added property methods. +17/-5    service.py Refactor Firefox service to use properties for service arguments py/selenium/webdriver/firefox/service.py Added type hints for Mapping, Optional, and Sequence. Replaced service_args with _service_args and added property methods. Updated command_line_args method to use _service_args. +20/-8    service.py Refactor IE service to use properties for service arguments py/selenium/webdriver/ie/service.py Added type hints for Optional and Sequence. Replaced service_args with _service_args and added property methods. Updated command_line_args method to use _service_args. +19/-8    service.py Refactor Safari service to use properties for service arguments py/selenium/webdriver/safari/service.py Added type hints for List, Mapping, Optional, and Sequence. Replaced service_args with _service_args and added property methods. Updated command_line_args method to use _service_args. +19/-6    service.py Refactor WebKitGTK service to use properties for service arguments py/selenium/webdriver/webkitgtk/service.py Added type hints for List, Mapping, Optional, and Sequence. Replaced service_args with _service_args and added property methods. Updated command_line_args method to use _service_args. +20/-7    service.py Refactor WPEWebKit service to use properties for service arguments py/selenium/webdriver/wpewebkit/service.py Added type hints for List, Mapping, Optional, and Sequence. Replaced service_args with _service_args and added property methods. Updated command_line_args method to use _service_args. +20/-7

---

💡 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]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Type Consistency The type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required. Type Consistency Similar to the chromium/service.py, the type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required. Type Consistency The type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required. Type Consistency The type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required. Type Consistency The type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required. Type Consistency The type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required. Type Consistency The type hint for service_args was changed from List[str] to Sequence[str], but the setter method still checks if the value is an instance of Sequence. This could potentially allow non-list sequences (like tuples) which might not be intended. Consider enforcing List[str] if mutability is required.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add error handling for opening the log file to prevent crashes Add error handling for file operations when opening the log file to prevent the program from crashing if the file cannot be opened. py/selenium/webdriver/webkitgtk/service.py [48] -log_file = open(log_path, "wb") if log_path else None +try: + log_file = open(log_path, "wb") if log_path else None +except IOError as e: + log_file = None + print(f"Failed to open log file {log_path}: {e}") Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding error handling for file operations is crucial to prevent the program from crashing if the file cannot be opened, improving the robustness of the code.	9
Possible bug	Ensure "--websocket-port" is appended correctly to service_args Modify the condition to check for the presence of "--connect-existing" in service_args to ensure it only appends "--websocket-port" when necessary, avoiding potential duplication or logic errors. py/selenium/webdriver/firefox/service.py [58-60] if "--connect-existing" not in self._service_args: - self._service_args.append("--websocket-port") - self._service_args.append(f"{utils.free_port()}") + self._service_args.extend(["--websocket-port", f"{utils.free_port()}"]) Apply this suggestion Suggestion importance[1-10]: 8 Why: Using extend instead of multiple append calls ensures that the arguments are added in a single operation, reducing the risk of duplication and improving code readability.	8
Enhancement	Improve type hint specificity for the env parameter Consider using a more specific type hint for env parameter in the constructor. Instead of Mapping[str, str], use Dict[str, str] which is more appropriate for environments that are expected to be mutable and directly accessible. py/selenium/webdriver/chromium/service.py [43] -env: Optional[Mapping[str, str]] = None, +env: Optional[Dict[str, str]] = None, Apply this suggestion Suggestion importance[1-10]: 7 Why: Using Dict instead of Mapping can be more appropriate for environments expected to be mutable and directly accessible, enhancing code clarity and correctness.	7
	Refactor command_line_args to use list comprehension for clarity and performance Refactor the command_line_args method to use list comprehension for building the command line arguments, which can enhance readability and performance. py/selenium/webdriver/ie/service.py [62-63] def command_line_args(self) -> List[str]: - return [f"--port={self.port}"] + self._service_args + return [f"--port={self.port}", *self._service_args] Apply this suggestion Suggestion importance[1-10]: 6 Why: Using list comprehension can enhance readability and potentially improve performance, though the improvement is minor.	6

[iampopovich]

@pujagani rbe test failed

//py:common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py       FAILED in 2 out of 2 in 13.9s
  Stats over 2 runs: max = 13.9s, min = 11.9s, avg = 12.9s, dev = 1.0s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/py/common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py/test_attempts/attempt_1.log

I'll try to figure out what went wrong

[iampopovich]

hm... I ran several failing tests on my local environment, and the tests passed successfully.

INFO: Analyzed target //py:common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
Target //py:common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py up-to-date:
  bazel-bin/py/common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py-runner.py
  bazel-bin/py/common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py
INFO: Elapsed time: 22.731s, Critical Path: 18.68s
INFO: 2 processes: 2 local.
INFO: Build completed successfully, 2 total actions
//py:common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py       PASSED in 17.7s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
(selenium-env) ➜  selenium git:(trunk) ✗ bazel test //py:common-edge-bidi-test/selenium/webdriver/common/frame_switching_tests.py
INFO: Analyzed target //py:common-edge-bidi-test/selenium/webdriver/common/frame_switching_tests.py (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
Target //py:common-edge-bidi-test/selenium/webdriver/common/frame_switching_tests.py up-to-date:
  bazel-bin/py/common-edge-bidi-test/selenium/webdriver/common/frame_switching_tests.py-runner.py
  bazel-bin/py/common-edge-bidi-test/selenium/webdriver/common/frame_switching_tests.py
INFO: Elapsed time: 106.817s, Critical Path: 105.79s
INFO: 2 processes: 2 local.
INFO: Build completed successfully, 2 total actions
//py:common-edge-bidi-test/selenium/webdriver/common/frame_switching_tests.py PASSED in 104.8s