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][js] Add callback handlers for logging APIs #14120

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jun 11, 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

Laying down the premise to ensure the logging APIs return the callback id. This was be the basis for adding high-level APIs.

Motivation and Context

Implementing the high-level logging APIs requires adding and removing handlers. This is the necessary changes required before it can be implemented.

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 callback handlers for various log types in logInspector.js.
  • Introduced methods to add, remove, and invoke callbacks.
  • Enhanced log listening methods to support multiple callbacks and filtering.
  • Added comprehensive tests in log_inspector_test.js to verify multiple callback handling and filtering.

Changes walkthrough 📝

Relevant files
Enhancement
logInspector.js
Add callback handlers and enhance log listening methods   

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

  • Added callback handlers for various log types.
  • Introduced methods to add, remove, and invoke callbacks.
  • Enhanced log listening methods to support multiple callbacks and
    filtering.
  • +110/-30
    Tests
    log_inspector_test.js
    Add tests for multiple callback handling in log inspector

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

  • Added tests for multiple callback handling.
  • Introduced delay function for asynchronous test handling.
  • Verified callback invocation with and without filters.
  • +132/-0 

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    4

    🧪 Relevant tests

    Yes

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The invokeCallbacksWithFilter method uses a filter.getLevel() method which is not defined within the provided PR. This could lead to runtime errors if not properly implemented.

    Code Duplication:
    There is noticeable duplication in the methods onConsoleEntry, onJavascriptLog, and onLog especially in the way callbacks and filters are handled. Consider refactoring to reduce duplication and improve maintainability.

    Error Handling:
    There is a lack of error handling in asynchronous operations within WebSocket message events. It's recommended to add error handling to prevent unhandled promise rejections.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to ensure filter is defined before calling filter.getLevel() to avoid potential runtime errors

    In the invokeCallbacksWithFilter method, add a check to ensure filter is defined before
    calling filter.getLevel() to avoid potential runtime errors.

    javascript/node/selenium-webdriver/bidi/logInspector.js [91-93]

    -if (filterLevel === filter.getLevel()) {
    +if (filter && filterLevel === filter.getLevel()) {
       callback(data)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug where calling getLevel() on an undefined filter would cause a runtime error. Adding a null check is a crucial safeguard and improves the robustness of the code.

    9
    Performance
    Break out of the loop after removing the callback to avoid unnecessary iterations

    In the removeCallback method, after deleting the callback, you should break out of the
    loop to avoid unnecessary iterations once the callback is found and removed.

    javascript/node/selenium-webdriver/bidi/logInspector.js [70-72]

     if (callbacks.has(id)) {
       callbacks.delete(id)
    +  break
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a performance improvement by breaking out of the loop once the callback is removed, which prevents unnecessary iterations and is a good practice in loop management.

    8
    Reduce the delay in test cases to speed up test execution

    Reduce the delay in the test cases from 3000ms to a lower value like 1000ms to speed up
    the test execution while still allowing enough time for asynchronous operations.

    javascript/node/selenium-webdriver/test/bidi/log_inspector_test.js [141]

    -await delay(3000)
    +await delay(1000)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Reducing the delay in asynchronous operations in test cases is a valid suggestion to improve test execution speed. However, the exact optimal delay might need verification to ensure it still allows operations to complete as expected.

    6
    Best practice
    Validate the eventType in the addCallback method to ensure it is a valid type before adding the callback

    In the addCallback method, validate the eventType to ensure it is a valid type before
    adding the callback to avoid potential issues with invalid event types.

    javascript/node/selenium-webdriver/bidi/logInspector.js [63-64]

     const eventCallbackMap = this.listener.get(eventType)
    +if (!eventCallbackMap) {
    +  throw new Error(`Invalid event type: ${eventType}`)
    +}
     eventCallbackMap.set(id, callback)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Validating the eventType before adding a callback is a good practice to prevent errors related to invalid event types. This suggestion enhances the code's reliability by ensuring that only valid event types are processed.

    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