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

[bidi] Add source type to log entry #14244

Merged
merged 3 commits into from
Jul 10, 2024
Merged

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jul 10, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Add source to log entry events in compliance with the spec so the user can use that browsing context information to chain more commands.

Motivation and Context

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.

PR Type

Enhancement, Tests


Description

  • Added Source field to various log entry classes (BaseLogEntry, ConsoleLogEntry, GenericLogEntry, JavascriptLogEntry) in both Java and JavaScript implementations.
  • Updated constructors and fromJson methods to handle the new Source field.
  • Removed realm field and related methods from log entry classes.
  • Updated tests to include assertions for the Source field and removed assertions for realm.

Changes walkthrough 📝

Relevant files
Enhancement
BaseLogEntry.java
Add `Source` field to `BaseLogEntry` class.                           

java/src/org/openqa/selenium/bidi/log/BaseLogEntry.java

  • Added Source field to BaseLogEntry.
  • Updated constructor to include Source.
  • Added getter method for Source.
  • +10/-1   
    ConsoleLogEntry.java
    Add `Source` field and remove `realm` in `ConsoleLogEntry`.

    java/src/org/openqa/selenium/bidi/log/ConsoleLogEntry.java

  • Added Source field to ConsoleLogEntry.
  • Updated constructor to include Source.
  • Modified fromJson method to read Source.
  • Removed realm field and related methods.
  • +10/-15 
    GenericLogEntry.java
    Add `Source` field to `GenericLogEntry` class.                     

    java/src/org/openqa/selenium/bidi/log/GenericLogEntry.java

  • Added Source field to GenericLogEntry.
  • Updated constructor to include Source.
  • Modified fromJson method to read Source.
  • +14/-3   
    JavascriptLogEntry.java
    Add `Source` field to `JavascriptLogEntry` class.               

    java/src/org/openqa/selenium/bidi/log/JavascriptLogEntry.java

  • Added Source field to JavascriptLogEntry.
  • Updated constructor to include Source.
  • Modified fromJson method to read Source.
  • +14/-3   
    logEntries.js
    Add `Source` field and remove `realm` in log entries.       

    javascript/node/selenium-webdriver/bidi/logEntries.js

  • Added Source field to BaseLogEntry.
  • Updated constructors to include Source.
  • Removed realm field and related methods in ConsoleLogEntry.
  • +15/-17 
    logInspector.js
    Include `Source` field in log entry creation.                       

    javascript/node/selenium-webdriver/bidi/logInspector.js

  • Updated log entry creation to include Source field.
  • Removed realm field handling.
  • +6/-2     
    Tests
    LogInspectorTest.java
    Update tests to include `Source` field assertions.             

    java/test/org/openqa/selenium/bidi/log/LogInspectorTest.java

  • Added assertions for Source field in log entry tests.
  • Removed assertions for realm.
  • +8/-6     
    log_inspector_test.js
    Update tests to include `Source` field assertions.             

    javascript/node/selenium-webdriver/test/bidi/log_inspector_test.js

  • Added assertions for Source field in log entry tests.
  • Removed assertions for realm.
  • +12/-5   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Possible Bug:
    The removal of the 'realm' field in the Java classes without a corresponding update in the JavaScript classes might lead to inconsistencies or errors when the 'realm' is expected but not provided.

    Code Consistency:
    The 'source' field is added to log entries, but the handling of this new field in JavaScript seems inconsistent. For example, the 'source' is used in constructors but not always in corresponding methods or JSON parsing functions.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add null checks for source in the constructor to ensure it is not null

    Add null checks for source parameter in the constructor to ensure robustness and
    prevent runtime exceptions when source is null.

    java/src/org/openqa/selenium/bidi/log/ConsoleLogEntry.java [37-46]

     public ConsoleLogEntry(
         LogLevel level, Source source, String text, long timestamp, String type, StackTrace stackTrace) {
    +  if (source == null) {
    +    throw new IllegalArgumentException("Source cannot be null");
    +  }
       super(level, source, text, timestamp, type, stackTrace);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding null checks for the source parameter in the constructor is a good practice to ensure robustness and prevent runtime exceptions, making the code more reliable.

    9
    Possible bug
    Initialize the source field to prevent null pointer exceptions

    Ensure that the source field is properly initialized in the constructor to avoid
    NullPointerException when the getSource() method is called without setting the
    source.

    java/src/org/openqa/selenium/bidi/log/BaseLogEntry.java [27-55]

    -private Source source;
    +private Source source = new Source(); // Adjust the constructor as per actual implementation
     ...
     this.source = source;
     
    Suggestion importance[1-10]: 7

    Why: Initializing the source field can prevent potential NullPointerException, improving robustness. However, the suggestion assumes a default constructor for Source, which may not be accurate without further context.

    7

    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.

    1 participant