Skip to content

[py][bidi]: add high level API for script module - pin, unpin and execute #15936

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

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

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

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    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.

    2 participants