Skip to content

[dotnet] [bidi] Added missing GenericLogEntry log entry type in Script module #15591

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 1 commit into from
Apr 7, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Apr 7, 2025

User description

https://w3c.github.io/webdriver-bidi/#types-log-logentry

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement


Description

  • Added support for GenericLogEntry in BiDi log module.

  • Updated LogEntryConverter to handle unrecognized log types.

  • Introduced GenericLogEntry record as a fallback log entry type.


Changes walkthrough 📝

Relevant files
Enhancement
LogEntryConverter.cs
Enhance LogEntryConverter with fallback deserialization   

dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/LogEntryConverter.cs

  • Updated LogEntryConverter to deserialize unrecognized log types.
  • Added fallback to GenericLogEntry for unknown log entry types.
  • +1/-1     
    LogEntry.cs
    Add GenericLogEntry record for fallback handling                 

    dotnet/src/webdriver/BiDi/Modules/Log/LogEntry.cs

  • Introduced GenericLogEntry record for fallback log entries.
  • Added new record to handle unrecognized log types.
  • +4/-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.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Apr 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()
    • Ensure JavaScript in href attributes is properly executed when clicking links in Firefox 42.0

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating ChromeDriver multiple times
    • Address connection issues with ChromeDriver 2.35 and Chrome 65.0.3325.181 on Ubuntu 16.04.4

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

    Error Handling

    The converter now returns a GenericLogEntry for unrecognized log types, but there's no validation or error logging when unexpected types are encountered. Consider adding diagnostic information for debugging.

        _ => JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<GenericLogEntry>()),
    };

    @nvborisenko nvborisenko changed the title [dotnet] [bidi] Added missing GenericLogEntry log entry type in Scrip… [dotnet] [bidi] Added missing GenericLogEntry log entry type in Script module Apr 7, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Preserve type information

    The Type parameter in GenericLogEntry is not being stored as a property, which
    means this important information will be lost after construction. Add a property
    to preserve the type information.

    dotnet/src/webdriver/BiDi/Modules/Log/LogEntry.cs [36-37]

     public record GenericLogEntry(BiDi BiDi, string Type, Level Level, Script.Source Source, string Text, DateTimeOffset Timestamp)
    -    : LogEntry(BiDi, Level, Source, Text, Timestamp);
    +    : LogEntry(BiDi, Level, Source, Text, Timestamp)
    +{
    +    public string Type { get; } = Type;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the Type parameter in GenericLogEntry is not being stored as a property, which would cause this important information to be lost after construction. Since GenericLogEntry is meant to handle unknown log types, preserving the type information is critical for proper functionality.

    Medium
    • More

    @nvborisenko nvborisenko requested a review from Copilot April 7, 2025 18:27
    Copy link

    @Copilot Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

    Comments suppressed due to low confidence (1)

    dotnet/src/webdriver/BiDi/Modules/Log/LogEntry.cs:36

    • [nitpick] The parameter name 'Type' in GenericLogEntry may be easily confused with C# type metadata. Consider renaming it to 'logType' or 'entryType' for improved clarity.
    public record GenericLogEntry(BiDi BiDi, string Type, Level Level, Script.Source Source, string Text, DateTimeOffset Timestamp)
    

    @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Apr 7, 2025
    @nvborisenko nvborisenko merged commit 3d0f690 into SeleniumHQ:trunk Apr 7, 2025
    14 of 15 checks passed
    @nvborisenko nvborisenko deleted the bidi-generic-logentry branch April 7, 2025 19:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants