Skip to content

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Nov 30, 2025

User description

DON'T MERGE UNTIL #15755 IS COMPLETE!!!

(it will probably fail CI until then)


🔗 Related Issues

This implements the Python portion of #15754
This corresponds to the change in #15755

💥 What does this PR do?

This PR adds the --selenium-version argument when calling the Selenium Manager binary.

💡 Additional Considerations

Our current process is to bump the version number after a release.

i.e. Currently the latest released version is 4.38.0 and the development version is 4.39.0.202510242320. Next release will be 4.39.0.

This is pretty confusing, and nightly builds and development versions will be reporting stats to plausible corresponding to version 4.39.0.XXXXXXXXXXXX, even though 4.39.0 has not been released.

Do we really want that, or should be reconsider how we number development versions? I don't know how other languages usually handle this, but the convention in Python would be to append dev to the version number while in development.

i.e. After 4.38.0 is released, the version gets bumped to 4.39.dev until 4.39.0 is released.

https://packaging.python.org/en/latest/specifications/version-specifiers/#development-release-separators

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Adds --selenium-version argument to Selenium Manager binary calls

  • Passes current Python Selenium version to Selenium Manager

  • Imports VERSION from selenium.webdriver module

  • Updates method docstring to reflect actual functionality


Diagram Walkthrough

flowchart LR
  A["SeleniumManager.binary_paths()"] -->|imports| B["VERSION from selenium.webdriver"]
  A -->|appends args| C["--selenium-version"]
  C -->|passes| D["VERSION to Selenium Manager binary"]
Loading

File Walkthrough

Relevant files
Enhancement
selenium_manager.py
Add version argument to Selenium Manager invocation           

py/selenium/webdriver/common/selenium_manager.py

  • Imports VERSION from selenium.webdriver module
  • Adds --selenium-version and VERSION arguments to Selenium Manager
    command line
  • Updates binary_paths() method docstring to accurately describe
    functionality
  • Adds blank line after license header for code formatting
+6/-1     

@cgoldberg cgoldberg requested a review from bonigarcia November 30, 2025 20:39
@cgoldberg cgoldberg self-assigned this Nov 30, 2025
@cgoldberg cgoldberg added the C-py Python Bindings label Nov 30, 2025
@qodo-code-review qodo-code-review bot changed the title [py] Add --selenium-version to Selenium Manager args [py] Add --selenium-version to Selenium Manager args Nov 30, 2025
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The added code invokes an external binary with modified arguments but does not add or
verify any auditing/logging of this critical action or its outcome.

Referred Code
args = [str(self._get_binary())] + args
if logger.getEffectiveLevel() == logging.DEBUG:
    args.append("--debug")
args.append("--language-binding")
args.append("python")
args.append("--selenium-version")
args.append(VERSION)
args.append("--output")
args.append("json")

return self._run(args)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing validation: The new arguments (including '--selenium-version' with VERSION) are appended
without validation or error handling specific to failures related to invalid/missing
version values from the external dependency.

Referred Code
args.append("--selenium-version")
args.append(VERSION)
args.append("--output")

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
External input use: The code passes VERSION (sourced externally) to a subprocess without explicit sanitization
or constraints, which may require validation to prevent unintended command behaviors.

Referred Code
args.append("--selenium-version")
args.append(VERSION)
args.append("--output")

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid a potential circular import

To avoid a potential circular dependency, move the import of VERSION from the
top level into the binary_paths method where it is used.

py/selenium/webdriver/common/selenium_manager.py [27-59]

 from selenium.common import WebDriverException
-from selenium.webdriver import __version__ as VERSION
 
 
 logger = logging.getLogger(__name__)
 ...
     def binary_paths(self, args: list) -> dict:
 ...
         args.append("python")
+        from selenium.webdriver import __version__ as VERSION
         args.append("--selenium-version")
         args.append(VERSION)
         args.append("--output")
         args.append("json")
 
         return self._run(args)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential circular import, which could cause a runtime ImportError, and proposes a standard and effective solution by moving the import to a local scope.

Medium
  • More

@bonigarcia
Copy link
Member

Thanks for the PR, @cgoldberg!

The Python code looks good to me, but as explained in the additional considerations notes, the versioning of the development releases can be confusing. In my opinion, it would be better if unstable versions ended in "-dev" (or "-snapshot" or similar) rather than a number. But I don't know if that is very difficult to change. Also, I am unsure of the motivation for using that version number for unstable releases.

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.

2 participants