SeleniumHQ / selenium

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

[js][bidi] Fix the event unsubscribe method. Update modules to have close methods. #14192

Closed pujagani closed 3 days ago

pujagani commented 3 days 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

Motivation and Context

Types of changes

Checklist


PR Type

Bug fix, Enhancement, Tests


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
5 files
browsingContextInspector.js
Add close method for event unsubscription in BrowsingContextInspector.

javascript/node/selenium-webdriver/bidi/browsingContextInspector.js
  • Added close method to handle unsubscription of events.
  • Conditional unsubscription based on _browsingContextIds.
  • +23/-0   
    index.js
    Improve unsubscribe method to handle event removal.           

    javascript/node/selenium-webdriver/bidi/index.js
  • Enhanced unsubscribe method to handle event removal more robustly.
  • Added checks to ensure only existing events are removed.
  • +14/-6   
    logInspector.js
    Add close method for event unsubscription in LogInspector.

    javascript/node/selenium-webdriver/bidi/logInspector.js
  • Added close method to handle unsubscription of log events.
  • Conditional unsubscription based on _browsingContextIds.
  • +9/-1     
    network.js
    Add close method for event unsubscription in Network.       

    javascript/node/selenium-webdriver/bidi/network.js
  • Added close method to handle unsubscription of network events.
  • Conditional unsubscription based on _browsingContextIds.
  • +20/-6   
    scriptManager.js
    Add close method for event unsubscription in ScriptManager.

    javascript/node/selenium-webdriver/bidi/scriptManager.js
  • Added close method to handle unsubscription of script events.
  • Conditional unsubscription based on _browsingContextIds.
  • +17/-0   
    Tests
    6 files
    add_intercept_parameters_test.js
    Ensure network cleanup in add intercept parameters tests.

    javascript/node/selenium-webdriver/test/bidi/add_intercept_parameters_test.js
  • Added network.close() call in afterEach to ensure proper cleanup.
  • Removed redundant network instance creation in each test.
  • +3/-6     
    bidi_test.js
    Ensure inspector cleanup in BiDi tests.                                   

    javascript/node/selenium-webdriver/test/bidi/bidi_test.js
  • Added inspector.close() call in afterEach to ensure proper cleanup.
  • +4/-2     
    browsingcontext_inspector_test.js
    Ensure browsing context inspector cleanup in tests.           

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js
  • Added browsingcontextInspector.close() call in afterEach to ensure
    proper cleanup.
  • Removed redundant browsingcontextInspector instance creation in each
    test.
  • +9/-7     
    locate_nodes_test.js
    Simplify driver setup in locate nodes tests.                         

    javascript/node/selenium-webdriver/test/bidi/locate_nodes_test.js - Removed redundant Firefox-specific configuration in `beforeEach`.
    +2/-2     
    network_test.js
    Ensure network cleanup in network tests.                                 

    javascript/node/selenium-webdriver/test/bidi/network_test.js
  • Added network.close() call in afterEach to ensure proper cleanup.
  • Removed redundant network instance creation in each test.
  • +3/-9     
    script_test.js
    Ensure script manager cleanup in script tests.                     

    javascript/node/selenium-webdriver/test/bidi/script_test.js
  • Added manager.close() call in afterEach to ensure proper cleanup.
  • Removed redundant manager instance creation in each test.
  • +37/-35 

    ๐Ÿ’ก 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 3 days ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 3
    ๐Ÿงช Relevant tests Yes
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The unsubscribe method in various classes (e.g., BrowsingContextInspector, LogInspector, Network, ScriptManager) checks if _browsingContextIds is not null or undefined and has length greater than 0. However, there is no handling for the case where _browsingContextIds might be an empty array, which could lead to unnecessary API calls with empty parameters.
    Redundancy:
    The unsubscribe method in index.js checks if eventsToRemove are in the subscribed events array and then filters them out. This could be simplified by directly filtering this.events without the intermediate check, as the filter operation inherently handles non-existent values.
    codiumai-pr-agent-pro[bot] commented 3 days ago

    Failed to generate code suggestions for PR