Skip to content

Conversation

@navin772
Copy link
Member

@navin772 navin772 commented Aug 7, 2025

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-merge-pro
Copy link
Contributor

qodo-merge-pro 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-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 7, 2025

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

Copy link
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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants