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

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

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

pujagani
Copy link
Contributor

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

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

Bug fix, Enhancement, Tests


Description

  • Added close methods to various modules (BrowsingContextInspector, LogInspector, Network, ScriptManager) to handle event unsubscription.
  • Enhanced unsubscribe method in Index to handle event removal more robustly.
  • Updated tests to ensure proper cleanup by calling close methods in afterEach.
  • Simplified driver setup in locate_nodes_test.js.

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

    Copy link
    Contributor

    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.

    Copy link
    Contributor

    Failed to generate code suggestions for PR

    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