Skip to content

[rb] add missing unit tests#17025

Merged
titusfortner merged 1 commit intotrunkfrom
rb_missing_unit
Jan 31, 2026
Merged

[rb] add missing unit tests#17025
titusfortner merged 1 commit intotrunkfrom
rb_missing_unit

Conversation

@titusfortner
Copy link
Member

A couple unit test directories didn't have BUILD files, so the tests weren't running
Found bugs

🔗 Related Issues

💥 What does this PR do?

  • Add BUILD files so unit tests run
  • Fix code
  • Fix unit tests

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

@qodo-code-review
Copy link
Contributor

PR Type

Bug fix, Tests


Description

  • Fix bug in Cookies#as_json method that incorrectly mutated self

  • Simplify and refactor unit tests for better clarity

  • Add missing BUILD file to enable test execution


File Walkthrough

Relevant files
Bug fix
cookies.rb
Fix Cookies#as_json mutation bug                                                 

rb/lib/selenium/webdriver/bidi/network/cookies.rb

  • Fixed as_json method to return proper hash objects instead of mutating
    self
  • Changed from modifying instance state to creating new hash objects per
    cookie
  • Improved code structure with proper nested hash formatting
+7/-4     
Tests
cookies_spec.rb
Simplify and refactor cookies unit tests                                 

rb/spec/unit/selenium/webdriver/bidi/cookies_spec.rb

  • Consolidated three separate test cases into one focused test
  • Simplified test setup using let block for cookie instance
  • Removed overly complex test scenarios with multiple cookie attributes
  • Added clearer assertions using find method for cookie lookup
+11/-64 
Configuration changes
BUILD.bazel
Add BUILD file for BiDi unit tests                                             

rb/spec/unit/selenium/webdriver/bidi/BUILD.bazel

  • Added missing BUILD file for BiDi unit tests
  • Configured Bazel to run all *_spec.rb files in directory
  • Set up proper dependencies on bidi library
+10/-0   

@selenium-ci selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Jan 30, 2026
@qodo-code-review
Copy link
Contributor

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
🟡
🎫 #1234
🟡
🎫 #5678
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle single cookie with attributes

Restore support for serializing a single cookie with multiple attributes by
checking for a :name key, in addition to handling multiple simple key-value
cookies.

rb/lib/selenium/webdriver/bidi/network/cookies.rb [27-39]

 class Cookies < Hash
   def as_json
+    # A single cookie is a Hash of attributes, multiple cookies is a Hash of name-value pairs
+    if key?(:name)
+      cookie = dup
+      cookie[:value] = {type: 'string', value: cookie[:value].to_s}
+      return [cookie]
+    end
+
     map do |name, val|
       {
         name: name.to_s,
         value: {
           type: 'string',
           value: val.to_s
         }
       }
     end
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The PR simplifies the code but removes support for serializing cookies with multiple attributes, a feature that was present before (as seen in the deleted tests). This suggestion correctly identifies this functional regression and provides a robust solution to support both simple key-value cookies and complex cookie objects.

Medium
  • More

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Enables Ruby BiDi unit tests to run under Bazel and fixes an issue in BiDi cookie serialization uncovered by the newly-enabled tests.

Changes:

  • Add a Bazel BUILD.bazel to register/run all *_spec.rb unit tests in rb/spec/unit/selenium/webdriver/bidi.
  • Fix Selenium::WebDriver::BiDi::Cookies#as_json to return the correct array-of-hashes structure (consistent with Headers#as_json).
  • Update the cookies unit spec to assert the corrected serialization output.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
rb/spec/unit/selenium/webdriver/bidi/cookies_spec.rb Updates unit test expectations to match the corrected cookie JSON-ready structure.
rb/spec/unit/selenium/webdriver/bidi/BUILD.bazel Adds Bazel targets so BiDi unit specs in this directory are executed.
rb/lib/selenium/webdriver/bidi/network/cookies.rb Fixes as_json to return a proper cookie entry per hash pair without mutating the underlying hash.

@titusfortner titusfortner merged commit 0e743fc into trunk Jan 31, 2026
65 checks passed
@titusfortner titusfortner deleted the rb_missing_unit branch January 31, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations 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.

3 participants