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

Update script commands to fix the failing BiDi tests #13737

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Mar 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

W3C BiDi spec was updated to ensure the if browsing context is passed for adding the preload script command, it cannot be empty. This was implemented from Firefox 124 onwards. Updated the command accordingly.

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.

Type

enhancement, bug_fix


Description

  • Updated Script.java to dynamically include contexts in addPreloadScript commands based on non-empty browsingContextIds, enhancing flexibility and adherence to updated W3C BiDi spec.
  • Enabled preload script tests for Firefox in ScriptCommandsTest.java by removing @NotYetImplemented(FIREFOX) annotations, indicating these features are now supported.
  • Modified scriptManager.js in the JavaScript bindings to accept and handle multiple browsing context IDs, allowing for more versatile script management.
  • Adjusted related tests in script_test.js to reflect these changes, ensuring consistency and correctness in testing.

Changes walkthrough

Relevant files
Enhancement
Script.java
Refactor and Update Script Command for Browsing Contexts 

java/src/org/openqa/selenium/bidi/module/Script.java

  • Updated addPreloadScript methods to conditionally include contexts
    parameter based on browsingContextIds being non-empty.
  • Refactored method parameters into a Map for dynamic construction of
    command parameters.
  • +37/-24 
    scriptManager.js
    Support Multiple Browsing Contexts in ScriptManager           

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

  • Modified init to accept multiple browsing context IDs.
  • Adjusted preload script command to support multiple contexts.
  • Fixed event subscription to use updated context IDs variable.
  • +12/-4   
    Tests
    ScriptCommandsTest.java
    Enable Preload Script Tests for Firefox                                   

    java/test/org/openqa/selenium/bidi/script/ScriptCommandsTest.java

  • Removed @NotYetImplemented(FIREFOX) annotations, indicating support
    for Firefox in preload script tests.
  • Minor adjustments to test assertions and parameters.
  • +0/-5     
    script_test.js
    Update ScriptManager Tests for Browsing Context Changes   

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

  • Updated tests to pass an array of browsing context IDs to
    ScriptManager.
  • Renamed a test for clarity.
  • +8/-8     

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

    Copy link

    PR Description updated to latest commit (e6d52db)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly structural, involving updates to parameter handling in method calls. The logic is straightforward and mainly involves checking for empty browsing context IDs and updating method parameters accordingly. The changes are consistent across multiple methods, which simplifies the review process.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The changes assume browsingContextIds is always initialized and can be checked for emptiness. If there's any scenario where browsingContextIds could be null, this could lead to a NullPointerException. It might be safer to also check for nullity.

    Consistency Issue: The JavaScript changes introduce checks for both array and string types of browsingContextIds. It's unclear if similar type flexibility is required or handled in the Java changes. Ensuring consistent handling of browsingContextIds types across different implementations could prevent potential bugs.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    codiumai-pr-agent-pro bot commented Mar 26, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Use Collections.singletonMap for single-entry maps.

    Consider using Collections.singletonMap for single-entry maps to improve readability and
    slightly enhance performance.

    java/src/org/openqa/selenium/bidi/module/Script.java [265-266]

    -parameters.put("functionDeclaration", functionDeclaration);
    +Map<String, Object> parameters = Collections.singletonMap("functionDeclaration", functionDeclaration);
     
    Use template literals for constructing error messages.

    Use template literals for better readability when constructing error messages.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [37]

    -throw Error('WebDriver instance must support BiDi protocol')
    +throw new Error(`WebDriver instance must support BiDi protocol but does not.`);
     
    Maintainability
    Refactor repeated code into a private method to reduce duplication.

    Refactor the repeated code for creating and populating the parameters map into a private
    method to reduce code duplication.

    java/src/org/openqa/selenium/bidi/module/Script.java [283-285]

    -Map<String, Object> parameters = new HashMap<>();
    -parameters.put("functionDeclaration", functionDeclaration);
    -parameters.put("arguments", arguments);
    +private Map<String, Object> createParameters(String functionDeclaration, List<ChannelValue> arguments) {
    +    Map<String, Object> parameters = new HashMap<>();
    +    parameters.put("functionDeclaration", functionDeclaration);
    +    parameters.put("arguments", arguments);
    +    return parameters;
    +}
     
    Simplify logic by combining conditions for this._browsingContextIds.

    Combine the conditions checking this._browsingContextIds to simplify the logic and improve
    code readability.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [176-182]

    -if (Array.isArray(this._browsingContextIds) && this._browsingContextIds.length > 0) {
    -    params.contexts = this._browsingContextIds
    -}
    -if (typeof this._browsingContextIds === 'string') {
    -    params.contexts = new Array(this._browsingContextIds)
    +if (this._browsingContextIds) {
    +    params.contexts = Array.isArray(this._browsingContextIds) ? this._browsingContextIds : [this._browsingContextIds];
     }
     
    Possible issue
    Use consistent property names within the class.

    Ensure consistent use of this.bidi and this._bidi within the class to avoid potential bugs
    related to accessing undefined properties.

    javascript/node/selenium-webdriver/bidi/scriptManager.js [40]

    -this.bidi = await this._driver.getBidi()
    +this._bidi = await this._driver.getBidi()
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you, @pujagani!

    @diemol diemol merged commit 9931d0a into SeleniumHQ:trunk Mar 26, 2024
    14 checks passed
    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.

    None yet

    2 participants