Skip to content

[wasm] Dispose Xunit ToolCommand #108319

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

Merged
merged 3 commits into from
Sep 30, 2024
Merged

[wasm] Dispose Xunit ToolCommand #108319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
039baa3
Wrap xunit's `WriteLine` methods in try catch.
ilonatommy Sep 27, 2024
f4c7943
Revert.
ilonatommy Sep 27, 2024
c127762
Make sure ToolCommand instances are disposed + do not log after the d…
ilonatommy Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/mono/wasm/Wasm.Build.Tests/Blazor/BlazorWasmTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ public void InitBlazorWasmProjectDir(string id, string targetFramework = Default
public string CreateBlazorWasmTemplateProject(string id)
{
InitBlazorWasmProjectDir(id);
new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("new blazorwasm")
.EnsureSuccessful();
using DotNetCommand dotnetCommand = new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false);
CommandResult result = dotnetCommand.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("new blazorwasm")
.EnsureSuccessful();

return Path.Combine(_projectDir!, $"{id}.csproj");
}
Expand Down Expand Up @@ -195,12 +195,12 @@ public async Task BlazorRunTest(string runArgs,
runOptions.ServerEnvironment?.ToList().ForEach(
kv => s_buildEnv.EnvVars[kv.Key] = kv.Value);

using var runCommand = new RunCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(workingDirectory);
using RunCommand runCommand = new RunCommand(s_buildEnv, _testOutput);
ToolCommand cmd = runCommand.WithWorkingDirectory(workingDirectory);

await using var runner = new BrowserRunner(_testOutput);
var page = await runner.RunAsync(
runCommand,
cmd,
runArgs,
onConsoleMessage: OnConsoleMessage,
onServerMessage: runOptions.OnServerMessage,
Expand Down
16 changes: 8 additions & 8 deletions src/mono/wasm/Wasm.Build.Tests/Blazor/CleanTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ public void Blazor_BuildThenClean_NativeRelinking(string config)
Assert.True(Directory.Exists(relinkDir), $"Could not find expected relink dir: {relinkDir}");

string logPath = Path.Combine(s_buildEnv.LogRootPath, id, $"{id}-clean.binlog");
new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("build", "-t:Clean", $"-p:Configuration={config}", $"-bl:{logPath}")
.EnsureSuccessful();
using ToolCommand cmd = new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!);
cmd.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("build", "-t:Clean", $"-p:Configuration={config}", $"-bl:{logPath}")
.EnsureSuccessful();

AssertEmptyOrNonExistentDirectory(relinkDir);
}
Expand Down Expand Up @@ -88,9 +88,9 @@ private void Blazor_BuildNativeNonNative_ThenCleanTest(string config, bool first
Assert.True(Directory.Exists(relinkDir), $"Could not find expected relink dir: {relinkDir}");

string logPath = Path.Combine(s_buildEnv.LogRootPath, id, $"{id}-clean.binlog");
new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _projectDir!)
using ToolCommand cmd = new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!);
cmd.WithEnvironmentVariable("NUGET_PACKAGES", _projectDir!)
.ExecuteWithCapturedOutput("build", "-t:Clean", $"-p:Configuration={config}", $"-bl:{logPath}")
.EnsureSuccessful();

Expand Down
12 changes: 6 additions & 6 deletions src/mono/wasm/Wasm.Build.Tests/Blazor/MiscTests2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ private CommandResult PublishForRequiresWorkloadTest(string config, string extra
extraItems: extraItems);

string publishLogPath = Path.Combine(s_buildEnv.LogRootPath, id, $"{id}.binlog");
return new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("publish",
$"-bl:{publishLogPath}",
$"-p:Configuration={config}");
using DotNetCommand cmd = new DotNetCommand(s_buildEnv, _testOutput);
return cmd.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("publish",
$"-bl:{publishLogPath}",
$"-p:Configuration={config}");
}
}
14 changes: 6 additions & 8 deletions src/mono/wasm/Wasm.Build.Tests/Blazor/MiscTests3.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,17 +99,15 @@ public void BugRegression_60479_WithRazorClassLib()
string wasmProjectDir = Path.Combine(_projectDir!, "wasm");
string wasmProjectFile = Path.Combine(wasmProjectDir, "wasm.csproj");
Directory.CreateDirectory(wasmProjectDir);
new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false)
.WithWorkingDirectory(wasmProjectDir)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("new blazorwasm")
.EnsureSuccessful();

using DotNetCommand cmd = new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false);
cmd.WithWorkingDirectory(wasmProjectDir)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("new blazorwasm")
.EnsureSuccessful();

string razorProjectDir = Path.Combine(_projectDir!, "RazorClassLibrary");
Directory.CreateDirectory(razorProjectDir);
new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false)
.WithWorkingDirectory(razorProjectDir)
cmd.WithWorkingDirectory(razorProjectDir)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("new razorclasslib")
.EnsureSuccessful();
Expand Down
8 changes: 4 additions & 4 deletions src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,10 @@ public BuildTestBase(ProjectProviderBase providerBase, ITestOutputHelper output,
if (buildProjectOptions.Publish && buildProjectOptions.BuildOnlyAfterPublish)
commandLineArgs.Append("-p:WasmBuildOnlyAfterPublish=true");

var cmd = new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.WithEnvironmentVariables(buildProjectOptions.ExtraBuildEnvironmentVariables);
using ToolCommand cmd = new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!);
cmd.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.WithEnvironmentVariables(buildProjectOptions.ExtraBuildEnvironmentVariables);
if (UseWBTOverridePackTargets && s_buildEnv.IsWorkload)
cmd.WithEnvironmentVariable("WBTOverrideRuntimePack", "true");

Expand Down
2 changes: 1 addition & 1 deletion src/mono/wasm/Wasm.Build.Tests/Common/TestOutputWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public void WriteLine(string message)
baseOutput.WriteLine(message);
_outputBuffer.AppendLine(message);
if (EnvironmentVariables.ShowBuildOutput)
Console.WriteLine(message);
Console.WriteLine(message);
}

public void WriteLine(string format, params object[] args)
Expand Down
8 changes: 6 additions & 2 deletions src/mono/wasm/Wasm.Build.Tests/Common/ToolCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace Wasm.Build.Tests
{
public class ToolCommand : IDisposable
{
private bool isDisposed = false;
private string _label;
protected ITestOutputHelper _testOutput;

Expand Down Expand Up @@ -93,12 +94,15 @@ public virtual CommandResult ExecuteWithCapturedOutput(params string[] args)

public virtual void Dispose()
{
if (isDisposed)
return;
if (CurrentProcess is not null && !CurrentProcess.HasExited)
{
CurrentProcess.Kill(entireProcessTree: true);
CurrentProcess.Dispose();
CurrentProcess = null;
}
isDisposed = true;
}

protected virtual string GetFullArgs(params string[] args) => string.Join(" ", args);
Expand All @@ -109,7 +113,7 @@ private async Task<CommandResult> ExecuteAsyncInternal(string executable, string
CurrentProcess = CreateProcess(executable, args);
DataReceivedEventHandler errorHandler = (s, e) =>
{
if (e.Data == null)
if (e.Data == null || isDisposed)
return;

string msg = $"[{_label}] {e.Data}";
Expand All @@ -120,7 +124,7 @@ private async Task<CommandResult> ExecuteAsyncInternal(string executable, string

DataReceivedEventHandler outputHandler = (s, e) =>
{
if (e.Data == null)
if (e.Data == null || isDisposed)
return;

string msg = $"[{_label}] {e.Data}";
Expand Down
22 changes: 10 additions & 12 deletions src/mono/wasm/Wasm.Build.Tests/NonWasmTemplateBuildTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,22 +108,20 @@ private void NonWasmConsoleBuild(string config,
File.WriteAllText(Path.Combine(_projectDir, "Directory.Build.props"), "<Project />");
File.WriteAllText(Path.Combine(_projectDir, "Directory.Build.targets"), directoryBuildTargets);

new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false)
.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput("new console --no-restore")
.EnsureSuccessful();
using DotNetCommand cmd = new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false);
cmd.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput("new console --no-restore")
.EnsureSuccessful();

new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false)
.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput($"build -restore -c {config} -bl:{Path.Combine(s_buildEnv.LogRootPath, $"{id}.binlog")} {extraBuildArgs} -f {targetFramework}")
.EnsureSuccessful();
cmd.WithWorkingDirectory(_projectDir!)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding working dir is void if we reuse the instance (which is by the way interesting idea!).

Does it have any side effects? The Dispose of ToolCommand disposes the CurrentProcess. Which happens only when the process didn't exit, which can't happen here I guess...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, thanks for pointing out. Follow up PR it is.

One side effect: runs will share the ITestOutputHelper instance, so when we do result.Output we will see output out of both runs. In this case it does not matter, first we only build, then only run. But in tests where we build twice but with different params and the check the output for Assert.Contains, it could be a problem.

.ExecuteWithCapturedOutput($"build -restore -c {config} -bl:{Path.Combine(s_buildEnv.LogRootPath, $"{id}.binlog")} {extraBuildArgs} -f {targetFramework}")
.EnsureSuccessful();

if (shouldRun)
{
var result = new DotNetCommand(s_buildEnv, _testOutput, useDefaultArgs: false)
.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput($"run -c {config} -f {targetFramework} --no-build")
.EnsureSuccessful();
CommandResult result = cmd.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput($"run -c {config} -f {targetFramework} --no-build")
.EnsureSuccessful();

Assert.Contains("Hello, World!", result.Output);
}
Expand Down
24 changes: 12 additions & 12 deletions src/mono/wasm/Wasm.Build.Tests/Templates/NativeBuildTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ public void BuildWithUndefinedNativeSymbol(bool allowUndefined)
File.WriteAllText(Path.Combine(_projectDir!, "Program.cs"), code);
File.Copy(Path.Combine(BuildEnvironment.TestAssetsPath, "native-libs", "undefined-symbol.c"), Path.Combine(_projectDir!, "undefined_xyz.c"));

CommandResult result = new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("build", "-c Release");
using DotNetCommand cmd = new DotNetCommand(s_buildEnv, _testOutput);
CommandResult result = cmd.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("build", "-c Release");

if (allowUndefined)
{
Expand Down Expand Up @@ -83,17 +83,17 @@ public void ProjectWithDllImportsRequiringMarshalIlGen_ArrayTypeParameter(string
Path.Combine(_projectDir!, "Program.cs"),
overwrite: true);

CommandResult result = new DotNetCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("build", $"-c {config} -bl");
using DotNetCommand cmd = new DotNetCommand(s_buildEnv, _testOutput);
CommandResult result = cmd.WithWorkingDirectory(_projectDir!)
.WithEnvironmentVariable("NUGET_PACKAGES", _nugetPackagesDir)
.ExecuteWithCapturedOutput("build", $"-c {config} -bl");

Assert.True(result.ExitCode == 0, "Expected build to succeed");

CommandResult res = new RunCommand(s_buildEnv, _testOutput)
.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput($"run --no-silent --no-build -c {config}")
.EnsureSuccessful();
using RunCommand runCmd = new RunCommand(s_buildEnv, _testOutput);
CommandResult res = runCmd.WithWorkingDirectory(_projectDir!)
.ExecuteWithCapturedOutput($"run --no-silent --no-build -c {config}")
.EnsureSuccessful();
Assert.Contains("Hello, Console!", res.Output);
Assert.Contains("Hello, World! Greetings from node version", res.Output);
}
Expand Down
Loading
Loading