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

[java][dotnet][js] Fix failing BiDi related tests #15296

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Feb 18, 2025

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.

Motivation and Context

Prepping for the release

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.

PR Type

Bug fix, Tests


Description

  • Fixed BiDi-related test failures across Java, .NET, and JavaScript.

  • Updated EvaluateParameters to remove userActivation in Java and .NET tests.

  • Adjusted JavaScript test to ignore specific browsers for fragment navigation.

  • Improved test consistency and alignment with specifications.


Changes walkthrough 📝

Relevant files
Tests
CallFunctionParameterTest.java
Adjusted `EvaluateParameters` usage in Java tests               

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

  • Removed userActivation parameter in EvaluateParameters.
  • Ensured consistent evaluation of scripts in tests.
  • +1/-2     
    EvaluateParametersTest.java
    Simplified script evaluation in Java tests                             

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

  • Removed userActivation parameter in EvaluateParameters.
  • Simplified script evaluation logic in tests.
  • +1/-2     
    CallFunctionParameterTest.cs
    Updated BiDi script evaluation in .NET tests                         

    dotnet/test/common/BiDi/Script/CallFunctionParameterTest.cs

  • Removed UserActivation parameter in EvaluateAsync.
  • Adjusted test logic for script evaluation.
  • +1/-1     
    EvaluateParametersTest.cs
    Enhanced script evaluation tests in .NET                                 

    dotnet/test/common/BiDi/Script/EvaluateParametersTest.cs

  • Removed UserActivation parameter in EvaluateAsync.
  • Improved test consistency for script evaluation.
  • +1/-1     
    browsingcontext_inspector_test.js
    Adjusted fragment navigation test in JavaScript                   

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js

  • Ignored fragment navigation test for Chrome and Edge.
  • Added comments for browser-specific behavior.
  • Improved test alignment with browser specifications.
  • +19/-14 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @pujagani pujagani marked this pull request as ready for review February 18, 2025 12:35
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Coverage

    The fragment navigation test is now skipped for Chrome and Edge browsers. Consider adding alternative test coverage or documenting why this functionality cannot be tested on these browsers.

    ignore(env.browsers(Browser.EDGE, Browser.CHROME)).it(
      'can listen to fragment navigated event',
      async function () {
        let navigationInfo = null
        const browsingConextInspector = await BrowsingContextInspector(driver)
    
        const browsingContext = await BrowsingContext(driver, {
          browsingContextId: await driver.getWindowHandle(),
        })
        await browsingContext.navigate(Pages.linkedImage, 'complete')
    
        await browsingConextInspector.onFragmentNavigated((entry) => {
          navigationInfo = entry
        })
    
        await browsingContext.navigate(Pages.linkedImage + '#linkToAnchorOnThisPage', 'complete')
    
        // Chrome/Edge do not return the window's browsing context id as per the spec.
        // This assertion fails.
        assert.equal(navigationInfo.browsingContextId, browsingContext.id)
        assert(navigationInfo.url.includes('linkToAnchorOnThisPage'))
      },
    )
    Parameter Change

    The userActivation parameter was removed from EvaluateParameters. Verify this change aligns with the BiDi specification and doesn't break expected browser behavior.

    script.evaluateFunction(
        new EvaluateParameters(new ContextTarget(id), "window.open();", false));

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix inconsistent test parameter value

    The false parameter in evaluateFunction seems incorrect for a test named
    "canEvaluateScriptWithUserActivationTrue". This could lead to incorrect test
    behavior since it contradicts the test's intention.

    java/test/org/openqa/selenium/bidi/script/CallFunctionParameterTest.java [62-63]

     script.evaluateFunction(
    -    new EvaluateParameters(new ContextTarget(id), "window.open();", false));
    +    new EvaluateParameters(new ContextTarget(id), "window.open();", true));
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical inconsistency where the test named "canEvaluateScriptWithUserActivationTrue" uses false for user activation, which contradicts the test's purpose and could lead to incorrect test behavior.

    High
    Fix contradictory test parameter

    Similar to the previous test, the false parameter contradicts the test method
    name "canEvaluateScriptWithUserActivationTrue" and could cause test failures.

    java/test/org/openqa/selenium/bidi/script/EvaluateParametersTest.java [56-57]

     script.evaluateFunction(
    -    new EvaluateParameters(new ContextTarget(id), "window.open();", false));
    +    new EvaluateParameters(new ContextTarget(id), "window.open();", true));
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion identifies the same critical issue in another test file where the parameter value contradicts the test method name, which could cause test validation failures and incorrect behavior verification.

    High
    Learned
    best practice
    Enhance debugging comments by including specific details about expected versus actual behavior

    The comment about Chrome/Edge behavior should be more specific and include
    details about what the actual behavior is versus what's expected according to
    the spec. This helps other developers better understand the issue.

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js [161-163]

    -// Chrome/Edge do not return the window's browsing context id as per the spec.
    -// This assertion fails.
    +// Chrome/Edge return null for browsingContextId instead of the window's context id.
    +// Spec requires: browsingContextId to match the window handle.
    +// Actual: Chrome/Edge return null. See: https://w3c.github.io/webdriver-bidi/#navigation
     assert.equal(navigationInfo.browsingContextId, browsingContext.id)
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    • More

    @pujagani pujagani merged commit 95fb4ce into SeleniumHQ:trunk Feb 18, 2025
    11 of 12 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.

    1 participant