Skip to content
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

Update Ruby BiDi script structs to match spec (as of 2024-07-08) #14236

Merged

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Jul 8, 2024

User description

Description

This updates the structs used by the BiDi Script and LogHandler classes to represent JS logs and errors so that they match the W3C spec as of 2024-07-08. This is meant to resolve the discussion about the context of JS Logs and errors from #13992.

There are a couple open questions here:

  1. These structs include some complex collections that have defined types in the spec, but are represented here as hashes, rather than other structs. Should they be structs instead? (For context, this PR was motivated by a desire to get at the information in log_entry_instance.source['context'], which would be nicer to get as log_entry_instance.source.context.)

    Caveat: args and stack_trace already existed as arrays of hashes and hashes of arrays of hashes respectively, so maybe making nice structs would be too much of a breaking change?

  2. The spec now has a GenericLogEntry type for things that aren’t JS logs or exceptions (I think this is for various warnings and errors logged by the browser itself, like CORS blocking or content integrity problems). That’s not handled here; should I try to add it? Happy to do that work here or in a separate PR if desired.

    It’s not currently a problem since Script doesn’t ever add handlers for other types of messages that would show up this way, but someone could theoretically subscribe to them via an instance of LogHandler and that might not work out so well.

Motivation and Context

The BiDi spec has added more fields since LogHandler was first implemented, and the current functionality is missing important information, like the ID of the browser context a log entry came from (in entry.source.context). This just updates the structs with the fields currently expected by the spec.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed. (I couldn’t get all tests to pass locally even before I touched things, but all tests that I expect could have been affected were passing and still are.)

PR Type

Enhancement, Tests


Description

  • Updated the ConsoleLogEntry and JavaScriptLogEntry structs in LogHandler to include the source field, aligning with the W3C spec as of 2024-07-08.
  • Added method, args, and stack_trace fields to ConsoleLogEntry struct.
  • Introduced a helper method a_stack_frame in the test suite to match script.StackFrame objects.
  • Enhanced existing tests to validate the new fields in ConsoleLogEntry and JavaScriptLogEntry, including detailed checks for source, args, and stack_trace.

Changes walkthrough 📝

Relevant files
Enhancement
log_handler.rb
Update BiDi log entry structs to match W3C spec                   

rb/lib/selenium/webdriver/bidi/log_handler.rb

  • Updated ConsoleLogEntry and JavaScriptLogEntry structs to include
    source.
  • Added method, args, and stack_trace to ConsoleLogEntry.
  • +2/-2     
    Tests
    script_spec.rb
    Enhance BiDi script tests for updated log entry structs   

    rb/spec/integration/selenium/webdriver/bidi/script_spec.rb

  • Added helper method a_stack_frame for matching script.StackFrame
    objects.
  • Enhanced tests to check for new fields in ConsoleLogEntry and
    JavaScriptLogEntry.
  • Added detailed expectations for source, args, and stack_trace.
  • +46/-3   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    This updates the structs used by the BiDi `Script` and `LogHandler` classes to match the W3C spec as of 2024-07-08. It also makes the tests slightly more detailed.
    @CLAassistant
    Copy link

    CLAassistant commented Jul 8, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 8, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The PR introduces changes to the ConsoleLogEntry and JavaScriptLogEntry structs, adding new fields such as stack_trace, source, method, and args. Ensure these fields are populated correctly as per the updated W3C spec and that they integrate seamlessly with existing functionalities.

    Data Structure Consistency:
    The PR uses hashes to represent complex collections. Consider whether using more structured types might improve code maintainability and readability.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 8, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add type validation to ensure ConsoleLogEntry and JavaScriptLogEntry are initialized with correct types

    Consider adding a validation to ensure that ConsoleLogEntry and JavaScriptLogEntry are
    initialized with the correct types for each attribute. This can help catch errors early if
    incorrect data is passed.

    rb/lib/selenium/webdriver/bidi/log_handler.rb [24-25]

    -ConsoleLogEntry = BiDi::Struct.new(:level, :text, :timestamp, :stack_trace, :type, :source, :method, :args)
    -JavaScriptLogEntry = BiDi::Struct.new(:level, :text, :timestamp, :stack_trace, :type, :source)
    +ConsoleLogEntry = BiDi::Struct.new(:level, :text, :timestamp, :stack_trace, :type, :source, :method, :args) do
    +  def initialize(*args)
    +    super
    +    raise TypeError, "level must be a String" unless level.is_a?(String)
    +    raise TypeError, "timestamp must be an Integer" unless timestamp.is_a?(Integer)
    +    # Add more type checks as needed
    +  end
    +end
     
    +JavaScriptLogEntry = BiDi::Struct.new(:level, :text, :timestamp, :stack_trace, :type, :source) do
    +  def initialize(*args)
    +    super
    +    raise TypeError, "level must be a String" unless level.is_a?(String)
    +    raise TypeError, "timestamp must be an Integer" unless timestamp.is_a?(Integer)
    +    # Add more type checks as needed
    +  end
    +end
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding type validation is a good practice to ensure data integrity and catch errors early. The suggestion correctly targets the new code in the PR.

    7
    Maintainability
    Extract repeated source matching code into a helper method for better readability

    To improve readability and maintainability, consider extracting the repeated
    expect(log_entry.source).to match(...) code into a helper method.

    rb/spec/integration/selenium/webdriver/bidi/script_spec.rb [70-73]

    -expect(log_entry.source).to match(
    -  'context' => an_instance_of(String),
    -  'realm' => an_instance_of(String)
    -)
    +def expect_source_to_match(source)
    +  expect(source).to match(
    +    'context' => an_instance_of(String),
    +    'realm' => an_instance_of(String)
    +  )
    +end
     
    +# Usage
    +expect_source_to_match(log_entry.source)
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Extracting repeated code into a helper method improves maintainability and readability. The suggestion is relevant and targets new code in the PR.

    6
    Enhancement
    Add a test to check for the presence of optional fields in JavaScriptLogEntry

    Consider adding a test to check for the presence of optional fields in JavaScriptLogEntry
    to ensure they are handled correctly.

    rb/spec/integration/selenium/webdriver/bidi/script_spec.rb [139-145]

    -expect(log_entry.stack_trace).to match(
    -  # Some browsers include stack traces from parts of the runtime, so we
    -  # just check the first frames that come from user code.
    +expect(log_entry.stack_trace).to be_nil.or match(
       'callFrames' => start_with(
         a_stack_frame('functionName' => 'createError'),
         a_stack_frame('functionName' => 'onclick')
       )
     )
    +# Additional test for optional fields
    +expect(log_entry).to respond_to(:optional_field)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to add tests for optional fields is valid and enhances the robustness of the test suite. However, the existing code already handles optional fields in stack traces, making this a moderate improvement.

    5
    Best practice
    Use be_a_kind_of for type checking to handle inheritance and ensure consistency

    To ensure consistency, consider using be_a_kind_of instead of be_a for type checking, as
    it is more flexible and can handle inheritance.

    rb/spec/integration/selenium/webdriver/bidi/script_spec.rb [61]

    -expect(log_entry).to be_a BiDi::LogHandler::ConsoleLogEntry
    +expect(log_entry).to be_a_kind_of BiDi::LogHandler::ConsoleLogEntry
     
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While using be_a_kind_of can be more flexible than be_a, this change is not crucial and the existing be_a is sufficient for the current test cases.

    4

    @Mr0grog
    Copy link
    Contributor Author

    Mr0grog commented Jul 8, 2024

    NOTE: This was based on some discussion in Slack with @titusfortner.

    @p0deje p0deje merged commit c145a83 into SeleniumHQ:trunk Oct 21, 2024
    1 check passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants