Skip to content

Conversation

pallavigitwork
Copy link
Member

@pallavigitwork pallavigitwork commented Sep 9, 2025

User description

🔗 Related Issues

#15697

💥 What does this PR do?

this PR fixes the mypy errors of the chromium, webdriver.py file .

🔧 Implementation Notes

The mypy errors were resolved by using ChromiumService, instead Service, and
ChromiumOptions instead of ArgOptions

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Fixed mypy type errors in chromium webdriver

  • Replaced generic types with specific chromium types

  • Added proper default value handling for parameters

  • Improved type safety with string conversions


Diagram Walkthrough

flowchart LR
  A["Generic Types"] --> B["Chromium-Specific Types"]
  B --> C["Type Safety Fixed"]
  D["None Parameters"] --> E["Default Values"]
  E --> C
Loading

File Walkthrough

Relevant files
Bug fix
webdriver.py
Fix mypy errors with chromium-specific types                         

py/selenium/webdriver/chromium/webdriver.py

  • Replaced generic Service and ArgOptions imports with ChromiumService
    and ChromiumOptions
  • Updated constructor parameters to use specific chromium types with
    Optional typing
  • Added default value initialization for service and options parameters
  • Added string type conversion for browser_name and vendor_prefix
    parameters
+10/-7   

@selenium-ci selenium-ci added the C-py Python Bindings label Sep 9, 2025
Copy link
Contributor

qodo-merge-pro bot commented Sep 9, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Diagnose and resolve repeated "ConnectFailure" when creating multiple ChromeDriver instances.
  • Ensure subsequent ChromeDriver instances connect reliably.
  • Provide a fix or guidance that prevents connection refusals across instances.
  • Validate behavior on Linux environment similar to the one described.

Requires further human verification:

  • Reproduce the multi-instantiation scenario on Ubuntu/Linux to verify connection stability.
  • Run end-to-end tests to confirm no connection refusal errors occur after changes.

1234 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

  • Restore click() behavior triggering JS in href for Firefox.
  • Address regression between 2.47.1 and 2.48.x.

Requires further human verification:

  • Manual browser testing to validate click behavior triggers JS alerts in Firefox.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Backwards Compatibility

Default-initializing ChromiumService and ChromiumOptions when None changes behavior from previously requiring explicit options; confirm this doesn't mask configuration errors and aligns with other drivers' constructors.

self.service = service if service else ChromiumService()
options = options if options else ChromiumOptions()
None handling

Casting browser_name and vendor_prefix to str may pass the literal 'None' to ChromiumRemoteConnection when parameters are omitted; verify that None should be allowed instead of 'None' strings.

browser_name=str(browser_name),
vendor_prefix=str(vendor_prefix),
keep_alive=keep_alive,

Copy link
Contributor

qodo-merge-pro bot commented Sep 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve None for optional fields
Suggestion Impact:The commit removed str() wrapping and passed browser_name and vendor_prefix directly to ChromiumRemoteConnection, aligning with the suggestion to avoid converting None to "None".

code diff:

         executor = ChromiumRemoteConnection(
             remote_server_addr=self.service.service_url,
-            browser_name=str(browser_name),
-            vendor_prefix=str(vendor_prefix),
+            browser_name=browser_name,
+            vendor_prefix=vendor_prefix,
             keep_alive=keep_alive,

Avoid coercing optional values to strings, since str(None) becomes the literal
"None", which will be sent to the remote as an invalid value. Pass these through
unchanged so None remains None and only actual strings are forwarded.

py/selenium/webdriver/chromium/webdriver.py [63-69]

 executor = ChromiumRemoteConnection(
             remote_server_addr=self.service.service_url,
-            browser_name=str(browser_name),
-            vendor_prefix=str(vendor_prefix),
+            browser_name=browser_name,
+            vendor_prefix=vendor_prefix,
             keep_alive=keep_alive,
             ignore_proxy=options._ignore_local_proxy,
         )

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that casting None to a string results in the literal "None", which is likely an invalid value for browser_name and vendor_prefix, thus fixing a bug introduced in the PR.

High
  • Update

@cgoldberg cgoldberg changed the title fixed mypy errors in the chromium webdriver file [py] Fix mypy errors in the chromium webdriver file Sep 9, 2025
@pallavigitwork
Copy link
Member Author

@cgoldberg kindly let me know if any more change is required?

Copy link
Contributor

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks.

@cgoldberg cgoldberg changed the title [py] Fix mypy errors in the chromium webdriver file [py] Fix type annotations in the chromium webdriver file Sep 15, 2025
@cgoldberg cgoldberg merged commit 3d16e7c into SeleniumHQ:trunk Sep 15, 2025
16 checks passed
@pallavigitwork
Copy link
Member Author

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants