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] Align binary location property for FirefoxOptions with other options #13901

Merged

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented May 2, 2024

User description

Description

This is continuation of https://github.com/SeleniumHQ/selenium/issues/13885.

Motivation and Context

FirefoxOptions class contains two properties with no difference. It is confusing.

Probably we can just deprecate one of them to be aligned with other optionos? (chromium, safari)

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.

Type

enhancement


Description

  • Renamed browserBinaryLocation to binaryLocation in FirefoxOptions to standardize property naming across different browser options.
  • Deprecated BrowserExecutableLocation and redirected its usage to BinaryLocation, marking the former for future removal.
  • Updated all references in the codebase from the old property name to the new one, ensuring consistency.

Changes walkthrough

Relevant files
Enhancement
FirefoxOptions.cs
Align FirefoxOptions Binary Location Property Naming         

dotnet/src/webdriver/Firefox/FirefoxOptions.cs

  • Renamed browserBinaryLocation to binaryLocation to align with other
    browser options.
  • Deprecated BrowserExecutableLocation property and redirected its
    getter and setter to BinaryLocation.
  • Updated references to the renamed property in capability settings.
  • +8/-7     
    DriverFactory.cs
    Update Driver Factory to Use New Binary Location Property

    dotnet/test/common/Environment/DriverFactory.cs

  • Updated reference from BrowserExecutableLocation to BinaryLocation in
    the driver factory setup.
  • +1/-1     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Description updated to latest commit (cd95b83)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and limited to renaming and deprecating properties in a single class. The logic remains unchanged, making the review process less complex.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The PR does not mention updating any tests to reflect the changes in property names. This might lead to failures in existing tests that rely on the old property names.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Improve thread safety by using thread-safe operations for property setters.

    To ensure thread safety and avoid potential race conditions in multi-threaded
    environments, consider using properties with backing fields that are thread-safe, such as
    using Interlocked operations or locks when setting the binaryLocation.

    dotnet/src/webdriver/Firefox/FirefoxOptions.cs [108]

     get { return this.binaryLocation; }
    -set { this.binaryLocation = value; }
    +set { Interlocked.Exchange(ref this.binaryLocation, value); }
     
    Improve type safety by checking the type before casting.

    To ensure type safety and avoid runtime errors, consider checking the type of options
    before casting it to FirefoxOptions.

    dotnet/test/common/Environment/DriverFactory.cs [120]

    -((FirefoxOptions)options).BinaryLocation = this.browserBinaryLocation;
    +if (options is FirefoxOptions firefoxOptions)
    +{
    +    firefoxOptions.BinaryLocation = this.browserBinaryLocation;
    +}
     
    Maintainability
    Ensure consistency by replacing all references to the deprecated property with the new property.

    Since the BinaryLocation property is now the main way to set the binary location, ensure
    that all references to the old BrowserExecutableLocation are replaced or refactored in the
    codebase to avoid confusion and maintain consistency.

    dotnet/src/webdriver/Firefox/FirefoxOptions.cs [114]

    -[Obsolete("Use BinaryLocation property instead of BrowserExecutableLocation. This one will be removed soon.")]
    +// Ensure all references in the codebase are updated:
    +// Replace all instances of `BrowserExecutableLocation` with `BinaryLocation`
     
    Enhance code clarity and maintainability by refactoring complex conditions into separate methods.

    To improve the clarity and maintainability of the ToCapabilities method, consider
    refactoring the condition checking and assignment into a separate method or using a more
    descriptive method name.

    dotnet/src/webdriver/Firefox/FirefoxOptions.cs [306-308]

    -if (!string.IsNullOrEmpty(this.binaryLocation))
    +AddBinaryLocationToCapabilities(firefoxOptions);
    +
    +// Elsewhere in the class
    +private void AddBinaryLocationToCapabilities(Dictionary<string, object> capabilities)
     {
    -    firefoxOptions[FirefoxBinaryCapability] = this.binaryLocation;
    +    if (!string.IsNullOrEmpty(this.binaryLocation))
    +    {
    +        capabilities[FirefoxBinaryCapability] = this.binaryLocation;
    +    }
     }
     
    Bug prevention
    Prevent potential null reference exceptions by initializing strings with default values.

    To avoid null reference exceptions and ensure the binaryLocation is always in a valid
    state, consider initializing binaryLocation with a default value or ensure it is set
    before being accessed.

    dotnet/src/webdriver/Firefox/FirefoxOptions.cs [64]

    -private string binaryLocation;
    +private string binaryLocation = string.Empty; // Default to an empty string to prevent null reference issues.
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @nvborisenko nvborisenko merged commit 3b42a89 into SeleniumHQ:trunk May 15, 2024
    10 of 11 checks passed
    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.

    None yet

    2 participants