Skip to content

[py][bidi]: add bidi command session.status and encapsulate in Session class #15615

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

Merged
merged 5 commits into from
Apr 23, 2025

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Apr 11, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Implements the session.status bidi command in python bindings - https://w3c.github.io/webdriver-bidi/#command-session-status

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement, Tests


Description

  • Added session.status BiDi command to Python bindings.

  • Implemented get_bidi_session_status method in WebDriver.

  • Created tests for session.status BiDi command functionality.

  • Enhanced BiDi support with session readiness checks.


Changes walkthrough 📝

Relevant files
Enhancement
session.py
Add `session.status` BiDi command function                             

py/selenium/webdriver/common/bidi/session.py

  • Added session_status function to handle session.status BiDi command.
  • Returns readiness and metadata information about the remote end.
  • +18/-0   
    webdriver.py
    Add `get_bidi_session_status` method to WebDriver               

    py/selenium/webdriver/remote/webdriver.py

  • Added get_bidi_session_status method to WebDriver.
  • Integrated BiDi session readiness check with example usage.
  • +24/-0   
    Tests
    bidi_session_tests.py
    Add tests for `session.status` BiDi command                           

    py/test/selenium/webdriver/common/bidi_session_tests.py

  • Added tests for session.status BiDi command.
  • Verified readiness and metadata in session status response.
  • Tested session status with multiple windows and tabs.
  • +51/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Apr 11, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()
    • Ensure JavaScript in href attributes is properly executed when clicking links in Firefox 42.0

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "ConnectFailure (Connection refused)" error when instantiating ChromeDriver
    • Address issue where subsequent ChromeDriver instantiations fail after the first one succeeds

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The get_bidi_session_status method doesn't include error handling for cases where the BiDi connection fails or returns unexpected data structure.

    def get_bidi_session_status(self):
        """
        Get the session status using WebDriver BiDi.
        Returns information about whether a remote end is in a state
        in which it can create new sessions.
    
        Returns:
        -------
        dict
            Dictionary containing the ready state (bool), message (str) and metadata
    
        Example:
        --------
        >>> status = driver.get_bidi_session_status()
        >>> print(status["ready"])
        >>> print(status["message"])
        """
        if not self._websocket_connection:
            self._start_bidi()
    
        from selenium.webdriver.common.bidi.session import session_status
    
        return self._websocket_connection.execute(session_status())

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 11, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Move import to module level
    Suggestion Impact:The commit implemented the suggestion by moving the session-related import to module level (line 8). Additionally, the commit completely refactored the session functionality, replacing the get_bidi_session_status method with a session property that uses the imported Session class.

    code diff:

    +from selenium.webdriver.common.bidi.browser import Browser
     from selenium.webdriver.common.bidi.network import Network
     from selenium.webdriver.common.bidi.script import Script
    +from selenium.webdriver.common.bidi.session import Session
     from selenium.webdriver.common.by import By
     from selenium.webdriver.common.options import ArgOptions
     from selenium.webdriver.common.options import BaseOptions
    @@ -254,6 +256,8 @@
             self._websocket_connection = None
             self._script = None
             self._network = None
    +        self._browser = None
    +        self._session = None
     
         def __repr__(self):
             return f'<{type(self).__module__}.{type(self).__name__} (session="{self.session_id}")>'
    @@ -1269,29 +1273,46 @@
     
             return self._network
     
    -    def get_bidi_session_status(self):
    -        """
    -        Get the session status using WebDriver BiDi.
    -        Returns information about whether a remote end is in a state
    -        in which it can create new sessions.
    +    @property
    +    def browser(self):
    +        """Returns a browser module object for BiDi browser commands.
     
             Returns:
    -        -------
    -        dict
    -            Dictionary containing the ready state (bool), message (str) and metadata
    -
    -        Example:
    -        --------
    -        >>> status = driver.get_bidi_session_status()
    -        >>> print(status["ready"])
    -        >>> print(status["message"])
    +        --------
    +        Browser: an object containing access to BiDi browser commands.
    +
    +        Examples:
    +        ---------
    +        >>> user_context = driver.browser.create_user_context()
    +        >>> user_contexts = driver.browser.get_user_contexts()
    +        >>> client_windows = driver.browser.get_client_windows()
    +        >>> driver.browser.remove_user_context(user_context)
             """
             if not self._websocket_connection:
                 self._start_bidi()
     
    -        from selenium.webdriver.common.bidi.session import session_status
    -
    -        return self._websocket_connection.execute(session_status())
    +        if self._browser is None:
    +            self._browser = Browser(self._websocket_connection)
    +
    +        return self._browser
    +
    +    @property
    +    def session(self):
    +        """Returns the BiDi session object for the current WebDriver session.
    +
    +        Example:
    +        --------
    +        >>> driver.session.subscribe()
    +        >>> driver.session.unsubscribe()
    +        >>> session = driver.session.status()
    +        """
    +        if not self._websocket_connection:
    +            self._start_bidi()
    +
    +        if self._session is None:
    +            self._session = Session(self._websocket_connection)
    +
    +        return self._session

    Move the import statement to the top of the file with other imports rather than
    inside the method. Importing within a method can cause performance issues if the
    method is called frequently, as the import operation will be repeated each time.

    py/selenium/webdriver/remote/webdriver.py [1289-1294]

     def get_bidi_session_status(self):
         """
         Get the session status using WebDriver BiDi.
         Returns information about whether a remote end is in a state
         in which it can create new sessions.
     
         Returns:
         -------
         dict
             Dictionary containing the ready state (bool), message (str) and metadata
     
         Example:
         --------
         >>> status = driver.get_bidi_session_status()
         >>> print(status["ready"])
         >>> print(status["message"])
         """
         if not self._websocket_connection:
             self._start_bidi()
     
    -    from selenium.webdriver.common.bidi.session import session_status
    -
         return self._websocket_connection.execute(session_status())

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 3

    __

    Why: Moving the import statement to the module level is a valid suggestion that follows Python best practices and could slightly improve performance if the method is called frequently. However, the impact is relatively minor as the performance difference would only be noticeable with frequent calls.

    Low
    • Update

    @navin772
    Copy link
    Member Author

    I am not sure if this is the best way to do it, or whether we should have a new class for session module (like we have for browser and network) but that might introduce breaking changes.
    Any feedback is welcomed.

    @shbenzer
    Copy link
    Contributor

    shbenzer commented Apr 12, 2025

    My preference would be a class-based approach

    @navin772
    Copy link
    Member Author

    @shbenzer I have updated the PR, it now uses class-based approach for Session module.

    I tried adding the session.new command but while testing the session was not created which is expected because in the test environment we already spawn a session and creating an additional session will result in an error.
    So, should I be adding it? I am not sure how to test it.
    cc: @p0deje

    @shbenzer
    Copy link
    Contributor

    If I’m not mistaken, each test is self-contained. So you should be able to test closing the session then opening a new one

    @navin772
    Copy link
    Member Author

    @shbenzer the session.end command is not yet implemented and using driver.close() doesn't works. Currently, there is no way to create a session without a browser driver, hence this BiDi new session won't work.
    No other bindings have implemented it yet so I think we can skip it for now.

    @navin772 navin772 requested a review from p0deje April 16, 2025 06:38
    @navin772 navin772 changed the title [py][bidi]: add bidi command session.status [py][bidi]: add bidi command session.status and encapsulate in Session class Apr 19, 2025
    @p0deje
    Copy link
    Member

    p0deje commented Apr 23, 2025

    The code looks good, but I'm not sure we want to expose session commands in this public API on the WebDriver instance. In fact, we might want to even make it private API, only used internally to start BiDi sessions when needed.

    @navin772
    Copy link
    Member Author

    @p0deje I have made the webdriver instance for session private.

    @p0deje p0deje merged commit 5a2d259 into SeleniumHQ:trunk Apr 23, 2025
    17 checks passed
    @navin772 navin772 deleted the py-bidi-session-status branch April 23, 2025 10:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants