Skip to content
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

Update InvokeTestingPlatformTask to prefer running Exe over dotnet exec dll #5094

Merged
merged 18 commits into from
Feb 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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)"
Comment on lines +366 to +370
Copy link
Member Author

Choose a reason for hiding this comment

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

@baronfel Could you please confirm if it's okay to use _NativeExecutableExtension and _IsExecutable? We want to avoid using things that may potentially break in future.

Copy link
Member

Choose a reason for hiding this comment

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

Both are set by the .NET SDK in props files and so should be available. They are _-prefixed properties, which means we don't like to encourage their direct use, but I don't think we make this data available in any other way at this time. cc @dsplaisted for heads-up that more folks are taking data dependencies on SDK-private members.

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
Loading