Skip to content

[build] Support bazel test on Windows for .NET#15923

Merged
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:build-bazel-win
Jun 20, 2025
Merged

[build] Support bazel test on Windows for .NET#15923
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:build-bazel-win

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

@nvborisenko nvborisenko commented Jun 20, 2025 •
edited by qodo-code-review bot
Loading

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-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jun 20, 2025

    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-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jun 20, 2025 •
    edited
    Loading

    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

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

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

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @nvborisenko @selenium-ci