Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jun 20, 2025

User description

Finally I can do bazel test //dotnet/test/common:AlertsTest-chrome on Windows, and use IDE.

💥 What does this PR do?

  • Use Runtimes dependency managed by nuget
  • Determine test web server binary properly on Windows under bazel

🔧 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

Bug fix


Description

• Add Windows support for bazel test execution in .NET
• Fix test web server binary path resolution on Windows
• Update dependency management to use nuget Runfiles


Changes walkthrough 📝

Relevant files
Bug fix
TestWebServer.cs
Add Windows executable path resolution                                     

dotnet/test/common/Environment/TestWebServer.cs

• Add Windows-specific path resolution for appserver binary
• Use .exe
extension for Windows executable path
• Maintain cross-platform
compatibility with OS detection

+9/-1     
Dependencies
BUILD.bazel
Update Runfiles dependency management                                       

dotnet/test/common/BUILD.bazel

• Replace rules_dotnet runfiles with nuget Runfiles dependency
• Add
Runfiles framework dependency to test suite
• Update dependency
management approach

+2/-1     

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 C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations labels Jun 20, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Path Separator

    Using backslash path separator in Rlocation call may cause issues on non-Windows systems or when paths are processed by cross-platform tools. Consider using forward slashes which work universally in most contexts.

        standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver.exe");
    }
    else
    {
        standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver");
    Error Handling

    The new Windows-specific path resolution logic is not wrapped in the existing try-catch block, potentially causing unhandled exceptions if the Windows path resolution fails.

    if (OperatingSystem.IsWindows())
    {
        standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver.exe");
    }
    else
    {
        standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver");
    }

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Reduce code duplication in paths
    Suggestion Impact:The suggestion was implemented with a slightly different approach - the commit extracts the base path into a variable and modifies it conditionally, then uses it once in a single Rlocation call, effectively eliminating the code duplication

    code diff:

    +                var standaloneAppserverProbingPath = @"_main/java/test/org/openqa/selenium/environment/appserver";
    +
                     if (OperatingSystem.IsWindows())
                     {
    -                    standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver.exe");
    +                    standaloneAppserverProbingPath += ".exe";
                     }
    -                else
    -                {
    -                    standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver");
    -                }
    +
    +                standaloneAppserverPath = runfiles.Rlocation(standaloneAppserverProbingPath);

    Consider storing the base path in a variable to avoid code duplication and make
    the path easier to maintain. This reduces the risk of typos and makes future
    path changes simpler.

    dotnet/test/common/Environment/TestWebServer.cs [56-63]

    +var basePath = @"_main/java/test/org/openqa/selenium/environment/appserver";
     if (OperatingSystem.IsWindows())
     {
    -    standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver.exe");
    +    standaloneAppserverPath = runfiles.Rlocation(basePath + ".exe");
     }
     else
     {
    -    standaloneAppserverPath = runfiles.Rlocation(@"_main/java/test/org/openqa/selenium/environment/appserver");
    +    standaloneAppserverPath = runfiles.Rlocation(basePath);
     }

    [Suggestion processed]

    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies a duplicated string literal for the application path and proposes a valid refactoring. Extracting the common base path into a variable improves code readability and maintainability, which is a good practice.

    Low
    • Update

    @nvborisenko nvborisenko merged commit c331228 into SeleniumHQ:trunk Jun 20, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the build-bazel-win branch June 20, 2025 14:17
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    B-build Includes scripting, bazel and CI integrations C-dotnet .NET Bindings Review effort 2/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants