SeleniumHQ / selenium

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

[bazel + js]: Get small js tests running on the rbe #14123

Closed shs96c closed 2 weeks ago

shs96c commented 2 weeks ago

PR Type

Enhancement, Tests


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
17 files
driver_factory.js
Update import paths in driver_factory.js                                 

javascript/node/selenium-webdriver/test/driver_factory.js - Updated import paths to use `selenium-webdriver` package.
+4/-4     
io_test.js
Update import path in io_test.js                                                 

javascript/node/selenium-webdriver/test/io/io_test.js
  • Updated import path for io module to use selenium-webdriver package.
  • +1/-1     
    zip_test.js
    Update import paths in zip_test.js                                             

    javascript/node/selenium-webdriver/test/io/zip_test.js
  • Updated import paths for io, zip, and InvalidArgumentError to use
    selenium-webdriver package.
  • +3/-3     
    by_test.js
    Update import path in by_test.js                                                 

    javascript/node/selenium-webdriver/test/lib/by_test.js
  • Updated import path for by module to use selenium-webdriver package.
  • +1/-1     
    credentials_test.js
    Update import path in credentials_test.js                               

    javascript/node/selenium-webdriver/test/lib/credentials_test.js
  • Updated import path for virtualAuthenticatorCredential to use
    selenium-webdriver package.
  • +1/-1     
    error_test.js
    Update import path in error_test.js                                           

    javascript/node/selenium-webdriver/test/lib/error_test.js
  • Updated import path for error module to use selenium-webdriver
    package.
  • +1/-1     
    http_test.js
    Update import paths in http_test.js                                           

    javascript/node/selenium-webdriver/test/lib/http_test.js
  • Updated import paths for various modules to use selenium-webdriver
    package.
  • +7/-7     
    input_test.js
    Update import paths in input_test.js                                         

    javascript/node/selenium-webdriver/test/lib/input_test.js
  • Updated import paths for various modules to use selenium-webdriver
    package.
  • +4/-4     
    logging_test.js
    Update import path in logging_test.js                                       

    javascript/node/selenium-webdriver/test/lib/logging_test.js
  • Updated import path for logging module to use selenium-webdriver
    package.
  • +1/-1     
    promise_test.js
    Update import path in promise_test.js                                       

    javascript/node/selenium-webdriver/test/lib/promise_test.js
  • Updated import path for promise module to use selenium-webdriver
    package.
  • +1/-1     
    until_test.js
    Update import paths in until_test.js                                         

    javascript/node/selenium-webdriver/test/lib/until_test.js
  • Updated import paths for various modules to use selenium-webdriver
    package.
  • +5/-5     
    virtualauthenticatoroptions_test.js
    Update import paths in virtualauthenticatoroptions_test.js

    javascript/node/selenium-webdriver/test/lib/virtualauthenticatoroptions_test.js
  • Updated import paths for virtualAuthenticatorOptions, Transport, and
    Protocol to use selenium-webdriver package.
  • +3/-3     
    webdriver_test.js
    Update import paths in webdriver_test.js                                 

    javascript/node/selenium-webdriver/test/lib/webdriver_test.js
  • Updated import paths for various modules to use selenium-webdriver
    package.
  • +9/-9     
    logging_test.js
    Update import paths in logging_test.js                                     

    javascript/node/selenium-webdriver/test/logging_test.js
  • Updated import paths for Browser and logging to use selenium-webdriver
    package.
  • +1/-1     
    index_test.js
    Update import path in index_test.js                                           

    javascript/node/selenium-webdriver/test/net/index_test.js
  • Updated import path for net module to use selenium-webdriver package.
  • +1/-1     
    portprober_test.js
    Update import path in portprober_test.js                                 

    javascript/node/selenium-webdriver/test/net/portprober_test.js
  • Updated import path for portprober module to use selenium-webdriver
    package.
  • +1/-1     
    mocha_test.bzl
    Add timeouts and size handling in mocha_test.bzl                 

    javascript/private/mocha_test.bzl
  • Added _TIMEOUTS dictionary for test timeouts.
  • Updated mocha_test function to include size and timeout arguments.
  • +16/-9   
    Configuration changes
    2 files
    ci-javascript.yml
    Update CI workflow to remove small-tests job                         

    .github/workflows/ci-javascript.yml
  • Removed small-tests job.
  • Updated browser-tests job to depend on build instead of small-tests.
  • +1/-10   
    BUILD.bazel
    Update BUILD.bazel for js_library and small-tests               

    javascript/node/selenium-webdriver/BUILD.bazel
  • Added dependencies for js_library.
  • Updated small-tests target to include args and removed unnecessary
    tags.
  • +6/-10   

    ๐Ÿ’ก 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 2 weeks ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 3
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Dependency Management:
    The PR includes updates to import paths across multiple files to use the `selenium-webdriver` package. It's important to ensure that these changes do not break existing functionality and that all dependencies are correctly resolved.
    Code Consistency:
    The changes in import paths should be reviewed for consistency across all modified files to ensure that similar changes adhere to project standards.
    Test Coverage:
    The removal of the `small-tests` job from the CI workflow raises concerns about whether the new test configurations and paths are adequately covered by automated tests.
    codiumai-pr-agent-pro[bot] commented 2 weeks ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Consolidate imports from selenium-webdriver into a single import statement ___ **To ensure compatibility and avoid potential issues with different versions of the
    selenium-webdriver package, consider using a single import statement to import all
    required modules from selenium-webdriver.** [javascript/node/selenium-webdriver/test/driver_factory.js [22-25]](https://github.com/SeleniumHQ/selenium/pull/14123/files#diff-b6b67e8fcc2c453ed2d932911c1ff7a2adcb56c5400c23e318969504163ad7f7R22-R25) ```diff -const { Browser } = require('selenium-webdriver/index') -const { Environment } = require('selenium-webdriver/testing') -const chrome = require('selenium-webdriver/chrome') -const firefox = require('selenium-webdriver/firefox') +const { Browser, Environment, chrome, firefox } = require('selenium-webdriver') ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Consolidating imports can enhance readability and maintainability, but it's not a major issue or bug fix.
    6
    Maintainability
    Group related imports together and separate them with a blank line for better readability ___ **To improve readability and maintainability, consider grouping related imports together and
    separating them with a blank line from other groups of imports.** [javascript/node/selenium-webdriver/test/lib/until_test.js [22-26]](https://github.com/SeleniumHQ/selenium/pull/14123/files#diff-b07f2a6b476ea13ebfd47b823bf1a4d22ca1b05397e68e10b40cd9337b3fef69R22-R26) ```diff -const By = require('selenium-webdriver/lib/by').By -const CommandName = require('selenium-webdriver/lib/command').Name -const error = require('selenium-webdriver/lib/error') -const until = require('selenium-webdriver/lib/until') -const webdriver = require('selenium-webdriver/lib/webdriver') +const { By } = require('selenium-webdriver/lib/by') +const { Name: CommandName } = require('selenium-webdriver/lib/command') +const { error, until } = require('selenium-webdriver/lib') +const { WebElement } = require('selenium-webdriver/lib/webdriver') ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: Grouping related imports improves readability but it's a minor style improvement.
    5
    Possible issue
    Add a default value for the size parameter in the mocha_test function definition ___ **To avoid potential issues with undefined sizes, consider adding a default value for the
    size parameter in the mocha_test function definition.** [javascript/private/mocha_test.bzl [9]](https://github.com/SeleniumHQ/selenium/pull/14123/files#diff-d97fd5df7a340da46ccf4e8610726dd3a78801f1c50a337387d505caf1824d9aR9-R9) ```diff -def mocha_test(name, args = [], env = {}, size = None, **kwargs): +def mocha_test(name, args = [], env = {}, size = "small", **kwargs): ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 4 Why: Adding a default value could prevent potential issues with undefined sizes, but it's not critical as the function handles `None` gracefully.
    4
    shs96c commented 2 weeks ago

    Failing JS tests aren't to do with this PR, and the RBE successfully ran the small JS tests. Calling this a huge success, and landing.