Skip to content

Commit

Permalink
Update InvokeTestingPlatformTask to prefer running Exe over `dotnet e…
Browse files Browse the repository at this point in the history
…xec dll` (#5094)
  • Loading branch information
Youssef1313 authored Feb 24, 2025
1 parent 26f91c4 commit 149076d
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,35 @@ public InvokeTestingPlatformTask()
[Required]
public ITaskItem TargetPath { get; set; }

// -------- BEGIN the following properties shouldn't be used. See https://github.com/microsoft/testfx/issues/5091 --------

/// <summary>
/// Gets or sets the value of MSBuild property UseAppHost.
/// </summary>
public ITaskItem? UseAppHost { get; set; }

/// <summary>
/// Gets or sets the value of MSBuild property _IsExecutable.
/// </summary>
public ITaskItem? IsExecutable { get; set; }

/// <summary>
/// Gets or sets the value of MSBuild property TargetDir.
/// </summary>
public ITaskItem? TargetDir { get; set; }

/// <summary>
/// Gets or sets the value of MSBuild property AssemblyName.
/// </summary>
public ITaskItem? AssemblyName { get; set; }

/// <summary>
/// Gets or sets the value of MSBuild property _NativeExecutableExtension.
/// </summary>
public ITaskItem? NativeExecutableExtension { get; set; }

// -------- END the previous properties shouldn't be used. See https://github.com/microsoft/testfx/issues/5091 --------

/// <summary>
/// Gets or sets the target framework.
/// </summary>
Expand Down Expand Up @@ -122,6 +151,12 @@ protected override string ToolName
{
get
{
if (TryGetRunCommand() is string runCommand)
{
Log.LogMessage(MessageImportance.Low, $"Constructed target path via similar logic as to RunCommand: '{runCommand}'");
return Path.GetFileName(runCommand);
}

// If target dll ends with .dll we're in the "dotnet" context
if (TargetPath.ItemSpec.EndsWith(".dll", StringComparison.InvariantCultureIgnoreCase))
{
Expand All @@ -146,6 +181,11 @@ protected override string ToolName
/// <inheritdoc />
protected override string? GenerateFullPathToTool()
{
if (TryGetRunCommand() is string runCommand)
{
return runCommand;
}

// If it's not netcore and we're on Windows we expect the TargetPath to be the executable, otherwise we try with mono.
if (!IsNetCoreApp && RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
Expand Down Expand Up @@ -202,6 +242,28 @@ protected override string ToolName
private bool IsCurrentProcessArchitectureCompatible() =>
_currentProcessArchitecture == EnumPolyfill.Parse<Architecture>(TestArchitecture.ItemSpec, ignoreCase: true);

private string? TryGetRunCommand()
{
// This condition specifically handles this part:
// https://github.com/dotnet/sdk/blob/5846d648f2280b54a54e481f55de4d9eea0e6a0e/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1152-L1155
// The more correct logic is implementing https://github.com/microsoft/testfx/issues/5091
// What we want to do here is to avoid using 'dotnet exec' if possible, and run the executable directly instead.
// When running with dotnet exec, we are run under dotnet.exe process, which can break some scenarios (e.g, loading PRI in WinUI tests).
// It seems like WinUI would try to resolve the PRI file relative to the path of the process, so relative to, e.g, C:\Program Files\dotnet
if (IsNetCoreApp &&
bool.TryParse(IsExecutable?.ItemSpec, out bool isExecutable) && isExecutable &&
bool.TryParse(UseAppHost?.ItemSpec, out bool useAppHost) && useAppHost)
{
string runCommand = $"{TargetDir?.ItemSpec}{AssemblyName?.ItemSpec}{NativeExecutableExtension?.ItemSpec}";
if (File.Exists(runCommand))
{
return runCommand;
}
}

return null;
}

/// <inheritdoc />
protected override string GenerateCommandLineCommands()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,16 @@
This let's terminal logger know that it should show `Testing` in the output,
and not the additional internal details, such as target names.
-->
<!-- Passing UseAppHost, IsExecutable, TargetDir, AssemblyName, and _NativeExecutableExtensions is mostly workaround -->
<!-- until we implement https://github.com/microsoft/testfx/issues/5091 properly -->
<!-- For now, we construct the correct "RunCommand" via these properties for a specific scenario that's known to be broken -->
<CallTarget Targets="_TestRunStart" />
<InvokeTestingPlatformTask TargetPath="$(TargetPath)"
UseAppHost="$(UseAppHost)"
IsExecutable="$(_IsExecutable)"
TargetDir="$(TargetDir)"
AssemblyName="$(AssemblyName)"
NativeExecutableExtension="$(_NativeExecutableExtension)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
TargetFramework="$(TargetFramework)"
TestArchitecture="$(_TestArchitecture)"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public async Task RunUsingTestTargetWithNetfxMSBuild()
await commandLine.RunAsync($"\"{msbuildExe}\" {testAsset.TargetAssetPath} /t:Restore");
await commandLine.RunAsync($"\"{msbuildExe}\" {testAsset.TargetAssetPath} /t:\"Build;Test\" /bl:\"{binlogFile}\"", environmentVariables: new Dictionary<string, string?>()
{
["DOTNET_ROOT"] = string.Empty,
["DOTNET_ROOT"] = Path.Combine(RootFinder.Find(), ".dotnet"),
});
StringAssert.Contains(commandLine.StandardOutput, "Tests succeeded");
}
Expand Down Expand Up @@ -152,7 +152,9 @@ await DotnetCli.RunAsync(
Assert.IsTrue(File.Exists(outputFileLog), $"Expected file '{outputFileLog}'");
string logFileContent = File.ReadAllText(outputFileLog);
Assert.IsTrue(Regex.IsMatch(logFileContent, ".*win-x86.*"), logFileContent);
Assert.IsTrue(Regex.IsMatch(logFileContent, @"\.dotnet\\x86\\dotnet\.exe"), logFileContent);

// This is the architecture part that's written by TerminalOutputDevice when there is no banner specified.
Assert.Contains($"[win-x86 - {TargetFrameworks.NetCurrent}]", logFileContent);
}

[TestMethod]
Expand All @@ -176,20 +178,32 @@ public async Task Invoke_DotnetTest_With_Incompatible_Arch()
$"test --arch {incompatibleArchitecture} -p:TestingPlatformDotnetTestSupport=True \"{testAsset.TargetAssetPath}\"",
AcceptanceFixture.NuGetGlobalPackagesFolder.Path,
failIfReturnValueIsNotZero: false);
// The output looks like:
/*
D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\tidOn\.packages\microsoft.testing.platform.msbuild\1.5.0-ci\buildMultiTargeting\Microsoft.Testing.Platform.MSBuild.targets(320,5): error : Could not find 'dotnet.exe' host for the 'arm64' architecture. [D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\vf8vR\MSBuildTests\MSBuild Tests.csproj]
D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\tidOn\.packages\microsoft.testing.platform.msbuild\1.5.0-ci\buildMultiTargeting\Microsoft.Testing.Platform.MSBuild.targets(320,5): error : [D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\vf8vR\MSBuildTests\MSBuild Tests.csproj]
D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\tidOn\.packages\microsoft.testing.platform.msbuild\1.5.0-ci\buildMultiTargeting\Microsoft.Testing.Platform.MSBuild.targets(320,5): error : You can resolve the problem by installing the 'arm64' .NET. [D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\vf8vR\MSBuildTests\MSBuild Tests.csproj]
D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\tidOn\.packages\microsoft.testing.platform.msbuild\1.5.0-ci\buildMultiTargeting\Microsoft.Testing.Platform.MSBuild.targets(320,5): error : [D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\vf8vR\MSBuildTests\MSBuild Tests.csproj]
D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\tidOn\.packages\microsoft.testing.platform.msbuild\1.5.0-ci\buildMultiTargeting\Microsoft.Testing.Platform.MSBuild.targets(320,5): error : The specified framework can be found at: [D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\vf8vR\MSBuildTests\MSBuild Tests.csproj]
D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\tidOn\.packages\microsoft.testing.platform.msbuild\1.5.0-ci\buildMultiTargeting\Microsoft.Testing.Platform.MSBuild.targets(320,5): error : - https://aka.ms/dotnet-download [D:\a\_work\1\s\artifacts\tmp\Debug\testsuite\vf8vR\MSBuildTests\MSBuild Tests.csproj]
*/
// Assert each error line separately for simplicity.
result.AssertOutputContains($"Could not find '{(OperatingSystem.IsWindows() ? "dotnet.exe" : "dotnet")}' host for the '{incompatibleArchitecture}' architecture.");
result.AssertOutputContains($"You can resolve the problem by installing the '{incompatibleArchitecture}' .NET.");
result.AssertOutputContains("The specified framework can be found at:");
result.AssertOutputContains(" - https://aka.ms/dotnet-download");

// On Windows, we run the exe directly.
// On other OSes, we run with dotnet exec.
// This yields two different outputs, pointing to the same issue.
string executableName = OperatingSystem.IsWindows() ? "MSBuild Tests.exe" : "MSBuild Tests";

result.AssertOutputContains($"error MSB6003: The specified task executable \"{executableName}\" could not be run. System.ComponentModel.Win32Exception");
result.AssertOutputContains("An error occurred trying to start process");

if (OperatingSystem.IsWindows())
{
result.AssertOutputContains("The specified executable is not a valid application for this OS platform.");
}
else if (OperatingSystem.IsMacOS())
{
result.AssertOutputContains("Bad CPU type in executable");
}
else if (OperatingSystem.IsLinux())
{
result.AssertOutputContains("Exec format error");
}
else
{
// Unexpected OS.
throw ApplicationStateGuard.Unreachable();
}
}

[TestMethod]
Expand Down Expand Up @@ -223,7 +237,8 @@ await DotnetCli.RunAsync(
string outputFileLog = Directory.GetFiles(testAsset.TargetAssetPath, "MSBuild Tests_net9.0_x64.log", SearchOption.AllDirectories).Single();
Assert.IsTrue(File.Exists(outputFileLog), $"Expected file '{outputFileLog}'");
string logFileContent = File.ReadAllText(outputFileLog);
Assert.IsTrue(Regex.IsMatch(logFileContent, @"\.dotnet\\dotnet\.exe"), logFileContent);
// This is the architecture part that's written by TerminalOutputDevice when there is no banner specified.
Assert.Contains($"[win-x64 - {TargetFrameworks.NetCurrent}]", logFileContent);
}

private static void CommonAssert(DotnetMuxerResult compilationResult, string tfm, bool testSucceeded, string testResultFolder)
Expand Down

0 comments on commit 149076d

Please sign in to comment.