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] Enable tests for Edge and Chrome #13790

Merged
merged 5 commits into from
Apr 10, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Apr 9, 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

Enable tests for Edge and Chrome for BiDi APIs.

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, tests


Description

  • Enabled BiDi tests for Chrome and Edge in addition to Firefox across various test files.
  • Added null check for sandbox parameter in scriptManager.js.
  • Fixed and enabled previously skipped tests related to user contexts and browsing contexts for multiple browsers.
  • Updated tests to support BiDi in Firefox, Chrome, and Edge for input, local value, locate nodes, log inspector, network commands, network, script, setFiles command, and storage.
  • Added capability to enable BiDi for Firefox, Chrome, and Edge in testing/index.js.

Changes walkthrough

Relevant files
Enhancement
1 files
scriptManager.js
Add null check for sandbox parameter in addPreloadScript 

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

  • Added a check to only include sandbox parameter if it's not null in
    addPreloadScript method.
  • +4/-1     
    Tests
    14 files
    add_intercept_parameters_test.js
    Enable BiDi tests for Chrome and Edge                                       

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

  • Enabled tests for Chrome and Edge in addition to Firefox.
  • Removed unused import.
  • +2/-3     
    bidi_session_test.js
    Enable BiDi session tests for multiple browsers                   

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

    • Enabled BiDi session tests for Chrome and Edge browsers.
    +2/-3     
    browser_test.js
    Enable user context tests for BiDi in multiple browsers   

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

  • Enabled previously skipped tests for creating, getting, and removing
    user contexts.
  • Tests are now enabled for Firefox, Chrome, and Edge.
  • +5/-6     
    browsingcontext_inspector_test.js
    Update browsing context inspector tests for BiDi                 

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

  • Enabled navigation started event test for all browsers except Chrome
    and Edge.
  • Other tests enabled for Firefox, Chrome, and Edge.
  • +19/-17 
    browsingcontext_test.js
    Enable browsing context tests for BiDi in multiple browsers

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

  • Enabled various browsing context tests for Firefox, Chrome, and Edge.
  • Added ignore for set viewport test on Firefox.
  • +5/-6     
    input_test.js
    Enable input tests for BiDi in multiple browsers                 

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

  • Enabled input tests for Firefox, Chrome, and Edge.
  • Marked move to and click element in an iframe test as ignored.
  • +4/-5     
    local_value_test.js
    Enable local value tests for BiDi in multiple browsers     

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

    • Enabled local value tests for Firefox, Chrome, and Edge.
    +2/-3     
    locate_nodes_test.js
    Enable locate nodes tests for BiDi in multiple browsers   

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

    • Enabled various locate nodes tests for Firefox, Chrome, and Edge.
    +8/-8     
    log_inspector_test.js
    Enable log inspector tests for BiDi in multiple browsers 

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

    • Enabled log inspector tests for Firefox, Chrome, and Edge.
    +1/-2     
    network_commands_test.js
    Enable network commands tests for BiDi in multiple browsers

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

    • Enabled network commands tests for Firefox, Chrome, and Edge.
    +10/-11 
    network_test.js
    Enable network tests for BiDi in multiple browsers             

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

  • Enabled network tests for Firefox, Chrome, and Edge.
  • Added ignore for redirect http equiv test on Chrome and Edge.
  • +7/-8     
    script_test.js
    Enable script tests for BiDi in multiple browsers               

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

    • Enabled script tests for Firefox, Chrome, and Edge.
    +6/-7     
    setFiles_command_test.js
    Enable set files command tests for BiDi in Chrome and Edge

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

  • Enabled set files command tests for Chrome and Edge, ignoring Firefox.

  • +5/-5     
    storage_test.js
    Enable storage tests for BiDi in multiple browsers             

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

    • Enabled storage tests for Firefox, Chrome, and Edge.
    +8/-9     
    Configuration changes
    1 files
    index.js
    Enable BiDi capability for multiple browsers                         

    javascript/node/selenium-webdriver/testing/index.js

    • Added capability to enable BiDi for Firefox, Chrome, and Edge.
    +5/-0     

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

    @pujagani pujagani marked this pull request as ready for review April 10, 2024 10:04
    Copy link
    Contributor

    PR Description updated to latest commit (1a14227)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the PR primarily focuses on enabling tests for additional browsers (Edge and Chrome) alongside Firefox, which were previously exclusive. The changes are straightforward, involving adjustments to test configurations and minor modifications to the script and network handling logic to accommodate these browsers. The effort required is moderate due to the need to understand the context of BiDi APIs and how they interact with different browsers, but the changes themselves are not complex.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Browser Compatibility: While the PR aims to enable tests for Edge and Chrome, it's crucial to ensure that all the BiDi API functionalities being tested are fully supported across these browsers. Any discrepancies in API support could lead to false negatives in the test results.

    Conditional Test Execution: The use of ignore and conditional test execution (e.g., ignore(env.browsers(Browser.FIREFOX)).it(...)) might mask potential issues in browsers that are not fully tested. It's important to have a clear strategy for dealing with browser-specific issues and ensuring that tests provide meaningful coverage across all targeted browsers.

    🔒 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
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Use a more idiomatic assertion for truthiness.

    Consider using assert.ok(intercept) instead of assert.notEqual(intercept, null) for a more
    idiomatic assertion that intercept is truthy.

    javascript/node/selenium-webdriver/test/bidi/network_commands_test.js [50]

    -assert.notEqual(intercept, null)
    +assert.ok(intercept)
     
    Improve assertion precision by checking for specific error messages.

    Instead of asserting fetchErrorEvent.errorText is not null, consider asserting that it
    contains a specific error message for more precise testing.

    javascript/node/selenium-webdriver/test/bidi/network_test.js [194]

    -assert.notEqual(fetchErrorEvent.errorText, null)
    +assert.strictEqual(fetchErrorEvent.errorText, 'Expected error message')
     
    Use direct comparison for domain assertion.

    Use assert.strictEqual instead of assert.strictEqual(resultCookie.domain.includes('http'),
    true) to directly compare the expected domain value.

    javascript/node/selenium-webdriver/test/bidi/storage_test.js [146]

    -assert.strictEqual(resultCookie.domain.includes('http'), true)
    +assert.strictEqual(resultCookie.domain, 'expected.domain.com')
     
    Improve error message assertion specificity.

    Instead of using assert.equal with .includes('SyntaxError:'), directly assert the expected
    syntax error message with assert.strictEqual for clarity and specificity.

    javascript/node/selenium-webdriver/test/bidi/script_test.js [217]

    -assert.equal(result.exceptionDetails.text.includes('SyntaxError:'), true)
    +assert.strictEqual(result.exceptionDetails.text, 'SyntaxError: expected expression, got \')\'')
     
    Check browser version before enabling BiDi to ensure compatibility.

    Ensure to check the browser version along with the browser name before enabling BiDi, to
    avoid enabling it on versions that do not support BiDi.

    javascript/node/selenium-webdriver/testing/index.js [306-308]

    -if (browser.name === Browser.FIREFOX || browser.name === Browser.CHROME || browser.name === Browser.EDGE) {
    +if ((browser.name === Browser.FIREFOX && browser.version >= 'specific_version') || (browser.name === Browser.CHROME && browser.version >= 'specific_version') || (browser.name === Browser.EDGE && browser.version >= 'specific_version')) {
       builder.setCapability('webSocketUrl', true)
     }
     
    Avoid sending unnecessary parameters by checking for undefined.

    Ensure sandbox parameter is only added to params if it is not undefined, to avoid sending
    unnecessary parameters in requests.

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

    -if (sandbox !== null) {
    +if (sandbox !== undefined) {
       params.sandbox = sandbox
     }
     
    Best practice
    Use destructuring for cleaner imports.

    Use destructuring to import AddInterceptParameters and InterceptPhase if they are
    available from a module, to make the code cleaner and more readable.

    javascript/node/selenium-webdriver/test/bidi/network_commands_test.js [49]

    +const { AddInterceptParameters, InterceptPhase } = require('../../bidi/network')
     const intercept = await network.addIntercept(new AddInterceptParameters(InterceptPhase.BEFORE_REQUEST_SENT))
     
    Use const for variable declaration to prevent accidental reassignment.

    Consider using const for driver declaration inside beforeEach to ensure it's not
    accidentally reassigned within the test scope, enhancing code readability and
    maintainability.

    javascript/node/selenium-webdriver/test/bidi/input_test.js [32-33]

    -let driver
    +const driver
     
    Possible issue
    Ensure tests are executed by using it instead of xit.

    Replace the xit function with it to ensure the test 'can move to and click element in an
    iframe' is executed during the test runs.

    javascript/node/selenium-webdriver/test/bidi/input_test.js [170]

    -xit('can move to and click element in an iframe', async function () {
    +it('can move to and click element in an iframe', async function () {
     

    ✨ 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.

    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