[py] Feature 12760 for python service properties by iampopovich · Pull Request #14263 · SeleniumHQ/selenium

iampopovich · 2024-07-14T15:32:46Z

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

qodo-code-review · 2024-07-14T15:33:49Z

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.

qodo-code-review · 2024-07-14T15:34:29Z

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
Enhancement	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 · 2024-07-18T07:43:41Z

@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 · 2024-07-18T08:14:22Z

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

iampopovich · 2024-11-08T08:22:43Z

i'll resolve conflicts later next week

cgoldberg · 2025-06-16T12:55:05Z

this was done in #15889

iampopovich added 8 commits July 14, 2024 21:22

added to chromium.service

d85743a

added to edge.service

03b9d54

added to firefox.service

5e2438c

added to ie.service

4e8358d

added to safari.service

1e2b509

added to webkitgtk.service

494366f

added to wpewebkit.service

4b583ce

applied formatting

5a12a25

qodo-code-review bot added the P-enhancement PR with a new feature label Jul 14, 2024

qodo-code-review bot added the Review effort [1-5]: 3 label Jul 14, 2024

replace Sequence with List and added type hints

c968254

iampopovich changed the title ~~Feature 12760 for python service properties~~ [py] Feature 12760 for python service properties Jul 17, 2024

Merge branch 'trunk' into feat-12760-py-service-prop

697e634

VietND96 added the C-py Python Bindings label Nov 1, 2024

iampopovich added 3 commits December 6, 2024 13:01

Merge branch 'trunk' into feat-12760-py-service-prop

c679fa2

fix for service constructor formatting

3b4c401

Merge branch 'trunk' into feat-12760-py-service-prop

e2fa6b5

github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label Jun 8, 2025

cgoldberg mentioned this pull request Jun 11, 2025

[py] Add properties (getter/setter) for service args #15889

Merged

cgoldberg closed this Jun 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[py] Feature 12760 for python service properties#14263

[py] Feature 12760 for python service properties#14263
iampopovich wants to merge 13 commits intoSeleniumHQ:trunkfrom
iampopovich:feat-12760-py-service-prop

iampopovich commented Jul 14, 2024 •

edited

Loading

Uh oh!

qodo-code-review bot commented Jul 14, 2024

Uh oh!

qodo-code-review bot commented Jul 14, 2024 •

edited

Loading

Uh oh!

iampopovich commented Jul 18, 2024

Uh oh!

iampopovich commented Jul 18, 2024

Uh oh!

iampopovich commented Nov 8, 2024

Uh oh!

cgoldberg commented Jun 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

iampopovich commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-code-review bot commented Jul 14, 2024

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review bot commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

iampopovich commented Jul 18, 2024

Uh oh!

iampopovich commented Jul 18, 2024

Uh oh!

iampopovich commented Nov 8, 2024

Uh oh!

cgoldberg commented Jun 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iampopovich commented Jul 14, 2024 •

edited

Loading

qodo-code-review bot commented Jul 14, 2024 •

edited

Loading