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 chrome tests #13770

Merged
merged 8 commits into from
Apr 4, 2024

Conversation

pujagani
Copy link
Contributor

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

Ensure all BiDi test run where applicable in Chrome since Selenium supports BiDi in Chrome too.

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 for Chrome in ChromeDriverInfo.
  • Enabled various BiDi tests for Chrome across multiple test files, including browsing context, input, network commands, network events, script commands, script events, and storage commands.
  • Marked specific tests as not yet implemented or to be ignored for Chrome, indicating areas that require further development or investigation.
  • Updated Bazel build configurations to include Chrome in the list of browsers for large test suites, facilitating broader test coverage.

Changes walkthrough

Relevant files
Enhancement
1 files
ChromeDriverInfo.java
Enable BiDi Support for Chrome in ChromeDriverInfo             

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

  • Enabled BiDi support for Chrome by returning true in isSupportingBiDi
    method.
  • +1/-1     
    Tests
    10 files
    BrowsingContextInspectorTest.java
    Enable BiDi Tests for Chrome in BrowsingContextInspectorTest

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

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

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

  • Removed @NotYetImplemented(CHROME) annotations to enable BiDi tests
    for Chrome.
  • +0/-11   
    CombinedInputActionsTest.java
    Mark Test as Not Yet Implemented for Chrome in
    CombinedInputActionsTest

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

  • Added @NotYetImplemented(CHROME) annotation, indicating a test not yet
    implemented for Chrome.
  • +1/-0     
    DefaultMouseTest.java
    Ignore Specific Mouse Tests for Chrome in DefaultMouseTest

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

  • Added @Ignore(value = CHROME, gitHubActions = true) to tests,
    indicating they should be ignored for Chrome on GitHub Actions.
  • +2/-0     
    NetworkCommandsTest.java
    Enable Network Command Tests for Chrome in NetworkCommandsTest

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

  • Added @NotYetImplemented(CHROME) annotations to enable network command
    tests for Chrome.
  • +11/-4   
    NetworkEventsTest.java
    Enable Network Events Tests for Chrome in NetworkEventsTest

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

  • Added @NotYetImplemented(CHROME) annotations to enable network events
    tests for Chrome.
  • +3/-3     
    LocalValueTest.java
    Enable Script Local Value Tests for Chrome in LocalValueTest

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

  • Removed @NotYetImplemented(CHROME) annotations to enable script local
    value tests for Chrome.
  • +0/-15   
    ScriptCommandsTest.java
    Enable Script Command Tests for Chrome in ScriptCommandsTest

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

  • Removed @NotYetImplemented(CHROME) annotations to enable script
    command tests for Chrome.
  • +2/-13   
    ScriptEventsTest.java
    Enable Script Event Tests for Chrome in ScriptEventsTest 

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

  • Removed @NotYetImplemented(CHROME) annotations to enable script event
    tests for Chrome.
  • +0/-2     
    StorageCommandsTest.java
    Enable Storage Command Tests for Chrome in StorageCommandsTest

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

  • Removed @NotYetImplemented(CHROME) annotations to enable storage
    command tests for Chrome.
  • +0/-4     
    Configuration changes
    6 files
    BUILD.bazel
    Include Chrome in Large Test Suites in BUILD.bazel             

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

    • Added "chrome" to the list of browsers for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in BrowsingContext Large Test Suites in BUILD.bazel

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

    • Added "chrome" to the list of browsers for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Input Large Test Suites in BUILD.bazel 

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

    • Added "chrome" to the list of browsers for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Log Large Test Suites in BUILD.bazel     

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

    • Added "chrome" to the list of browsers for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Script Large Test Suites in BUILD.bazel

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

    • Added "chrome" to the list of browsers for large tests.
    +1/-0     
    BUILD.bazel
    Include Chrome in Storage Large Test Suites in BUILD.bazel

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

    • Added "chrome" to the list of browsers for large tests.
    +1/-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 draft April 3, 2024 11:27
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 3, 2024

    PR Description updated to latest commit (4dbbce9)

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 3, 2024

    PR Review

    (Review updated until commit f1032eb)

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly annotations and simple boolean value changes, which are straightforward to review. However, the impact of enabling BiDi tests for Chrome across multiple test files requires some understanding of the Selenium testing framework and the BiDi protocol implementation in Chrome.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Test Coverage Gaps: Enabling tests for Chrome without adding new tests might leave some BiDi features in Chrome untested. It's important to ensure that the existing tests comprehensively cover the BiDi functionalities for Chrome.

    Potential Flakiness in Tests: Given the nature of browser testing, especially with features like BiDi that involve asynchronous operations, there's a risk of introducing flaky tests. It would be beneficial to monitor the tests closely after merging this PR to identify and address any flakiness.

    🔒 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

    qodo-merge-pro bot commented Apr 3, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Maintainability
    Replace wildcard import with specific browser imports for clarity and maintainability.

    Instead of importing all browsers statically, consider importing only the browsers you
    need. This will make the code more readable and maintainable.

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

    -import static org.openqa.selenium.testing.drivers.Browser.*;
    +import static org.openqa.selenium.testing.drivers.Browser.CHROME;
    +import static org.openqa.selenium.testing.drivers.Browser.FIREFOX;
    +import static org.openqa.selenium.testing.drivers.Browser.EDGE;
    +import static org.openqa.selenium.testing.drivers.Browser.IE;
    +import static org.openqa.selenium.testing.drivers.Browser.SAFARI;
     
    Enhancement
    Add an assertion to verify the reload operation's success.

    Consider adding an assertion to verify the expected behavior or outcome after the reload
    operation.

    java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java [235]

     NavigationResult reloadInfo = browsingContext.reload();
    +assertThat(reloadInfo.getUrl()).contains("/bidi/logEntryAdded.html");
    +// Ensure the navigation ID is not null to verify the reload was successful
    +assertThat(reloadInfo.getNavigationId()).isNotNull();
     
    Verify script execution result for correctness.

    Consider verifying the script execution result to ensure it behaves as expected.

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

     Script script = new Script(id, driver);
    +// Example of verifying script execution result
    +Object result = script.executeScript("return 'test';");
    +assertThat(result).isEqualTo("test");
     
    Add assertions to verify script execution results.

    To ensure the script execution behaves as expected, consider adding assertions to verify
    the script's effect on the web page or its return value.

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

     Script script = new Script(id, driver);
    +// Assuming the script returns a value or modifies the DOM in a verifiable way
    +Object result = script.executeScript("return document.title;");
    +assertThat(result).isNotNull();
     
    Best practice
    Use specific imports instead of wildcard for browser static members.

    To improve code readability and maintainability, consider importing only the necessary
    static members instead of using a wildcard import.

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

    -import static org.openqa.selenium.testing.drivers.Browser.*;
    +import static org.openqa.selenium.testing.drivers.Browser.CHROME;
    +import static org.openqa.selenium.testing.drivers.Browser.FIREFOX;
    +import static org.openqa.selenium.testing.drivers.Browser.EDGE;
    +import static org.openqa.selenium.testing.drivers.Browser.IE;
    +import static org.openqa.selenium.testing.drivers.Browser.SAFARI;
     

    ✨ 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-commenter commented Apr 4, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.63%. Comparing base (68ad984) to head (efa4189).

    ❗ Current head efa4189 differs from pull request most recent head f1032eb. Consider uploading reports for the commit f1032eb 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   #13770   +/-   ##
    =======================================
      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 marked this pull request as ready for review April 4, 2024 08:42
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 4, 2024

    PR Description updated to latest commit (f1032eb)

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 4, 2024

    Persistent review updated to latest commit f1032eb

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 4, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Replace wildcard import with specific imports.

    Consider using specific imports instead of a wildcard import for better clarity and to
    avoid potential conflicts.

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

    -import static org.openqa.selenium.testing.drivers.Browser.*;
    +import static org.openqa.selenium.testing.drivers.Browser.CHROME;
    +import static org.openqa.selenium.testing.drivers.Browser.EDGE;
    +import static org.openqa.selenium.testing.drivers.Browser.FIREFOX;
    +import static org.openqa.selenium.testing.drivers.Browser.IE;
    +import static org.openqa.selenium.testing.drivers.Browser.SAFARI;
     
    Replace wildcard import with specific imports in NetworkEventsTest.

    Similar to other test files, consider using specific imports instead of a wildcard import
    for clarity and to avoid potential conflicts.

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

    -import static org.openqa.selenium.testing.drivers.Browser.*;
    +import static org.openqa.selenium.testing.drivers.Browser.CHROME;
    +import static org.openqa.selenium.testing.drivers.Browser.EDGE;
    +import static org.openqa.selenium.testing.drivers.Browser.FIREFOX;
    +import static org.openqa.selenium.testing.drivers.Browser.IE;
    +import static org.openqa.selenium.testing.drivers.Browser.SAFARI;
     
    Enhancement
    Remove @NotYetImplemented(CHROME) annotations for Chrome-supported tests.

    Since CHROME is now supported, consider removing the @NotYetImplemented(CHROME)
    annotations to enable tests for Chrome.

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

    -@NotYetImplemented(CHROME)
    +// @NotYetImplemented(CHROME) - Removed as Chrome is now supported.
     
    Possible issue
    Verify full BiDi support before enabling it for ChromeDriver.

    Ensure that the ChromeDriver actually supports BiDi in all required aspects before
    returning true in isSupportingBiDi().

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

    -return true;
    +// Ensure full BiDi support is verified
    +return checkFullBiDiSupport();
     
    Maintainability
    Ensure test cases are fully functional with Chrome support.

    Given that Chrome support is now enabled, ensure that all test cases previously marked
    with @NotYetImplemented(CHROME) are now fully functional and do not require any additional
    modifications.

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

    -// @NotYetImplemented(CHROME) - Removed as Chrome is now supported.
    +// Ensure test cases are fully functional with Chrome support.
     

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

    2 participants