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

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

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented Jun 12, 2024

PR Type

Enhancement, Tests


Description

  • Updated import paths in multiple test files to use selenium-webdriver package.
  • Added _TIMEOUTS dictionary and updated mocha_test function in mocha_test.bzl to handle test timeouts.
  • Removed small-tests job from CI workflow and updated dependencies.
  • Updated BUILD.bazel to include new dependencies and modify small-tests target.

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

    Copy link
    Contributor

    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.

    Copy link
    Contributor

    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]

    -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]

    -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]

    -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
    Copy link
    Member Author

    shs96c commented Jun 12, 2024

    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.

    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