-
Notifications
You must be signed in to change notification settings - Fork 1
feat(#5): Auto-check .Net SDK installation #52
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| using System; | ||
| using System.Threading.Tasks; | ||
| using Xunit; | ||
| using BannerlordModEditor.Common.Services; | ||
|
|
||
| namespace BannerlordModEditor.Common.Tests.Services | ||
| { | ||
| public class SdkCheckerServiceTests | ||
| { | ||
| private readonly SdkCheckerService _service; | ||
|
|
||
| public SdkCheckerServiceTests() | ||
| { | ||
| _service = new SdkCheckerService(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetDownloadUrl_ReturnsValidUrl() | ||
| { | ||
| var url = _service.GetDownloadUrl(); | ||
| Assert.NotNull(url); | ||
| Assert.Contains("dotnet", url); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task CheckSdkAsync_ReturnsValidInfo() | ||
| { | ||
| var info = await _service.CheckSdkAsync(); | ||
|
|
||
| Assert.NotNull(info); | ||
| Assert.NotNull(info.RecommendedVersion); | ||
| Assert.NotNull(info.DownloadUrl); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void CheckSdk_ReturnsValidInfo() | ||
| { | ||
| var info = _service.CheckSdk(); | ||
|
|
||
| Assert.NotNull(info); | ||
| Assert.NotNull(info.RecommendedVersion); | ||
| Assert.NotNull(info.DownloadUrl); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void IsSdkInstalled_ReturnsBoolean() | ||
| { | ||
| var result = _service.IsSdkInstalled(); | ||
| Assert.IsType<bool>(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetSdkVersion_ReturnsStringOrNull() | ||
| { | ||
| var result = _service.GetSdkVersion(); | ||
| Assert.True(result == null || !string.IsNullOrEmpty(result)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetInstalledVersions_ReturnsList() | ||
| { | ||
| var versions = _service.GetInstalledVersions(); | ||
| Assert.NotNull(versions); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,202 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Collections.Generic; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Diagnostics; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.IO; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Linq; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Runtime.InteropServices; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Threading.Tasks; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace BannerlordModEditor.Common.Services | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class SdkInfo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public bool IsInstalled { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public string? Version { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public string? Path { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public List<string> InstalledVersions { get; set; } = new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public string? RecommendedVersion { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public string? DownloadUrl { get; set; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public interface ISdkCheckerService | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Task<SdkInfo> CheckSdkAsync(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SdkInfo CheckSdk(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool IsSdkInstalled(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string? GetSdkVersion(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| List<string> GetInstalledVersions(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string GetDownloadUrl(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class SdkCheckerService : ISdkCheckerService | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private const string RecommendedSdkVersion = "9.0.100"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private const string DownloadUrlBase = "https://dotnet.microsoft.com/download/dotnet/9.0"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public async Task<SdkInfo> CheckSdkAsync() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return await Task.Run(() => CheckSdk()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+38
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public async Task<SdkInfo> CheckSdkAsync() | |
| { | |
| return await Task.Run(() => CheckSdk()); | |
| public Task<SdkInfo> CheckSdkAsync() | |
| { | |
| return Task.FromResult(CheckSdk()); |
Copilot
AI
Mar 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can deadlock/hang: RedirectStandardError = true but stderr is never drained. If dotnet writes enough to stderr, the child process can block, causing ReadToEnd()/WaitForExit() to hang indefinitely. Fix by either reading stderr as well (or not redirecting it), and consider using WaitForExitAsync plus async reads to avoid blocking.
| var output = process.StandardOutput.ReadToEnd(); | |
| process.WaitForExit(); | |
| var stdoutTask = process.StandardOutput.ReadToEndAsync(); | |
| var stderrTask = process.StandardError.ReadToEndAsync(); | |
| process.WaitForExit(); | |
| Task.WaitAll(stdoutTask, stderrTask); | |
| var output = stdoutTask.Result; |
Copilot
AI
Mar 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process-start logic is duplicated across IsSdkInstalled, GetSdkVersion, GetInstalledVersions, and GetSdkPath, making it harder to consistently apply fixes (timeouts, stderr draining, disposal, etc.). Consider extracting a single helper (e.g., RunDotnet(args) returning exit code + stdout/stderr) and reuse it for all commands.
| public bool IsSdkInstalled() | |
| { | |
| try | |
| { | |
| var process = new Process | |
| { | |
| StartInfo = new ProcessStartInfo | |
| { | |
| FileName = "dotnet", | |
| Arguments = "--version", | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| UseShellExecute = false, | |
| CreateNoWindow = true | |
| } | |
| }; | |
| process.Start(); | |
| var output = process.StandardOutput.ReadToEnd(); | |
| process.WaitForExit(); | |
| return process.ExitCode == 0 && !string.IsNullOrWhiteSpace(output); | |
| private (int ExitCode, string StdOut, string StdErr) RunDotnet(string arguments) | |
| { | |
| var startInfo = new ProcessStartInfo | |
| { | |
| FileName = "dotnet", | |
| Arguments = arguments, | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, | |
| UseShellExecute = false, | |
| CreateNoWindow = true | |
| }; | |
| using var process = new Process | |
| { | |
| StartInfo = startInfo | |
| }; | |
| process.Start(); | |
| string stdout = process.StandardOutput.ReadToEnd(); | |
| string stderr = process.StandardError.ReadToEnd(); | |
| process.WaitForExit(); | |
| return (process.ExitCode, stdout, stderr); | |
| } | |
| public bool IsSdkInstalled() | |
| { | |
| try | |
| { | |
| var result = RunDotnet("--version"); | |
| return result.ExitCode == 0 && !string.IsNullOrWhiteSpace(result.StdOut); |
Copilot
AI
Mar 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All process invocations use blocking ReadToEnd() + WaitForExit() without any timeout/cancellation. In environments where dotnet is slow/hung/misconfigured, this can block the calling thread (and CheckSdkAsync will still tie up a thread-pool thread). Consider adding a bounded timeout (and/or cancellation token support) and cleaning up the process if it exceeds the limit.
Copilot
AI
Mar 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing uses Split(' ') without removing empty entries. If the --list-sdks output contains multiple spaces/tabs, parts[0] can be empty, producing empty version strings in versions. Use a split that removes empty entries (and preferably handles tabs) before taking the first token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests execute the real
dotnetCLI on the machine running the tests. That makes them environment-dependent and potentially flaky in CI (e.g., missing SDK, different PATH, localized output). To make them true unit tests, introduce an abstraction for process execution (or inject a runner) and mock outputs to deterministically test parsing/version detection/path extraction; then add tests for--list-sdksparsing andGetSdkPath()behavior.