Skip to content

[dotnet] Enhance Selenium Manager platform detection #15649

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

Merged
merged 2 commits into from
Apr 20, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Apr 20, 2025

User description

🔗 Related Issues

Fixes #15646

💥 What does this PR do?

For .NET 8 target framework we use new OperatingSystem.Is*() API, and fallback to RuntimeInformation.IsOSPlatform() in netstandard 2.0.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement, Bug fix


Description

  • Refactored platform detection for Selenium Manager in .NET

    • Uses OperatingSystem.Is*() API for .NET 8+
    • Falls back to RuntimeInformation.IsOSPlatform() for netstandard2.0
  • Introduced SupportedPlatform enum for clearer platform handling

  • Improved maintainability and clarity of platform-specific binary selection


Changes walkthrough 📝

Relevant files
Enhancement
SeleniumManager.cs
Refactor and enhance platform detection logic in SeleniumManager

dotnet/src/webdriver/SeleniumManager.cs

  • Refactored platform detection logic for .NET 8 and netstandard2.0
  • Added SupportedPlatform enum for platform abstraction
  • Used switch expression for binary path selection
  • Improved error handling for unsupported platforms
  • +37/-8   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Apr 20, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating ChromeDriver multiple times

    Requires further human verification:

    • Need to verify if the platform detection enhancement indirectly addresses the connection issues with ChromeDriver

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()

    Requires further human verification:

    • Need to verify if platform detection improvements have any impact on JavaScript execution in Firefox

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

    Platform Detection Edge Case

    The code handles Windows, Linux, macOS, and FreeBSD, but might not properly handle other Unix-like systems. Consider if additional platform detection is needed.

    else if (OperatingSystem.IsLinux() || OperatingSystem.IsFreeBSD())
    {
        platform = SupportedPlatform.Linux;
    }
    Error Handling

    The PlatformNotSupportedException provides limited guidance to users. Consider adding more helpful information about supported platforms or configuration options.

        _ => throw new PlatformNotSupportedException(
                                $"Selenium Manager doesn't support your runtime platform: {RuntimeInformation.OSDescription}"),
    };

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle null platform value

    The platform variable could be null when neither NET8_0_OR_GREATER nor
    NETSTANDARD2_0 is defined, or when running on an unsupported platform. The
    switch expression will throw a NullReferenceException when platform is null,
    instead of the intended PlatformNotSupportedException.

    dotnet/src/webdriver/SeleniumManager.cs [81-88]

     binaryFullPath = platform switch
     {
         SupportedPlatform.Windows => Path.Combine(currentDirectory, "selenium-manager", "windows", "selenium-manager.exe"),
         SupportedPlatform.Linux => Path.Combine(currentDirectory, "selenium-manager", "linux", "selenium-manager"),
         SupportedPlatform.MacOS => Path.Combine(currentDirectory, "selenium-manager", "macos", "selenium-manager"),
    -    _ => throw new PlatformNotSupportedException(
    +    null or _ => throw new PlatformNotSupportedException(
                                 $"Selenium Manager doesn't support your runtime platform: {RuntimeInformation.OSDescription}"),
     };
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion addresses a critical null reference issue that could occur when the platform variable is null. Adding the null pattern match in the switch expression ensures proper error handling with the intended PlatformNotSupportedException rather than an unexpected NullReferenceException.

    High
    • Update

    @nvborisenko
    Copy link
    Member Author

    @qodo you are wrong, docs explicitly says pattern matching handles null gracefully. Interesting, what copilot thinks of...

    @nvborisenko nvborisenko requested a review from Copilot April 20, 2025 11:08
    Copy link

    @Copilot 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 refactors the platform detection logic for Selenium Manager to use the new OperatingSystem.Is*() API for .NET 8+ while falling back to RuntimeInformation.IsOSPlatform() for netstandard2.0. The changes include introducing a SupportedPlatform enum, using a switch expression for binary path selection, and improving error handling for unsupported platforms.

    @nvborisenko nvborisenko merged commit 727fae0 into SeleniumHQ:trunk Apr 20, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the sm-platform-detection branch April 20, 2025 11:36
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: Support MAUI/MacCatalyst projects in SeleniumManager for C#
    2 participants