Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dotnet] Address warnings for Firefox devtool depreciations #14786

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 21, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

A new warning was added with a recent obsoletion, in the FirefoxDriver.GetDevToolsSession method.

Additionally, == "firefox" comparison was buggy. GetCapability returns an object, and object == string does not do what it seems. This is fixed now, and the string comparison will use value equality instead of reference equality.

Motivation and Context

This warning can be bubbled up to users:
image

And potentially unlogged warnings

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix, Enhancement


Description

  • Marked the GetDevToolsSession method in FirefoxDriver as obsolete, providing a warning about the deprecation of CDP support for Firefox.
  • Fixed a bug in RemoteWebDriver by correcting the string comparison for the browser name capability, ensuring proper type checking using pattern matching.

Changes walkthrough 📝

Relevant files
Enhancement
FirefoxDriver.cs
Mark `GetDevToolsSession` as obsolete with deprecation warning

dotnet/src/webdriver/Firefox/FirefoxDriver.cs

  • Added an Obsolete attribute to GetDevToolsSession method.
  • Provided a warning message about the deprecation of CDP support for
    Firefox.
  • +1/-0     
    Bug fix
    RemoteWebDriver.cs
    Fix browser name comparison for Firefox capability             

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs

  • Fixed string comparison for browser name capability.
  • Used pattern matching for more reliable comparison.
  • +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Pattern Matching
    The pattern matching expression is "firefox" might not handle null values from GetCapability correctly. Consider adding null check or using string.Equals() for more robust comparison

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect string comparison that could lead to false negatives when checking browser capabilities

    The pattern matching comparison is "firefox" will fail because GetCapability likely
    returns an object that needs to be cast to string first. Use string equality
    comparison instead.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [431]

    -if (this.Capabilities.GetCapability(CapabilityType.BrowserName) is "firefox")
    +if (this.Capabilities.GetCapability(CapabilityType.BrowserName)?.ToString().Equals("firefox", StringComparison.OrdinalIgnoreCase))
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a critical issue where the pattern matching operator 'is' would fail for object comparison, potentially causing the Firefox deprecation warning to be missed. The proposed fix using ToString() and case-insensitive string comparison is more robust and prevents runtime errors.

    9

    💡 Need additional feedback ? start a PR chat

    @RenderMichael RenderMichael changed the title [dotnet] Fix warnings for firefox devtool depreciations [dotnet] Address warnings for Firefox devtool depreciations Nov 21, 2024
    @nvborisenko
    Copy link
    Member

    Thanks, @RenderMichael !

    @nvborisenko nvborisenko merged commit e2b33c6 into SeleniumHQ:trunk Nov 21, 2024
    1 check passed
    @RenderMichael RenderMichael deleted the firefox-devtools branch November 21, 2024 19:02
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants