Skip to content

[js][bidi]: fix flaky bidi network test - can request cookies#16141

Merged
diemol merged 2 commits intoSeleniumHQ:trunkfrom
navin772:fix-js-bidi-network-test
Aug 7, 2025
Merged

[js][bidi]: fix flaky bidi network test - can request cookies#16141
diemol merged 2 commits intoSeleniumHQ:trunkfrom
navin772:fix-js-bidi-network-test

Conversation

@navin772
Copy link
Copy Markdown
Member

@navin772 navin772 commented Aug 7, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

💥 What does this PR do?

Fixes the flaky can request cookies test in network_test.js by waiting for cookies to load completely.

🔧 Implementation Notes

The async method doesn't waits for cookie to load completely, adding a driver wait fixes it.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Bug fix

Description

  • Fix flaky BiDi network test by adding wait conditions

  • Ensure cookies are loaded before assertions in test

Diagram Walkthrough

flowchart LR
  A["Add Cookie"] --> B["Navigate Refresh"] --> C["Wait for Cookies"] --> D["Assert Cookie Values"]
Loading

File Walkthrough
Relevant files
Bug fix
network_test.js
Add wait conditions for cookie loading                                     

javascript/selenium-webdriver/test/bidi/network_test.js

  • Added driver.wait() calls to ensure cookies are loaded before
    assertions
  • Wait for cookie array length to be greater than expected count
  • Prevents race conditions in async cookie loading
 +2/-0     

@selenium-ci selenium-ci added C-nodejs JavaScript Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Aug 7, 2025
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Aug 7, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

1234 - Not compliant

Non-compliant requirements:

• Fix JavaScript execution in link href on click() for Firefox
• Ensure compatibility with Selenium 2.48.x versions
• Restore functionality that worked in 2.47.1

5678 - Not compliant

Non-compliant requirements:

• Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4
• Resolve connection refused errors for subsequent ChromeDriver instances
• Ensure first ChromeDriver instance works without console errors

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

Timeout Value

The hardcoded 2000ms timeout may be insufficient for slower environments or CI systems, potentially causing intermittent test failures

 
await driver.wait(() => beforeRequestEvent.request.cookies.length > 0, 2000)
assert.equal(beforeRequestEvent.request.cookies[0].name, 'north')
assert.equal(beforeRequestEvent.request.cookies[0].value.value, 'biryani')
const url = beforeRequestEvent.request.url
assert.equal(url, await driver.getCurrentUrl())

await driver.manage().addCookie({
  name: 'south',
  value: 'dosa',
})
await driver.navigate().refresh()

await driver.wait(() => beforeRequestEvent.request.cookies.length > 1, 2000)

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Aug 7, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null safety check

Add null check for beforeRequestEvent before accessing its properties. The event
might not be captured yet when the wait condition is evaluated, causing
potential null reference errors.

javascript/selenium-webdriver/test/bidi/network_test.js [72-74]

-await driver.wait(() => beforeRequestEvent.request.cookies.length > 0, 2000)
+await driver.wait(() => beforeRequestEvent && beforeRequestEvent.request.cookies.length > 0, 2000)
 assert.equal(beforeRequestEvent.request.cookies[0].name, 'north')
 assert.equal(beforeRequestEvent.request.cookies[0].value.value, 'biryani')
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential null reference error if beforeRequestEvent is not yet defined when the wait condition is checked, and the proposed fix makes the test more robust.

Medium
  • Update

diemol
diemol approved these changes Aug 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@diemol diemol added this to the Selenium 4.35 milestone Aug 7, 2025
@diemol diemol merged commit a6b06fb into SeleniumHQ:trunk Aug 7, 2025
11 checks passed
@navin772 navin772 deleted the fix-js-bidi-network-test branch August 7, 2025 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

Assignees

No one assigned

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-nodejs JavaScript Bindings Review effort 2/5

Projects

None yet

Milestone

Selenium 4.35

Development

Successfully merging this pull request may close these issues.

3 participants

@navin772 @diemol @selenium-ci