Skip to content

[py][bidi]: add bidi webExtension module #15749

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 15 commits into from
May 22, 2025

Conversation

navin772
Copy link
Member

@navin772 navin772 commented May 19, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds the BiDi webExtension module to the python bindings - https://w3c.github.io/webdriver-bidi/#module-webExtension

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement, Tests


Description

  • Add BiDi webExtension module to Python bindings

  • Implement install and uninstall methods for web extensions

  • Expose webextension property in Remote WebDriver

  • Add comprehensive tests for webExtension functionality


Changes walkthrough 📝

Relevant files
Enhancement
webextension.py
Add BiDi WebExtension module with install/uninstall methods

py/selenium/webdriver/common/bidi/webextension.py

  • Introduces new WebExtension class for BiDi protocol
  • Implements install and uninstall methods for web extensions
  • Provides parameter validation and command execution logic
  • +74/-0   
    webdriver.py
    Integrate webExtension property into Remote WebDriver       

    py/selenium/webdriver/remote/webdriver.py

  • Imports and initializes the new WebExtension class
  • Adds webextension property to expose BiDi webExtension commands
  • Updates internal attributes to support webExtension
  • Documents usage with examples in the property docstring
  • +24/-0   
    Tests
    bidi_webextension_tests.py
    Add tests for BiDi webExtension install/uninstall               

    py/test/selenium/webdriver/common/bidi_webextension_tests.py

  • Adds tests for BiDi webExtension module functionality
  • Tests install/uninstall via path, archive, and base64
  • Includes fixture for locating extension files
  • Marks tests as expected to fail on Chrome and Edge
  • +98/-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 May 19, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating Chrome Driver multiple times

    Requires further human verification:

    • Need to verify if the PR is related to this issue at all, as it appears to be adding new functionality rather than fixing the connection issue

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where JavaScript in link's href is not triggered on click() in Firefox

    Requires further human verification:

    • Need to verify if the PR is related to this issue at all, as it appears to be adding new functionality rather than fixing the Firefox click issue

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

    Error Handling

    The uninstall method doesn't validate if the extension_id is None or empty, which could lead to unexpected behavior when trying to uninstall an extension with an invalid ID.

    if isinstance(extension_id_or_result, dict):
        extension_id = extension_id_or_result.get("extension")
    else:
        extension_id = extension_id_or_result
    
    params = {"extension": extension_id}
    Documentation Error

    The docstring example for webextension property has a syntax error with an extra closing parenthesis in the install method call example.

    caps = _create_caps(capabilities)

    Copy link
    Contributor

    qodo-merge-pro bot commented May 19, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate extension ID

    Add validation to check if extension_id is None or empty after extraction from
    the dictionary. This prevents sending invalid parameters to the BiDi command
    when the dictionary doesn't contain the expected "extension" key.

    py/selenium/webdriver/common/bidi/webextension.py [60-74]

     def uninstall(self, extension_id_or_result: Union[str, Dict]) -> None:
         """Uninstalls a web extension from the remote end.
     
         Parameters:
         -----------
             extension_id_or_result: Either the extension ID as a string or the result dictionary
                                    from a previous install() call containing the extension ID.
         """
         if isinstance(extension_id_or_result, dict):
             extension_id = extension_id_or_result.get("extension")
         else:
             extension_id = extension_id_or_result
     
    +    if not extension_id:
    +        raise ValueError("Extension ID cannot be empty or None")
    +
         params = {"extension": extension_id}
         self.conn.execute(command_builder("webExtension.uninstall", params))
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: Adding validation for the extension_id before proceeding with the uninstall operation is a good defensive programming practice. It prevents sending invalid or empty IDs to the BiDi command, which could otherwise result in confusing errors or unintended behavior. While not critical, it improves robustness and error clarity.

    Medium
    General
    Fix documentation example
    Suggestion Impact:The commit fixed the exact syntax error in the documentation example that was pointed out in the suggestion, removing the extra closing parenthesis from the webextension.install example

    code diff:

    -        >>> extension_result = driver.webextension.install(path=extension_path)))
    +        >>> extension_result = driver.webextension.install(path=extension_path)

    Fix the syntax error in the example code where there's an extra closing
    parenthesis. This will prevent confusion for users following the documentation.

    py/selenium/webdriver/remote/webdriver.py [1348-1361]

     @property
     def webextension(self):
         """Returns a webextension module object for BiDi webextension commands.
     
         Returns:
         --------
         WebExtension: an object containing access to BiDi webextension commands.
     
         Examples:
         ---------
         >>> extension_path = "/path/to/extension"
    -    >>> extension_result = driver.webextension.install(path=extension_path)))
    +    >>> extension_result = driver.webextension.install(path=extension_path)
         >>> driver.webextension.uninstall(extension_result)
         """

    [Suggestion processed]

    Suggestion importance[1-10]: 6

    __

    Why: Correcting the extra parenthesis in the documentation example improves clarity and prevents user confusion, but it does not affect the functionality or correctness of the code itself. This is a minor but helpful documentation fix.

    Low
    • Update

    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.

    Nice. This looks really good.

    @navin772
    Copy link
    Member Author

    @cgoldberg yes, we should verify after loading a page. I will update the tests. Also, found that using absolute path for the extensions is much better from the ff_addon test file.

    Regarding xfail markers for safari and other non-bidi browsers, I think we already skip them here:

    selenium/py/conftest.py

    Lines 137 to 139 in a413faa

    if request.config.option.bidi:
    if driver_class in ("Ie", "Safari", "WebKitGTK", "WPEWebKit"):
    pytest.skip(f"{driver_class} does not support BiDi")

    Adding separate markers will make the tests unnecessarily lengthy. WDYT?

    @navin772
    Copy link
    Member Author

    The tests are failing due to extension path not found on engflow runner, locally they work fine. If someone can directly run on engflow and troubleshoot the path, it would be great. I cannot access it directly.

    @cgoldberg
    Copy link
    Contributor

    Regarding xfail markers for safari and other non-bidi browsers, I think we already skip them

    you're right.. I forgot about that.

    The tests are failing due to extension path not found on engflow runner

    It is failing on the GitHub runner also... I'm not sure why.. nothing looks incorrect in the failing test.

    other comments:

    • You might want to create a function that installs/uninstalls the extension for the tests, to cut down on code repetition. The tests mostly do the same thing.
    • The test for uninstalling looks redundant since all of the tests do an uninstall. You can probably remove that one, right?

    @navin772
    Copy link
    Member Author

    @cgoldberg thanks for the review. I will see if I can reduce the code duplication in the tests.

    The test for uninstalling looks redundant since all of the tests do an uninstall. You can probably remove that one, right?

    There's a slight difference in the way it uninstalls in that test, it uses the extension id directly instead of the dict response we get from installing the extension.
    Basically it covers the else statement in the uninstall method here:

            if isinstance(extension_id_or_result, dict):
                extension_id = extension_id_or_result.get("extension")
            else:
                extension_id = extension_id_or_result

    And for the test failures in the CI, I am still not sure why that's happening, any tips that will help in debugging?

    @cgoldberg
    Copy link
    Contributor

    any tips that will help in debugging?

    Not yet. I'll merge trunk so it runs again... maybe it will go away :) I'll take another look later today to help figure it out.

    @navin772
    Copy link
    Member Author

    It is failing on the GitHub runner also...

    @cgoldberg I added the required directories in the bazel BUILD file and now the CI that runs on github runner (CI / Python / Browser Tests (firefox, ubuntu)) passes but CI-RBE still fails :(

    @cgoldberg
    Copy link
    Contributor

    @oliverdunk
    Regarding those flags (--remote-debugging-pipe, --enable-unsafe-extension-debugging), we would either need to make this apparent to users through documentation (or possibly in an error message), or just add those to the default startup options. Would there be any danger or drawback of always running with those flags enabled? And will they remain available in future versions of Chrome? (I know flags often change).

    @navin772 navin772 requested a review from cgoldberg May 21, 2025 14:32
    @navin772
    Copy link
    Member Author

    @cgoldberg all the tests are now passing. @shs96c helped in getting the files/dirs referenced correctly in bazel.

    Also, updated the tests to reduce duplication.

    @oliverdunk
    Copy link

    Would there be any danger or drawback of always running with those flags enabled? And will they remain available in future versions of Chrome? (I know flags often change).

    Great question! I spoke to the team about this:

    • Enabling --remote-debugging-pipe is always safe, and in fact safer than the default where ChromeDriver uses a WebSocket to connect to Chrome (since that allows other processes to open parallel connections). However, it would likely be a breaking change, and also cause issues with the current implementation of the WebDriver.createCDPConnection method in Selenium.
    • The --enable-unsafe-extension-debugging flag allows for installing persistent extensions which can be dangerous. Therefore, it should only be enabled if a user wants to test extensions.

    I'm keen to see if we can loosen any of these requirements where the risk is lower enough, but I don't expect we will do that in the immediate future.

    Side-note: It would be great to see #15660 land but I haven't been able to reach anyone on the Selenium team about it. Is there any chance you would be able to help with finding a reviewer?

    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.

    @navin772 LGTM .. thanks
    My only question is how do the tests in ff_installs_addons_tests.py possibly work without the same runfiles change? That's really weird. Should we make the same change in that file for consistency?

    @navin772
    Copy link
    Member Author

    @cgoldberg ff_installs_addons_tests.py test doesn't run on CI-RBE but only on the GitHub runner, the CI-RBE is a much more isolated environment and only files/dirs exposed are available. That's why they are passing,

    I added the required directories in the bazel BUILD file and now the CI that runs on github runner (CI / Python / Browser Tests (firefox, ubuntu)) passes but CI-RBE still fails :(

    It happened with this test also in this PR.

    @cgoldberg
    Copy link
    Contributor

    Is there any chance you would be able to help with finding a reviewer?

    @oliverdunk I'll see if I can nudge someone to review it

    @cgoldberg
    Copy link
    Contributor

    @navin772 is this ready to merge? 4.33 release is going out soon and it would be nice to include this.

    @navin772
    Copy link
    Member Author

    @cgoldberg yes, it's ready!

    @cgoldberg
    Copy link
    Contributor

    @navin772 oops.. I just thought of one more thing. Add the module name to py/docs/source/api.rst so the API docs get generated for it!

    Then go ahead and merge it.

    @cgoldberg cgoldberg merged commit 43e6bb9 into SeleniumHQ:trunk May 22, 2025
    19 checks passed
    @navin772 navin772 deleted the py-bidi-webextension branch May 24, 2025 17:50
    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