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

qodo-code-review bot commented Dec 1, 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
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

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

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 1, 2025

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
  • Update

@nvborisenko
Copy link
Member

Risky in terms of breaking change?.. I think so.

@RenderMichael
Copy link
Contributor Author

I did my best to keep all things the same, including keeping the culture for string comparisons. All of this works exactly as before, with one change: Cookie.GetHashCode() now includes Value instead of just Name. I consider this a bugfix, since equality requires the same Value.

@nvborisenko
Copy link
Member

nvborisenko commented Dec 11, 2025

Yes, changing implementation of GetHashCode() is a bug fix.

Type starts to implement new interface, if some another type rely on this new interface (which is not implemented by consumers)... binary breaking change?

UPD: starts to rely on this interface

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

This PR standardizes the implementation of IEquatable<T> across .NET types that override Equals(object), aligning with best practices recommended by analyzer rules like CA1066. The changes improve type safety, enable better IntelliSense support, and establish a consistent pattern for equality comparisons throughout the codebase.

Key changes:

  • Implemented IEquatable<T> interface on 10 types (WebElement, SessionId, Cookie, InitializationScript, DesiredCapabilities, and event firing wrapper classes)
  • Refactored Equals(object) methods to delegate to strongly-typed Equals(T) implementations
  • Improved hash code calculations with better null-handling using null-coalescing operators
  • Standardized XML documentation to use <see langword> tags for boolean literals

Reviewed changes

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

Show a summary per file
File Description
WebElement.cs Implements IEquatable with proper delegation pattern and null handling for wrapped elements
SessionId.cs Adds IEquatable with string comparison for session keys
ReadOnlyDesiredCapabilities.cs Improves GetHashCode readability using null-coalescing operator
DesiredCapabilities.cs Implements IEquatable and refactors equality logic for better structure
ILogger.cs Updates documentation to use tags for boolean literals
ILogContext.cs Updates documentation to use tags for boolean literals
InitializationScript.cs Adds IEquatable comparing script ID, name, and source
Cookie.cs Implements IEquatable and improves GetHashCode to include both name and value
EventFiringWebDriver.cs Adds IEquatable implementations to EventFiringWebElement and EventFiringShadowRoot wrapper classes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@RenderMichael
Copy link
Contributor Author

@nvborisenko Do you mean if someone did something like this?

public class DerivedCookie : Cookie, IEquatable<Cookie>
{
    public bool Equals(Cookie other)
    {
        return other is not null && other.Name == Name && other.Value == Value;
    }
}

If so, they would now just get a warning that Equals(Cookie) hides the base Equals(Cookie). No big deal.

If you mean something else, could you provide a code sample?

@nvborisenko
Copy link
Member

No, it is not binary breaking change. Existing code still invokes Method(object) overload.

@RenderMichael
Copy link
Contributor Author

CI is broken and it will not give any useful results to wait for it; previous runs showed green CI.

@RenderMichael RenderMichael merged commit 41f8735 into SeleniumHQ:trunk Dec 11, 2025
3 of 4 checks passed
@RenderMichael RenderMichael deleted the equatable branch December 11, 2025 20:12
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