Skip to content

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Oct 15, 2025

💥 What does this PR do?

This PR resolves a bug where optional, unspecified parameters were being sent as null in the JSON payload for network modification commands (network.continueRequest, network.continueResponse, network.provideResponse).

This was causing errors, particularly with the Firefox BiDi implementation, which expects optional fields to be nil.

The fix uses the Ruby method .compact to remove any key-value pairs with a nil value from the command arguments hash before serialization.


🔧 Implementation Notes

The core of the issue was that parameters like cookies, headers, or body, when left unprovided by the user, defaulted to nil.

When constructing the command hash (e.g., in #continue_request), these nil values were present, and upon JSON serialization, they became JSON null values.

We called .compact on the hash arguments to remove nil values:

          args = {
            request: args[:id],
            body: args[:body],
            cookies: args[:cookies],
            # ...
          }.compact

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added C-rb Ruby Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Oct 15, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 15, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect test assertion order

In the RSpec tests, move the action (e.g., network.continue_request) before the
expect(...).to have_received(...) assertion to correctly verify the behavior.

rb/spec/unit/selenium/webdriver/bidi/network_spec.rb [35-41]

 it 'sends only the mandatory request ID when all optional args are nil' do
   expected_payload = {request: request_id}
 
+  network.continue_request(id: request_id)
+
   expect(mock_bidi).to have_received(:send_cmd).with('network.continueRequest', expected_payload)
-
-  network.continue_request(id: request_id)
 end
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical flaw in the test implementation where the assertion precedes the action, rendering all new tests ineffective at verifying the intended behavior.

High
  • Update

@diemol
Copy link
Member

diemol commented Oct 17, 2025

A few tests are consistently failing. Could you please have a look, @aguspe?

@titusfortner
Copy link
Member

well, I don't think that's what I wanted to do. It doesn't want me to push from command line, so I tried to edit on Github, don't think it did what I wanted it to do...

@titusfortner
Copy link
Member

I accidentally copied the integration tests onto the unit tests.
I've put them back, but between github complaining about my authentication from CLI and inability to edit a file from the browser in a PR that doesn't have that file edited.... I can't fix this one. Easy enough to do, just use the header and cookie setters with a Hash.

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-rb Ruby Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants