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 authentication handlers #14345

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Aug 5, 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

Related to #13993

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 NetworkEvent constants and private fields #callbackId and #listener to the Network class.
  • Implemented methods for adding, removing, and invoking callbacks in the Network class.
  • Created a new Network class to manage network intercepts and authentication handlers.
  • Integrated the Network class into WebDriver and added a network method for accessing network functionalities.
  • Updated network command tests to handle multiple callback invocations.
  • Added tests for authentication handler functionalities, including adding, removing, and clearing handlers.

Changes walkthrough 📝

Relevant files
Enhancement
network.js
Add callback handling and event subscription enhancements in Network
class

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

  • Added NetworkEvent constants.
  • Introduced private fields #callbackId and #listener.
  • Implemented methods for adding, removing, and invoking callbacks.
  • Modified event subscription to use the new callback mechanism.
  • +53/-6   
    network.js
    Introduce Network class for managing network intercepts and
    authentication

    javascript/node/selenium-webdriver/lib/network.js

  • Created Network class to manage network intercepts and authentication
    handlers.
  • Added methods to add, remove, and clear authentication handlers.
  • +78/-0   
    webdriver.js
    Integrate Network class into WebDriver for network functionalities

    javascript/node/selenium-webdriver/lib/webdriver.js

  • Integrated Network class into WebDriver.
  • Added network method to WebDriver for accessing network
    functionalities.
  • +12/-0   
    Tests
    network_commands_test.js
    Update network command tests to handle multiple callback invocations

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

    • Updated assertions to handle multiple callback invocations.
    +3/-3     
    webdriver_network_test.js
    Add tests for WebDriver network authentication handlers   

    javascript/node/selenium-webdriver/test/lib/webdriver_network_test.js

  • Added tests for authentication handler functionalities.
  • Tested adding, removing, and clearing authentication handlers.
  • +80/-0   

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

    @pujagani pujagani added C-devtools BiDi or Chrome DevTools related issues javascript Pull requests that update Javascript code labels Aug 5, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Error Handling
    The method removeCallback throws an error if the callback ID is not found. It might be beneficial to handle this scenario more gracefully, perhaps by logging a warning instead of throwing, to prevent potential disruptions in the application flow.

    Callback ID Increment
    The addCallback method increments the #callbackId directly. Consider using a more robust ID generation strategy to avoid potential issues with ID conflicts or limits in long-running applications.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add error handling for unsupported event types in callback management

    Add error handling for the get method calls on this.#listener to ensure that the
    event type exists before attempting to manipulate the callback map, preventing
    runtime errors.

    javascript/node/selenium-webdriver/bidi/network.js [60]

     const eventCallbackMap = this.#listener.get(eventType)
    +if (!eventCallbackMap) {
    +  throw new Error(`Event type ${eventType} is not supported.`);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for unsupported event types is a significant improvement that prevents potential runtime errors, enhancing the robustness of the code.

    8
    Provide feedback on callback removal success

    Modify the removeCallback method to return a boolean indicating whether the callback
    was successfully removed, providing clearer feedback to the caller.

    javascript/node/selenium-webdriver/bidi/network.js [74-76]

    -if (!hasId) {
    -  throw Error(`Callback with id ${id} not found`)
    -}
    +return hasId
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Modifying the removeCallback method to return a boolean indicating success provides clearer feedback to the caller, improving usability. However, the current exception handling already provides a clear indication of failure.

    6
    Possible bug
    Ensure the event type is specified before invoking callbacks

    Implement a check to ensure that the eventType parameter in invokeCallbacks method
    is not undefined or null before proceeding with invoking callbacks, to prevent
    potential runtime errors.

    javascript/node/selenium-webdriver/bidi/network.js [80]

    +if (!eventType) {
    +  throw new Error('Event type must be specified.');
    +}
     const callbacks = this.#listener.get(eventType)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Implementing a check to ensure the event type is specified before invoking callbacks helps prevent potential runtime errors, improving code reliability. However, the likelihood of eventType being undefined or null might be low.

    7
    Possible issue
    Improve the reliability of the callback ID increment operation

    Replace the increment operation inside the addCallback method with a safer approach
    using this.#callbackId += 1 to avoid potential issues with the ++ operator when used
    in more complex expressions.

    javascript/node/selenium-webdriver/bidi/network.js [58]

    -const id = ++this.#callbackId
    +const id = this.#callbackId += 1
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion improves the reliability of the callback ID increment operation by using a more explicit and safer approach. However, the original code is not necessarily problematic, so the improvement is minor.

    5

    @harsha509 harsha509 merged commit 1167e3d into SeleniumHQ:trunk Aug 6, 2024
    10 of 11 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    C-devtools BiDi or Chrome DevTools related issues enhancement javascript Pull requests that update Javascript code Review effort [1-5]: 3 tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants