Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[py] Allow driver path to be set using ENV variables #14528

Merged

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Sep 23, 2024

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

Allows using custom driver paths using ENV variables

Implements the functionality from the Ruby port in commit 7602371#diff-076503247734547bc24938fcc3aa2b317890093dae9376e32ac9b1e41b7037f9

Motivation and Context

As explained in #14045 the goal of this PR is to provide users with an alternative option to set the path to their own drivers if they do not want to use selenium manager

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.

PS: Thanks to @Animesh-Ghosh and @aguspe for the help 🚀


PR Type

Enhancement, Tests


Description

  • Added support for setting custom driver paths using environment variables across multiple drivers (Chrome, Firefox, Edge, IE, Safari).
  • Introduced a new parameter driver_path_env_key to specify the environment variable key for driver paths.
  • Implemented env_path method in the Service class to fetch driver paths from environment variables.
  • Updated service path logic to prioritize environment variable paths over default paths.
  • Added tests to verify the functionality of using environment variables for driver paths.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
service.py
Add environment variable support for ChromeDriver path     

py/selenium/webdriver/chromium/service.py

  • Added driver_path_env_key parameter to the constructor.
  • Set default environment variable key for ChromeDriver.
  • +3/-0     
    webdriver.py
    Use environment variable for Chromium driver path               

    py/selenium/webdriver/chromium/webdriver.py

    • Updated service path to use environment variable if available.
    +1/-1     
    service.py
    Implement environment variable path support in Service     

    py/selenium/webdriver/common/service.py

  • Added driver_path_env_key parameter to the constructor.
  • Implemented env_path method to fetch path from environment variable.
  • +6/-2     
    service.py
    Add environment variable support for EdgeDriver path         

    py/selenium/webdriver/edge/service.py

  • Added driver_path_env_key parameter to the constructor.
  • Set default environment variable key for EdgeDriver.
  • +3/-0     
    service.py
    Add environment variable support for GeckoDriver path       

    py/selenium/webdriver/firefox/service.py

  • Added driver_path_env_key parameter to the constructor.
  • Set default environment variable key for GeckoDriver.
  • +3/-0     
    webdriver.py
    Use environment variable for Firefox driver path                 

    py/selenium/webdriver/firefox/webdriver.py

    • Updated service path to use environment variable if available.
    +1/-1     
    service.py
    Add environment variable support for IEDriver path             

    py/selenium/webdriver/ie/service.py

  • Added driver_path_env_key parameter to the constructor.
  • Set default environment variable key for IEDriver.
  • +4/-0     
    webdriver.py
    Use environment variable for IE driver path                           

    py/selenium/webdriver/ie/webdriver.py

    • Updated service path to use environment variable if available.
    +1/-1     
    service.py
    Add environment variable support for SafariDriver path     

    py/selenium/webdriver/safari/service.py

  • Added driver_path_env_key parameter to the constructor.
  • Set default environment variable key for SafariDriver.
  • +3/-0     
    webdriver.py
    Use environment variable for Safari driver path                   

    py/selenium/webdriver/safari/webdriver.py

    • Updated service path to use environment variable if available.
    +1/-1     
    Tests
    2 files
    chrome_service_tests.py
    Add tests for ChromeDriver path from environment variable

    py/test/selenium/webdriver/chrome/chrome_service_tests.py

  • Added tests for ChromeDriver service path using environment variable.
  • +24/-0   
    firefox_service_tests.py
    Add tests for GeckoDriver path from environment variable 

    py/test/selenium/webdriver/firefox/firefox_service_tests.py

  • Added tests for GeckoDriver service path using environment variable.
  • +25/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Bug
    The env_path() method returns None if the environment variable is not set, which could lead to unexpected behavior if not handled properly in the calling code.

    Code Duplication
    The logic for setting the service path is duplicated across different webdriver implementations (Chrome, Firefox, IE, Safari). Consider refactoring this into a common method.

    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 23, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a docstring to the env_path method for better documentation

    Consider adding a docstring for the env_path method to explain its purpose and
    return value.

    py/selenium/webdriver/common/service.py [241-242]

     def env_path(self) -> Optional[str]:
    +    """
    +    Retrieve the driver path from the environment variable.
    +    
    +    Returns:
    +        Optional[str]: The driver path if set in the environment, None otherwise.
    +    """
         return os.getenv(self.DRIVER_PATH_ENV_KEY, None)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Providing a docstring significantly improves code documentation, helping developers understand the method's purpose and return value, which is crucial for maintainability.

    9
    Enhancement
    Add a type hint for the driver_path_env_key parameter in the constructor

    Consider adding a type hint for the driver_path_env_key parameter in the init
    method to improve code readability and maintainability.

    py/selenium/webdriver/common/service.py [50-58]

     def __init__(
         self,
         executable_path: str = None,
         port: int = 0,
         log_output: SubprocessStdAlias = None,
         env: typing.Optional[typing.Mapping[typing.Any, typing.Any]] = None,
    -    driver_path_env_key: str = None,
    +    driver_path_env_key: typing.Optional[str] = None,
         **kwargs,
     ) -> None:
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a type hint enhances code readability and maintainability, making it clear that the parameter is optional and expected to be a string.

    8
    Rename the environment variable key attribute for better clarity and consistency

    Consider using a more descriptive name for the environment variable key attribute.
    Instead of DRIVER_PATH_ENV_KEY, a name like driver_path_env_var might be clearer and
    more consistent with Python naming conventions.

    py/selenium/webdriver/common/service.py [73]

    -self.DRIVER_PATH_ENV_KEY = driver_path_env_key
    +self.driver_path_env_var = driver_path_env_key
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability by using a more descriptive and consistent naming convention, which is beneficial for maintainability.

    7
    Maintainability
    Add a comment to explain the fallback mechanism for setting the service path

    Consider adding a comment to explain the fallback mechanism when setting the
    self.service.path. This will help other developers understand the logic behind using
    both env_path() and finder.get_driver_path().

    py/selenium/webdriver/chromium/webdriver.py [54]

    +# Use the path from environment variable if set, otherwise use the path found by DriverFinder
     self.service.path = self.service.env_path() or finder.get_driver_path()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The comment clarifies the logic behind the fallback mechanism, aiding in code comprehension and future maintenance, though it is not critical.

    6

    💡 Need additional feedback ? start a PR chat

    @Delta456 Delta456 force-pushed the py_use_env_driver_location branch from 8ed0f37 to 2a63e21 Compare September 23, 2024 09:19
    @AutomatedTester AutomatedTester merged commit a2cacc1 into SeleniumHQ:trunk Oct 4, 2024
    12 of 13 checks passed
    @Delta456 Delta456 deleted the py_use_env_driver_location branch November 6, 2024 10:30
    lauromoura added a commit to lauromoura/selenium that referenced this pull request Dec 6, 2024
    Follow up to PR SeleniumHQ#14528, to avoid `os.getenv` raising `TypeError`
    in `env_path` when `driver_path_env_key` is not passed to `Service`
    constructor.
    lauromoura added a commit to lauromoura/selenium that referenced this pull request Dec 12, 2024
    Follow up to PR SeleniumHQ#14528, to avoid `os.getenv` raising `TypeError`
    in `env_path` when `driver_path_env_key` is not passed to `Service`
    constructor.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants