Skip to content

[dotnet] Lazy-load Selenium manager binary location#14639

Merged
nvborisenko merged 8 commits intoSeleniumHQ:trunkfrom
RenderMichael:lazy-manager
Oct 24, 2024
Merged

[dotnet] Lazy-load Selenium manager binary location#14639
nvborisenko merged 8 commits intoSeleniumHQ:trunkfrom
RenderMichael:lazy-manager

Conversation

@RenderMichael
Copy link
Copy Markdown
Contributor

@RenderMichael RenderMichael commented Oct 24, 2024 •
edited by qodo-code-review Bot
Loading

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

The location of the selenium manager binary is calculated when it is first accessed.

Motivation and Context

I ran into this while working on #14480

It is considered bad practice for static constructors to throw exceptions. If an exception is throws, the type is rendered unusable for the rest of the application lifecycle. The stack-trace of the exception is also much harder to follow, and the debugging experience is worsened. The fact that it throws TypeInitializationException threw me off initially, because I was testing AOT at the time.

Exceptions are expected on this flow, in exceptional cases, they are even thrown directly. With this change, the Stack trace changes:

Before:
image

After:
image

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

Enhancement

Description

  • Implemented lazy-loading for the Selenium Manager binary path to improve performance and avoid exceptions during static initialization.
  • Replaced the static constructor with a method GetBinaryFullPath to determine the binary path dynamically.
  • Enhanced error handling by providing more informative exception messages when the binary is not found or the platform is unsupported.

Changes walkthrough 📝

Relevant files
Enhancement
SeleniumManager.cs
Implement lazy-loading for Selenium Manager binary path   

dotnet/src/webdriver/SeleniumManager.cs

  • Introduced lazy-loading for the Selenium Manager binary path.
  • Replaced static constructor with a method to determine binary path.
  • Improved error handling for unsupported platforms.
  • Enhanced exception messages for missing binaries.
    		•  +11/-9   

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

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 24, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    14480 - Partially compliant

    Fully compliant requirements:

    • Partially addresses Selenium Manager compatibility

    Not compliant requirements:

    • Does not address W3C WebDriver, CDP, or W3C BiDi compatibility
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Thread Safety
    The lazy initialization of _binaryFullPath is not thread-safe. Consider using Lazy<T> or double-checked locking to ensure thread safety.

    Error Handling
    The error message when the binary is not found could be more informative. Consider adding suggestions for resolving the issue.

    @RenderMichael RenderMichael mentioned this pull request Oct 24, 2024
    Open
    7 tasks
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 24, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use a switch expression for platform-specific path assignments to improve code readability and maintainability

    Consider using a switch expression instead of multiple if-else statements for
    platform-specific path assignments. This can make the code more concise and easier
    to maintain.

    dotnet/src/webdriver/SeleniumManager.cs [50-66]

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

    Why: This suggestion improves code readability and maintainability by replacing multiple if-else statements with a switch expression, which is more concise and easier to understand.

    		8
    Best practice
    Use a constant for frequently used string values to improve maintainability and reduce errors

    Consider using a constant or readonly field for the "selenium-manager" folder name
    to improve maintainability and reduce the risk of typos when used in multiple
    places.

    dotnet/src/webdriver/SeleniumManager.cs [52]

    -binaryFullPath = Path.Combine(currentDirectory, "selenium-manager", "windows", "selenium-manager.exe");
+private const string SeleniumManagerFolder = "selenium-manager";
+...
+binaryFullPath = Path.Combine(currentDirectory, SeleniumManagerFolder, "windows", "selenium-manager.exe");
    Suggestion importance[1-10]: 7

    Why: Introducing a constant for the "selenium-manager" folder name enhances maintainability by reducing the risk of typos and making future changes easier, especially when the string is used in multiple places.

    		7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 requested a review from nvborisenko October 24, 2024 04:29
    @VietND96 VietND96 added the C-dotnet .NET Bindings label Oct 24, 2024
    nvborisenko
    nvborisenko requested changes Oct 24, 2024
    View reviewed changes
    Copy link
    Copy Markdown
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

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

    Thank you! It can be even simplified to avoid cunning logic, please see inner comment.

    Comment thread dotnet/src/webdriver/SeleniumManager.cs Outdated
    @RenderMichael
    Copy link
    Copy Markdown
    Contributor Author

    RenderMichael commented Oct 24, 2024 •
    edited
    Loading

    Thanks for the feedback, the PR has been updated

    nvborisenko
    nvborisenko reviewed Oct 24, 2024
    View reviewed changes
    Comment thread dotnet/src/webdriver/SeleniumManager.cs
    nvborisenko
    nvborisenko approved these changes Oct 24, 2024
    View reviewed changes
    Copy link
    Copy Markdown
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

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

    Thank you!

    @nvborisenko nvborisenko merged commit c3e3bc8 into SeleniumHQ:trunk Oct 24, 2024
    @RenderMichael RenderMichael deleted the lazy-manager branch October 24, 2024 18:16
    @RenderMichael
    Copy link
    Copy Markdown
    Contributor Author

    RenderMichael commented Oct 24, 2024

    Thank you @nvborisenko !

    I dug into the record syntax that worked, and you were right, it was working. There may be some special support for records. However, the moment I added [JsonRequired], I got an exception:
    image
    image

    I'm not familiar with the STJ esoterica here, but it looks like what we have now works. Maybe once v9 comes out, the internal serialization options could enable the feature flags I linked and prevent null reference exceptions even further.

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

    Reviewers

    @nvborisenko nvborisenko nvborisenko approved these changes

    Assignees

    No one assigned

    Labels

    C-dotnet .NET Bindings P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @RenderMichael @nvborisenko @VietND96