[dotnet] toggle starting test server with Java if Runfiles are not available

[titusfortner]

User description

Description

This allows users to run .NET tests with bazel (both pinned and unpinned browsers), as well as locally with Java using dotnet on Mac. @nvborisenko can you verify this works on Windows? I can spin up a VM if I need to.

This might be more of a hack than the right solution, but it's the best I know how to do. Feedback appreciated.

Motivation and Context

Current code does not work on Mac to run tests locally

---

PR Type

Enhancement, Bug fix

---

Description

• Improved Runfiles handling in EnvironmentManager by adding null checks and adjusting logic for FileNotFoundException.
• Enhanced TestWebServer startup logic:
  • Added fallback to JAVA_HOME environment variable.
  • Refactored platform-specific executable path handling.
  • Improved error handling for missing Java executable.
  • Simplified process start logic and environment variable setup.

---

Changes walkthrough 📝

Relevant files

Enhancement

EnvironmentManager.cs Improve `Runfiles` handling in `EnvironmentManager`            dotnet/test/common/Environment/EnvironmentManager.cs Added null check for Runfiles creation. Adjusted logic to handle FileNotFoundException when Runfiles are not available. +4/-1      TestWebServer.cs Enhance `TestWebServer` startup logic and error handling  dotnet/test/common/Environment/TestWebServer.cs Added fallback to JAVA_HOME environment variable if javaHomeDirectory is not set. Refactored Start method to handle different platforms and executable paths. Improved error handling for missing Java executable. Simplified process start logic and environment variable setup. +46/-54

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-code-review[bot]]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across different files with modifications in logic, error handling, and environment variable management. The changes are not overly complex but require a good understanding of the existing system and the impact of these changes.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The fallback to JAVA_HOME when javaHomeDirectory is not set might not handle cases where JAVA_HOME is also not set, leading to potential runtime errors.
	Error Handling: The changes in error handling and file path adjustments need thorough testing to ensure they do not introduce new edge cases, especially in different operating system environments.
🔒 Security concerns	No

[qodo-code-review[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add a check to ensure javaHomeDirectory is not null or empty after setting it from the environment variable To ensure that javaHomeDirectory is not null or empty before using it, consider adding a check right after setting it from the environment variable. This will prevent potential issues if the environment variable is also not set. dotnet/test/common/Environment/TestWebServer.cs [29-32] if (string.IsNullOrEmpty(this.javaHomeDirectory)) { this.javaHomeDirectory = System.Environment.GetEnvironmentVariable("JAVA_HOME"); } +if (string.IsNullOrEmpty(this.javaHomeDirectory)) +{ + throw new InvalidOperationException("JAVA_HOME is not set."); +} Suggestion importance[1-10]: 7 Why: This suggestion is valid as it adds a safety check to ensure javaHomeDirectory is properly set, which is crucial for the correct execution of the program.	7
Maintainability	Extract the process start configuration into a separate method to improve readability and maintainability To improve readability and maintainability, consider extracting the process start configuration into a separate method. This will make the Start method cleaner and easier to understand. dotnet/test/common/Environment/TestWebServer.cs [68-75] -ProcessStartInfo startInfo = new ProcessStartInfo +ProcessStartInfo startInfo = CreateProcessStartInfo(); + +private ProcessStartInfo CreateProcessStartInfo() { - WorkingDirectory = projectRootPath, - UseShellExecute = !(hideCommandPrompt || captureWebServerOutput), - CreateNoWindow = hideCommandPrompt, - RedirectStandardOutput = captureWebServerOutput, - RedirectStandardError = captureWebServerOutput -}; + return new ProcessStartInfo + { + WorkingDirectory = projectRootPath, + UseShellExecute = !(hideCommandPrompt || captureWebServerOutput), + CreateNoWindow = hideCommandPrompt, + RedirectStandardOutput = captureWebServerOutput, + RedirectStandardError = captureWebServerOutput + }; +} Suggestion importance[1-10]: 7 Why: Extracting the process start configuration into a separate method is a good practice for improving the modularity and readability of the code, making it easier to manage and understand.	7
Enhancement	Initialize the runfiles variable directly within the try block to avoid unnecessary initialization Instead of initializing runfiles to null and then using the null-coalescing assignment operator, consider initializing runfiles directly within the try block. This will make the code cleaner and avoid unnecessary initialization. dotnet/test/common/Environment/EnvironmentManager.cs [27-30] -Runfiles runfiles = null; try { - runfiles = Runfiles.Create(); + Runfiles runfiles = Runfiles.Create(); Suggestion importance[1-10]: 6 Why: The suggestion correctly identifies an opportunity to streamline the initialization of runfiles by moving it directly into the try block, enhancing code clarity.	6
Best practice	Use Path.Join instead of Path.Combine for constructing the Java executable path Instead of using Path.Combine to construct the Java executable path, use Path.Join which is more efficient and avoids potential issues with trailing slashes. dotnet/test/common/Environment/TestWebServer.cs [80] -string javaExecutable = Path.Combine(javaHomeDirectory, "bin", "java"); +string javaExecutable = Path.Join(javaHomeDirectory, "bin", "java"); Suggestion importance[1-10]: 4 Why: While Path.Join can be more modern and handle separators more cleanly, the improvement in this context is minor as Path.Combine is already suitable for the given use case.	4

[nvborisenko]

It doesn't work on my side.

  Message: 
OneTimeSetUp: System.ArgumentNullException : Value cannot be null. (Parameter 'path1')

  Stack Trace: 
ArgumentNullException.Throw(String paramName)
Path.Combine(String path1, String path2, String path3)
TestWebServer.Start() line 80
AssemblyFixture.RunBeforeAnyTest() line 19

This is because this.javaHomeDirectory = System.Environment.GetEnvironmentVariable("JAVA_HOME"); - I don't have java installed on my machine, and honestly I don't want to install it.

The code in trunk works for me perfectly.

[titusfortner]

Good point, I thought it wasn't getting used if it wasn't needed, I'll recheck the flow.

My windows dev environment seems to have broken again. I'm still trying to debug locally.

[titusfortner]

After doing way too much work, apparently all that was needed was 6785daca6a92358b92635c5a221dfe9c7d937481