Skip to content

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

Merged
pujagani merged 2 commits into
SeleniumHQ:trunkfrom
pujagani:fix-bidi-tests
Feb 18, 2025
Merged

[java][dotnet][js] Fix failing BiDi related tests#15296
pujagani merged 2 commits into
SeleniumHQ:trunkfrom
pujagani:fix-bidi-tests

Conversation

@pujagani

@pujagani pujagani commented Feb 18, 2025 •
edited by qodo-code-review Bot
Loading

Copy link
Copy Markdown
Contributor

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
    @qodo-code-review

    qodo-code-review Bot commented Feb 18, 2025

    Copy link
    Copy Markdown
    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));

    @qodo-code-review

    qodo-code-review Bot commented Feb 18, 2025

    Copy link
    Copy Markdown
    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
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @pujagani @sandeepsuryaprasad
    [java][dotnet][js] Fix failing BiDi related tests (SeleniumHQ#15296)
    a208f6f
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @pujagani