Skip to content

Conversation

@Delta456
Copy link
Member

@Delta456 Delta456 commented Apr 25, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Implements log module as a separate package

🔧 Implementation Notes

log module was already implemented inside script module but log has to be implemented as their own separate package

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement, Tests


Description

  • Extracted log handling into a separate log module

  • Refactored script module to use new log module

  • Added tests for JavaScript error log handling

  • Improved test coverage for log handler removal


Changes walkthrough 📝

Relevant files
Enhancement
log.py
Add standalone BiDi log handling module                                   

py/selenium/webdriver/common/bidi/log.py

  • Introduced new log module for BiDi log entry handling
  • Defined data classes for console and JavaScript log entries
  • Provided factory for parsing log events from JSON
  • +70/-0   
    script.py
    Refactor script module to use new log module                         

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

  • Removed log entry classes and logic from script module
  • Imported and used new log module for log entry handling
  • Cleaned up code by delegating log parsing to new module
  • +1/-53   
    Tests
    bidi_script_tests.py
    Add and extend tests for BiDi log handling                             

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

  • Added tests for JavaScript error log handling
  • Added tests for removing JavaScript error handlers
  • Improved coverage for log handler registration/removal
  • +34/-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.
  • @Delta456 Delta456 requested a review from p0deje April 25, 2025 07:41
    @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Apr 25, 2025
    @qodo-merge-pro
    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)" issue when instantiating Chrome Driver multiple times

    Requires further human verification:

    • The PR is implementing BiDi log module functionality, which appears unrelated to the ChromeDriver connection issue

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()

    Requires further human verification:

    • The PR is implementing BiDi log module functionality, which appears unrelated to the Firefox JavaScript click issue

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

    Error Handling

    The LogEntryAdded.from_json method doesn't handle unknown log types, which could lead to silent failures if new log types are introduced in the BiDi protocol.

    @classmethod
    def from_json(cls, json):
        if json["type"] == "console":
            return ConsoleLogEntry.from_json(json)
        elif json["type"] == "javascript":
            return JavaScriptLogEntry.from_json(json)

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 25, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle unknown log types

    The method doesn't handle unknown log entry types, which could lead to silent
    failures. Add a default case to handle unexpected log types.

    py/selenium/webdriver/common/bidi/log.py [25-30]

     @classmethod
     def from_json(cls, json):
         if json["type"] == "console":
             return ConsoleLogEntry.from_json(json)
         elif json["type"] == "javascript":
             return JavaScriptLogEntry.from_json(json)
    +    else:
    +        raise ValueError(f"Unknown log entry type: {json['type']}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding error handling for unknown log types is important for robustness. Without this, the code would silently return None for unrecognized log types, potentially causing hard-to-debug issues downstream when code expects a valid log entry object.

    Medium
    General
    Fix attribute naming consistency

    There's a mismatch between the parameter name in the class (stacktrace) and the
    JSON key (stackTrace). This could lead to confusion when accessing the
    attribute.

    py/selenium/webdriver/common/bidi/log.py [54-70]

     @dataclass
     class JavaScriptLogEntry:
         level: str
         text: str
         timestamp: str
    -    stacktrace: dict
    +    stack_trace: dict
         type_: str
     
         @classmethod
         def from_json(cls, json):
             return cls(
                 level=json["level"],
                 text=json["text"],
                 timestamp=json["timestamp"],
    -            stacktrace=json["stackTrace"],
    +            stack_trace=json["stackTrace"],
                 type_=json["type"],
             )
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a naming inconsistency between the class attribute name (stacktrace) and the JSON key (stackTrace). Renaming to stack_trace improves code readability and maintains Python naming conventions while reducing potential confusion for developers.

    Medium
    Learned
    best practice
    Add input validation

    Add a check for the case when the "type" key is missing or has an unexpected
    value to prevent KeyError or returning None implicitly

    py/selenium/webdriver/common/bidi/log.py [25-30]

     @classmethod
     def from_json(cls, json):
    +    if not json or "type" not in json:
    +        raise ValueError("Invalid log entry: missing 'type' field")
    +    
         if json["type"] == "console":
             return ConsoleLogEntry.from_json(json)
         elif json["type"] == "javascript":
             return JavaScriptLogEntry.from_json(json)
    +    else:
    +        raise ValueError(f"Unknown log entry type: {json['type']}")
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    • Update

    @p0deje p0deje merged commit 88479dc into SeleniumHQ:trunk Apr 25, 2025
    17 checks passed
    @Delta456 Delta456 deleted the py_bidi_log branch April 25, 2025 11:37
    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.

    3 participants