Skip to content

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Jun 23, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds 3 high level SCRIPT APIs as mentioned in #13992

  1. pin
  2. unpin
  3. execute

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Add high-level BiDi script API methods: pin, unpin, execute

  • Implement argument conversion for BiDi LocalValue format

  • Add comprehensive test coverage for script execution scenarios

  • Enable script pinning/unpinning and execution with error handling


Changes walkthrough 📝

Relevant files
Enhancement
script.py
Implement high-level BiDi script API methods                         

py/selenium/webdriver/common/bidi/script.py

  • Add three high-level API methods: pin(), unpin(), execute()
  • Implement argument conversion to BiDi LocalValue format
  • Add error handling for script execution failures
  • Include driver reference for browsing context access
  • +96/-1   
    webdriver.py
    Update Script initialization with driver reference             

    py/selenium/webdriver/remote/webdriver.py

    • Pass driver reference to Script constructor for context access
    +1/-1     
    Tests
    bidi_script_tests.py
    Add comprehensive tests for high-level script API               

    py/test/selenium/webdriver/common/bidi_script_tests.py

  • Fix log level comparison using LogLevel.ERROR enum
  • Add comprehensive tests for pin, unpin, execute methods
  • Test various argument types and error scenarios
  • Include DOM access and nested object tests
  • +217/-1 

    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 Jun 23, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    13992 - PR Code Verified

    Compliant requirements:

    • Add pin() method for script pinning
    • Add unpin() method for script unpinning
    • Add execute() method for script execution
    • Methods should be accessible directly from Driver class via script() method
    • Implementation should be for Python binding

    Requires further human verification:

    • Keep methods labeled as beta during development

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

    Error Handling

    The error handling in execute() method checks for 'text' and 'message' fields in exception_details but may not cover all possible error response formats from BiDi specification

    error_message = "Error while executing script"
    if result.exception_details:
        if "text" in result.exception_details:
            error_message += f": {result.exception_details['text']}"
        elif "message" in result.exception_details:
            error_message += f": {result.exception_details['message']}"
    
    raise WebDriverException(error_message)
    Type Conversion

    The __convert_to_local_value method handles basic Python types but may not properly handle edge cases like NaN, Infinity, or complex nested structures with circular references

    def __convert_to_local_value(self, value) -> dict:
        """
        Converts a Python value to BiDi LocalValue format.
        """
        if value is None:
            return {"type": "undefined"}
        elif isinstance(value, bool):
            return {"type": "boolean", "value": value}
        elif isinstance(value, (int, float)):
            return {"type": "number", "value": value}
        elif isinstance(value, str):
            return {"type": "string", "value": value}
        elif isinstance(value, (list, tuple)):
            return {"type": "array", "value": [self.__convert_to_local_value(item) for item in value]}
        elif isinstance(value, dict):
            return {
                "type": "object",
                "value": [
                    [self.__convert_to_local_value(k), self.__convert_to_local_value(v)] for k, v in value.items()
                ],
            }
        else:
            # For other types, convert to string
            return {"type": "string", "value": str(value)}

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 23, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @navin772 navin772 requested review from p0deje and shbenzer June 27, 2025 11:56
    Copy link
    Member

    @p0deje p0deje left a comment

    Choose a reason for hiding this comment

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

    Looks great! I think we might want to tweak execute so it can accept non-function scripts because at some point we'd like to replace driver.execute_script with driver.script.execute and the former one accepts plain scripts. Something like this:

    driver.script.execute("return window.location;")

    This can be done later however, thank you for working on this!

    @navin772
    Copy link
    Member Author

    Thank you @p0deje for the review, I will try to implement your suggestion in the future.

    @navin772 navin772 merged commit 134c0b7 into SeleniumHQ:trunk Jul 1, 2025
    16 checks passed
    @navin772 navin772 deleted the py-bidi-script-high-level-API branch July 1, 2025 05:49
    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 4/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants