Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 1, 2025

User description

It is considered standard practice to implement IEquatable<T> when overriding bool Equals(object).

  • This gives users/IntelliSense knowledge that this type has meaningful equality.
  • There are analyzer rules for this, such as CA1066.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Implement IEquatable<T> interface across multiple types

  • Standardize Equals() method implementations with proper null handling

  • Improve hash code calculations for better equality semantics

  • Update XML documentation to use standard <see langword> tags


Diagram Walkthrough

flowchart LR
  A["Types overriding Equals"] -->|"Add IEquatable&lt;T&gt;"| B["Implement Equals&lt;T&gt;"]
  B -->|"Refactor Equals&lt;object&gt;"| C["Delegate to Equals&lt;T&gt;"]
  C -->|"Improve GetHashCode"| D["Consistent hash calculations"]
  D -->|"Update docs"| E["Use langword tags"]
Loading

File Walkthrough

Relevant files
Enhancement
10 files
EventFiringWebDriver.cs
Add IEquatable implementations to event firing elements   
+27/-18 
BrowsingContext.cs
Implement IEquatable with ordinal comparison
+7/-4     
Collector.cs
Add IEquatable with ordinal string comparison 
+7/-4     
Intercept.cs
Implement IEquatable with ordinal comparison   
+7/-4     
Cookie.cs
Implement IEquatable with improved hash code         
+24/-12 
InitializationScript.cs
Add IEquatable implementation           
+12/-2   
DesiredCapabilities.cs
Implement IEquatable with refactored logic
+23/-13 
ReadOnlyDesiredCapabilities.cs
Improve hash code calculation with null-coalescing operator
+3/-3     
SessionId.cs
Add IEquatable implementation                                 
+12/-2   
WebElement.cs
Implement IEquatable with proper delegation 
+15/-9   
Documentation
2 files
ILogContext.cs
Update documentation tags to use langword format                 
+1/-1     
ILogger.cs
Update documentation tags to use langword format                 
+1/-1     

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-support Issue or PR related to support classes labels Dec 1, 2025
@selenium-ci
Copy link
Member

Thank you, @RenderMichael for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

@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
🎫 No ticket provided
  • Create ticket/issue
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
Use remote call for element equality

Update the WebElement.Equals method to perform a remote driver call for element
equality checking, as defined by the WebDriver specification, instead of only
comparing element IDs.

dotnet/src/webdriver/WebElement.cs [670-691]

 public bool Equals(IWebElement? other)
 {
     if (other is IWrapsElement objAsWrapsElement)
     {
         other = objAsWrapsElement.WrappedElement;
     }
 
     if (other is not WebElement otherAsElement)
     {
         return false;
     }
 
     if (this.Id == otherAsElement.Id)
     {
-        // For drivers that implement ID equality, we can check for equal IDs
-        // here, and expect them to be equal. There is a potential danger here
-        // where two different elements are assigned the same ID.
         return true;
     }
 
-    return false;
+    // The WebDriver spec defines element equality as a remote call to the driver.
+    Dictionary<string, object> parameters = new Dictionary<string, object>
+    {
+        { "id", this.Id },
+        { "other", otherAsElement.Id }
+    };
+
+    Response commandResponse = this.Execute(DriverCommand.ElementEquals, parameters);
+    return (bool)commandResponse.Value;
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that the PR's implementation of Equals is a regression that deviates from the WebDriver specification, potentially causing incorrect element comparisons.

High
Improve equality check for all capabilities

Modify the Equals method in DesiredCapabilities to compare all capabilities in
the underlying dictionary, not just BrowserName, Platform, and Version.

dotnet/src/webdriver/Remote/DesiredCapabilities.cs [262-290]

 public bool Equals(DesiredCapabilities? other)
 {
     if (other is null)
     {
         return false;
     }
 
     if (ReferenceEquals(this, other))
     {
         return true;
     }
 
-    if (this.BrowserName != other.BrowserName)
+    if (this.capabilities.Count != other.capabilities.Count)
     {
         return false;
     }
 
-    if (!this.Platform.IsPlatformType(other.Platform.PlatformType))
+    foreach (var kvp in this.capabilities)
     {
-        return false;
-    }
-
-    if (this.Version != other.Version)
-    {
-        return false;
+        if (!other.capabilities.TryGetValue(kvp.Key, out var otherValue) || !Equals(kvp.Value, otherValue))
+        {
+            return false;
+        }
     }
 
     return true;
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the Equals method introduced in the PR is incomplete, leading to potential bugs where different capability sets are considered equal.

Medium
  • More

namespace OpenQA.Selenium.BiDi.BrowsingContext;

public sealed class BrowsingContext
public sealed class BrowsingContext : IEquatable<BrowsingContext>
Copy link
Member

Choose a reason for hiding this comment

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

In BiDi namespace this type wants to be a record. Historically I used class just as quick start. record's equality treats all properties. If we can resolve it somehow, it would be amazing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-support Issue or PR related to support classes C-dotnet .NET Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants