search

SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.74k stars 8.02k forks source link

[bidi][js] Add callback handlers for logging APIs #14120

Closed pujagani closed 2 weeks ago

pujagani commented 2 weeks ago

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

Checklist

PR Type

Enhancement, Tests

Description

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

    codiumai-pr-agent-pro[bot] commented 2 weeks ago

    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.
    codiumai-pr-agent-pro[bot] commented 2 weeks ago

    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]](https://github.com/SeleniumHQ/selenium/pull/14120/files#diff-c8f0c0185fabe12906d45a35bc7bf890b65f2fba0d6b80040552c5c2176a6d87R91-R93) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14120/files#diff-c8f0c0185fabe12906d45a35bc7bf890b65f2fba0d6b80040552c5c2176a6d87R70-R72) ```diff 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]](https://github.com/SeleniumHQ/selenium/pull/14120/files#diff-f172e17060711f9511bdc0830aac9fba714639b13d06cffcc01988e611aa7862R141-R141) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14120/files#diff-c8f0c0185fabe12906d45a35bc7bf890b65f2fba0d6b80040552c5c2176a6d87R63-R64) ```diff 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