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][java] Enable grid BiDi chrome test #13778

Merged
merged 11 commits into from
Apr 5, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Apr 4, 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 grid BiDi Chrome test to ensure Augmenter works as expected and Grid supports BiDi.

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 support in ChromeDriverInfo.
  • Enabled various BiDi related tests for Chrome, including browsing context, input actions, network commands, script commands, and storage commands.
  • Added setup and cleanup methods in RemoteWebDriverBiDiTest and enabled tests for detected browsers, excluding IE, Safari, and Edge.
  • Updated Bazel configurations to include Chrome in large tests and refactor RemoteWebDriverBiDiTest configuration.

Changes walkthrough

Relevant files
Enhancement
2 files
ChromeDriverInfo.java
Enable BiDi Support in ChromeDriverInfo                                   

java/src/org/openqa/selenium/chrome/ChromeDriverInfo.java

  • Enabled BiDi support by returning true in isSupportingBiDi method.
  • +1/-1     
    RemoteWebDriverBiDiTest.java
    Enhance RemoteWebDriverBiDiTest with Setup and Browser Detection

    java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java

  • Added setup and cleanup methods.
  • Enabled BiDi session creation and other tests for detected browsers,
    ignoring IE, Safari, and Edge.
  • +31/-56 
    Tests
    10 files
    BrowsingContextInspectorTest.java
    Enable Browsing Context Inspector Tests for Chrome             

    java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java

  • Removed @NotYetImplemented(CHROME) annotations to enable tests for
    Chrome.
  • +0/-8     
    BrowsingContextTest.java
    Enable Browsing Context Tests for Chrome                                 

    java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java

  • Enabled various browsing context tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +0/-11   
    CombinedInputActionsTest.java
    Mark Combined Input Actions Test as Not Yet Implemented for Chrome

    java/test/org/openqa/selenium/bidi/input/CombinedInputActionsTest.java

  • Added @NotYetImplemented(CHROME) annotation to a test, marking it not
    yet implemented for Chrome.
  • +1/-0     
    DefaultMouseTest.java
    Ignore Default Mouse Tests for Chrome on GitHub Actions   

    java/test/org/openqa/selenium/bidi/input/DefaultMouseTest.java

  • Added @Ignore(value = CHROME, gitHubActions = true) to tests, ignoring
    them for Chrome on GitHub Actions.
  • +2/-0     
    NetworkCommandsTest.java
    Enable Network Commands Tests for Chrome                                 

    java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java

  • Enabled network commands tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +11/-4   
    NetworkEventsTest.java
    Enable Network Events Tests for Chrome                                     

    java/test/org/openqa/selenium/bidi/network/NetworkEventsTest.java

  • Enabled network events tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +3/-3     
    LocalValueTest.java
    Enable Script Local Value Tests for Chrome                             

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

  • Enabled script local value tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +0/-15   
    ScriptCommandsTest.java
    Enable Script Commands Tests for Chrome                                   

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

  • Enabled script commands tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +2/-13   
    ScriptEventsTest.java
    Enable Script Events Tests for Chrome                                       

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

  • Enabled script events tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +0/-2     
    StorageCommandsTest.java
    Enable Storage Commands Tests for Chrome                                 

    java/test/org/openqa/selenium/bidi/storage/StorageCommandsTest.java

  • Enabled storage commands tests for Chrome by removing
    @NotYetImplemented(CHROME) annotations.
  • +0/-4     
    Configuration changes
    7 files
    BUILD.bazel
    Include Chrome in Bazel Large Tests Configuration               

    java/test/org/openqa/selenium/bidi/browser/BUILD.bazel

    • Added "chrome" to the browsers list for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Bazel Browsing Context Tests Configuration

    java/test/org/openqa/selenium/bidi/browsingcontext/BUILD.bazel

    • Added "chrome" to the browsers list for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Bazel Input Tests Configuration               

    java/test/org/openqa/selenium/bidi/input/BUILD.bazel

    • Added "chrome" to the browsers list for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Bazel Log Tests Configuration                   

    java/test/org/openqa/selenium/bidi/log/BUILD.bazel

    • Added "chrome" to the browsers list for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Bazel Script Tests Configuration             

    java/test/org/openqa/selenium/bidi/script/BUILD.bazel

    • Added "chrome" to the browsers list for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Bazel Storage Tests Configuration           

    java/test/org/openqa/selenium/bidi/storage/BUILD.bazel

    • Added "chrome" to the browsers list for large tests.
    +1/-0     
    BUILD.bazel
    Refactor RemoteWebDriverBiDiTest Bazel Configuration         

    java/test/org/openqa/selenium/grid/router/BUILD.bazel

  • Removed separate firefox-only tests configuration.
  • Included BiDi related dependencies and tests in the main
    configuration.
  • +5/-26   

    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 (44f636d)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the changes involve enabling BiDi (Bidirectional) support for Chrome in Selenium tests, which requires understanding of both the Selenium framework and the BiDi protocol. The changes are spread across multiple files and test cases, indicating a moderate level of complexity. However, the modifications are mostly straightforward, such as enabling tests for Chrome and changing boolean flags.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Test Coverage: While the PR enables BiDi tests for Chrome, it's important to ensure that all relevant test scenarios are covered. This includes not just enabling existing tests, but also adding new tests if necessary to cover any Chrome-specific BiDi behaviors.

    Browser Compatibility: The changes assume that the BiDi implementation in Chrome is compatible with the tests that were previously marked as not yet implemented. It's crucial to validate this assumption across different versions of Chrome to ensure the tests are reliable.

    🔒 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

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Ensure resources are always cleaned up properly.

    Consider using try-with-resources or finally block to ensure driver.quit() and
    server.stop() are always executed even if exceptions occur.

    java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java [136-137]

    -driver.quit();
    -server.stop();
    +try {
    +    // test code
    +} finally {
    +    driver.quit();
    +    server.stop();
    +}
     
    Validate variable content before use.

    To ensure the test's robustness, consider verifying the intercept variable's content or
    format before using it in network operations.

    java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java [62]

    -String intercept =
    +String intercept = // ensure it's a valid intercept value
    +assertNotNull(intercept, "Intercept should not be null");
     
    Verify new dependencies for version conflicts or build time impact.

    After adding new dependencies, verify that they do not introduce version conflicts or
    increase the build time significantly. Consider using dependency analysis tools to check
    for potential issues.

    java/test/org/openqa/selenium/grid/router/BUILD.bazel [56-59]

    -"//java/src/org/openqa/selenium/bidi",
    +"//java/src/org/openqa/selenium/bidi", // Verify no version conflicts
     "//java/src/org/openqa/selenium/bidi/browsingcontext",
     "//java/src/org/openqa/selenium/bidi/log",
     "//java/src/org/openqa/selenium/bidi/module",
     
    Enhancement
    Improve test reliability by waiting for page load.

    To improve test reliability, consider adding explicit waits or assertions to ensure the
    page and elements are fully loaded before interacting with them.

    java/test/org/openqa/selenium/bidi/script/ScriptCommandsTest.java [122]

     driver.get(url);
    +new WebDriverWait(driver, Duration.ofSeconds(10)).until(
    +    webDriver -> ((JavascriptExecutor) webDriver).executeScript("return document.readyState").equals("complete"));
     
    Assert script execution results for accuracy.

    To improve test accuracy, consider asserting the results of script executions to verify
    they meet expected outcomes.

    java/test/org/openqa/selenium/bidi/script/ScriptCommandsTest.java [189-190]

     Script script = new Script(id, driver);
    +Object result = script.executeScript("return true;");
    +assertTrue((Boolean) result, "Script execution did not return the expected result.");
     
    Ensure all large tests pass with Chrome without browser-specific issues.

    Adding Chrome to the list of browsers for large tests implies significant changes. Ensure
    that all tests pass and that there are no browser-specific issues that could affect the
    test outcomes.

    java/test/org/openqa/selenium/bidi/browsingcontext/BUILD.bazel [9]

    -"chrome",
    +"chrome", // Ensure all tests pass with Chrome
     
    Maintainability
    Use descriptive variable names for better clarity.

    To enhance test clarity and maintainability, consider using descriptive variable names for
    CompletableFuture instances related to specific network events.

    java/test/org/openqa/selenium/bidi/network/NetworkEventsTest.java [177]

    -CompletableFuture<FetchError> future = new CompletableFuture<>();
    +CompletableFuture<FetchError> fetchErrorFuture = new CompletableFuture<>();
     
    Consolidate multiple @Ignore annotations into a single annotation.

    Consider consolidating the @Ignore annotations for different browsers into a single
    annotation if the test framework supports it. This can improve readability and
    maintainability of the test annotations.

    java/test/org/openqa/selenium/bidi/input/DefaultMouseTest.java [226-227]

    -@Ignore(value = FIREFOX, gitHubActions = true)
    -@Ignore(value = CHROME, gitHubActions = true)
    +@Ignore(value = {FIREFOX, CHROME}, gitHubActions = true)
     
    Possible issue
    Verify compatibility of BiDi support with Chrome versions.

    Ensure that enabling BiDi support for ChromeDriver is well-tested and compatible with all
    intended Chrome versions, as this change affects the driver's capabilities and might
    impact existing tests or user scripts.

    java/src/org/openqa/selenium/chrome/ChromeDriverInfo.java [63]

    -return true;
    +return true; // Ensure compatibility with intended Chrome versions
     
    Maintain or adapt tests for Firefox-specific features to ensure coverage.

    Removing the firefox-only-large-tests suite may impact test coverage for Firefox-specific
    features. Consider maintaining or adapting these tests to ensure comprehensive test
    coverage.

    java/test/org/openqa/selenium/grid/router/BUILD.bazel [85]

    -exclude = LARGE_TESTS,
    +exclude = LARGE_TESTS, // Ensure Firefox-specific features are tested
     

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

    @codecov-commenter
    Copy link

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.63%. Comparing base (68ad984) to head (3148e63).
    Report is 3 commits behind head on trunk.

    ❗ Current head 3148e63 differs from pull request most recent head 9ba73d5. Consider uploading reports for the commit 9ba73d5 to get more accurate results

    ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

    Additional details and impacted files
    @@           Coverage Diff           @@
    ##            trunk   #13778   +/-   ##
    =======================================
      Coverage   58.63%   58.63%           
    =======================================
      Files          86       86           
      Lines        5272     5272           
      Branches      218      218           
    =======================================
      Hits         3091     3091           
      Misses       1963     1963           
      Partials      218      218           

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @pujagani pujagani merged commit 38829c7 into SeleniumHQ:trunk Apr 5, 2024
    36 of 38 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