Skip to content

[js][bidi]: fix chrome and firefox test for CI RBE#15405

Merged
titusfortner merged 1 commit intoSeleniumHQ:trunkfrom
navin772:fix-js-bidi-test
Mar 12, 2025
Merged

[js][bidi]: fix chrome and firefox test for CI RBE#15405
titusfortner merged 1 commit intoSeleniumHQ:trunkfrom
navin772:fix-js-bidi-test

Conversation

@navin772
Copy link
Copy Markdown
Member

@navin772 navin772 commented Mar 12, 2025 •
edited by qodo-code-review bot
Loading

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

Fix chrome and firefox test for CI RBE for JS.
Resolves the CI failures

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

Description

  • Fixed test assertion for children in browsing context inspector.

  • Ensured children is validated as an empty array.

  • Improved robustness of CI tests for Chrome and Firefox.

Changes walkthrough 📝

Relevant files
Bug fix
browsingcontext_inspector_test.js
Fix and enhance browsing context inspector test                   

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

  • Updated assertion for children to check for an empty array.
  • Replaced null check with array validation for children.
  • Enhanced test reliability for CI environments.
    		• +5/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 12, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 12, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use specific array assertion

    Use a more specific assertion method like assert.deepStrictEqual() to directly
    compare with an empty array, which provides clearer error messages and is more
    idiomatic for array checking.

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js [72-75]

    -assert(
-  Array.isArray(contextInfo.children) && contextInfo.children.length === 0,
-  'children should be an empty array',
+assert.deepStrictEqual(
+  contextInfo.children,
+  [],
+  'children should be an empty array'
 )
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion improves code readability and error reporting by using assert.deepStrictEqual() instead of a generic assertion with a condition. This is a better practice for array comparison as it will show the actual difference in values if the test fails.

    		Low
    • More

    @navin772 navin772 requested a review from harsha509 March 12, 2025 13:28
    @navin772 navin772 added the C-nodejs JavaScript Bindings label Mar 12, 2025
    @titusfortner
    Copy link
    Copy Markdown
    Member

    titusfortner commented Mar 12, 2025

    Thanks for looking into this and getting it passing!
    Did the spec change, or was the assertion wrong before?

    @titusfortner
    Copy link
    Copy Markdown
    Member

    titusfortner commented Mar 12, 2025

    This seems like a regression. The spec seems to indicate it should be null:
    https://www.w3.org/TR/webdriver-bidi/#type-browsingContext-Info

    @shs96c
    Copy link
    Copy Markdown
    Member

    shs96c commented Mar 12, 2025

    Reading the spec, we see that in step 6 Let child infos be null. but then in step 7 (If max depth is null, or max depth is greater than 0:) we hit step 7.3, which sets child infos to the empty list. In step 12, the child infos is added to the return value. So, theoretically it's possible for children to be either null (if max depth is less than 0) but it's likely to be an empty list in most cases.

    @titusfortner titusfortner merged commit 9a53f27 into SeleniumHQ:trunk Mar 12, 2025
    10 of 11 checks passed
    @navin772 navin772 deleted the fix-js-bidi-test branch March 12, 2025 18:38
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @navin772 @sandeepsuryaprasad
    [js][bidi]: fix chrome and firefox test for CI RBE (SeleniumHQ#15405)
    f9a875c 
    fix chrome and firefox test for CI RBE
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    This was referenced Dec 21, 2025
    Open
    Closed
    Closed
    Closed
    This was referenced Jan 19, 2026
    Closed
    Closed
    Closed
    Merged
    This was referenced Feb 16, 2026
    Closed
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @harsha509 harsha509 Awaiting requested review from harsha509

    Assignees

    No one assigned

    Labels

    C-nodejs JavaScript Bindings Review effort 1/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @navin772 @titusfortner @shs96c