Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 20, 2025
Merged

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

merged 2 commits into from
Jun 20, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jun 20, 2025 •
edited by qodo-merge-pro 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-merge-pro 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 Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro 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