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 Edge BiDi tests #13780

Merged
merged 10 commits into from
Apr 8, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Apr 5, 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 Edge BiDi tests

Motivation and Context

Enable Edge BiDi tests to ensure Edge BiDi functionality is supported.

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 Edge browser tests across various BiDi test suites by removing @NotYetImplemented(EDGE) annotations.
  • Added Edge to the list of browsers in Bazel build configurations.

Changes walkthrough

Relevant files
Tests
14 files
BrowserCommandsTest.java
Enable Edge Browser BiDi Tests                                                     

java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java

  • Removed @NotYetImplemented(EDGE) annotations to enable tests for Edge
    browser.
  • +0/-4     
    BrowsingContextInspectorTest.java
    Enable Browsing Context Inspector Tests for Edge                 

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

  • Enabled Edge browser tests by removing @NotYetImplemented(EDGE)
    annotations.
  • Added a TODO comment regarding flaky test for Chrome and Edge.
  • +3/-12   
    BrowsingContextTest.java
    Enable Browsing Context Tests for Edge                                     

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

  • Enabled tests for Edge by removing @NotYetImplemented(EDGE)
    annotations.
  • +0/-5     
    CombinedInputActionsTest.java
    Enable Input Actions Tests for Edge                                           

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

  • Enabled input actions tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +1/-19   
    DefaultKeyboardTest.java
    Enable Keyboard Input Tests for Edge                                         

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

  • Enabled keyboard input tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +0/-11   
    DefaultMouseTest.java
    Enable Mouse Input Tests for Edge                                               

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

  • Enabled mouse input tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +1/-17   
    DefaultWheelTest.java
    Enable Wheel Input Tests for Edge                                               

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

  • Enabled wheel input tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +0/-3     
    DragAndDropTest.java
    Enable Drag and Drop Tests for Edge                                           

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

  • Enabled drag and drop tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +0/-5     
    ReleaseCommandTest.java
    Enable Release Command Tests for Edge                                       

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

  • Enabled release command tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +0/-2     
    SetFilesCommandTest.java
    Enable Set Files Command Tests for Edge                                   

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

  • Enabled set files command tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +0/-5     
    LocalValueTest.java
    Enable Local Value Tests for Edge                                               

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

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

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

  • Enabled script commands tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +1/-14   
    ScriptEventsTest.java
    Enable Script Events Tests for Edge                                           

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

  • Enabled script events tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +1/-7     
    StorageCommandsTest.java
    Enable Storage Commands Tests for Edge                                     

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

  • Enabled storage commands tests for Edge by removing
    @NotYetImplemented(EDGE) annotations.
  • +1/-10   
    Configuration changes
    1 files
    BUILD.bazel
    Update Bazel Build to Include Edge Browser Tests                 

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

    • Added Edge to the list of browsers for testing.
    +1/-0     

    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 Description updated to latest commit (ac4e12b)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the PR primarily involves enabling tests for the Edge browser by removing the @NotYetImplemented(EDGE) annotations across various test classes. The changes are straightforward and consistent across multiple files, making it relatively easy to review. However, familiarity with the Selenium testing framework and the specific requirements for Edge browser support is necessary.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Flaky Test: The comment added in BrowsingContextInspectorTest.java mentions a flaky test related to comparing browsing context id for Chrome and Edge. It's important to address the flakiness to ensure reliable test results.

    🔒 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
    Add a test case for EDGE browser.

    Consider adding a test case for the EDGE browser now that the @NotYetImplemented(EDGE)
    annotation has been removed. This ensures that the functionality is verified to work as
    expected on EDGE.

    java/test/org/openqa/selenium/bidi/input/DefaultMouseTest.java [111-114]

     @Test
     @NotYetImplemented(SAFARI)
     @NotYetImplemented(IE)
    +// Add a test case specifically for EDGE if applicable
     public void testDraggingElementWithMouseMovesItToAnotherList() {
     
    Ensure test coverage for EDGE browser.

    Given that the @NotYetImplemented(EDGE) annotation has been removed, ensure that there are
    test cases covering the EDGE browser to verify the script command functionalities.

    java/test/org/openqa/selenium/bidi/script/ScriptCommandsTest.java [62-65]

     @Test
     @NotYetImplemented(SAFARI)
     @NotYetImplemented(IE)
    +// Consider adding a test case for EDGE
     void canCallFunctionWithDeclaration() {
     
    Verify browsing context functionalities for EDGE.

    After removing the @NotYetImplemented(EDGE) annotation, it's important to verify that the
    browsing context functionalities are correctly implemented for the EDGE browser by adding
    specific test cases if necessary.

    java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [49-52]

     @Test
     @NotYetImplemented(SAFARI)
     @NotYetImplemented(IE)
    +// Add test cases for EDGE if needed
     void canListenToWindowBrowsingContextCreatedEvent() {
     
    Maintainability
    Import only used browsers instead of all.

    Instead of importing all browsers statically, consider importing only the browsers that
    are actually used in the test cases. This can help in maintaining clarity and reducing
    unnecessary dependencies.

    java/test/org/openqa/selenium/bidi/input/CombinedInputActionsTest.java [29]

    -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;
    +// Add other browsers as needed
     
    Sort the browser names alphabetically within the browsers list.

    Consider sorting the browser names alphabetically within the browsers list to maintain
    consistency and improve readability. This can help in quickly identifying if a specific
    browser is included in the tests and ensures that the list is organized as more browsers
    might be added in the future.

    java/test/org/openqa/selenium/bidi/script/BUILD.bazel [9-11]

     browsers = [
         "chrome",
         "edge",
    -    "firefox",
    +    "firefox",  # Ensure this list is sorted alphabetically
     ],
     
    Possible issue
    Confirm EDGE configurations in BUILD.bazel.

    Ensure that the addition of "edge" to the browsers list in the BUILD.bazel file is
    intentional and that all necessary configurations and dependencies for running tests on
    EDGE are properly set up.

    java/test/org/openqa/selenium/bidi/input/BUILD.bazel [10]

     browsers = [
         "chrome",
    -    "edge",
    +    "edge",  # Confirm that EDGE configurations are correctly set up
         "firefox",
     ],
     

    ✨ 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