Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
pujagani merged 1 commit into from
Jun 26, 2024
Merged

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

pujagani merged 1 commit into from
Jun 26, 2024

Conversation

@pujagani
Copy link
Contributor

@pujagani pujagani commented Jun 26, 2024 •
edited by qodo-code-review bot
Loading

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

    @qodo-code-review qodo-code-review bot added P-enhancement PR with a new feature P-bug fix PR addresses a known issue tests labels Jun 26, 2024
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 26, 2024

    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.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 26, 2024

    Failed to generate code suggestions for PR

    @pujagani pujagani merged commit abdaa75 into SeleniumHQ:trunk Jun 26, 2024
    @DariuszPiskorowski DariuszPiskorowski mentioned this pull request Aug 9, 2024
    Merged
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @pujagani @sandeepsuryaprasad
    [js][bidi] Fix the event unsubscribe method. Update modules to have c…
    3d6eeed 
    …lose methods. (SeleniumHQ#14192)
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Nov 8, 2024
    Open
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @pujagani @M1troll
    [js][bidi] Fix the event unsubscribe method. Update modules to have c…
    bee02d1 
    …lose methods. (SeleniumHQ#14192)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-bug fix PR addresses a known issue P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @pujagani