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.

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

Fix typo in determining _TestArchitecture #5087

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Fix typo in determining _TestArchitecture #5087

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ...t.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@
<!--
NETCoreSdkRuntimeIdentifier is used by DefaultAppHostRuntimeIdentifier to figure out the architecture, and is defined also for .NET Framework.
-->
<_TestArchitecture Condition="('$(_TestArchitecture)' == '' or '$(_TestArchitecture)' == 'AnyCpu') and '$(NETCoreSdkRuntimeIdentifier)' != ''">$([System.Text.RegularExpressions.Regex]::Replace($(DefaultAppHostRuntimeIdentifier), '.*-', ''))</_TestArchitecture>
<_TestArchitecture Condition="('$(_TestArchitecture)' == '' or '$(_TestArchitecture)' == 'AnyCpu') and '$(NETCoreSdkRuntimeIdentifier)' != ''">$([System.Text.RegularExpressions.Regex]::Replace($(NETCoreSdkRuntimeIdentifier), '.*-', ''))</_TestArchitecture>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a copy from line 339, but when it was copy-pasted, the condition was modified but the actual value is not. The code is meaningless on its current form.

<!-- If everyting else fails, fallback to x64. -->
<_TestArchitecture Condition="'$(_TestArchitecture)' == '' or '$(_TestArchitecture)' == 'AnyCpu'">x64</_TestArchitecture>
Comment on lines 344 to 345
Copy link
Member Author

Choose a reason for hiding this comment

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

@Evangelink This looks suspicious. I don't know how we can reach this "fallback" case. But if we do, I think we should use whatever dotnet we find from PATH and not force a specific architecture.

Copy link
Member

Choose a reason for hiding this comment

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

@MarcoRossignoli do you recall?

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

#5091 suggests a larger change to how InvokeTestingPlatformTask work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Merging for now as-is and we can address this concern later if #5091 isn't going to happen soon.

Copy link
Member

Choose a reason for hiding this comment

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

I believe it was me who wrote some of the code in test anywhere repo and lot of the comments :) see 330, the _testArchitecture is used for log naming, we don't use it to influence which architecture will run, instead we just try to guess what is the architecture of the exe we run.

So when user builds for arm64 and runs they would see _arm64 in the log names, and not _x64.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall well @nohwnd did this update, @nohwnd do you recall?

Copy link
Member

Choose a reason for hiding this comment

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

just posted above 1 min ago :)

Copy link
Member Author

Choose a reason for hiding this comment

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

we don't use it to influence which architecture will run

The current implementation does that.

testfx/src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/InvokeTestingPlatformTask.cs

Line 172 in cd21e24

if (dotnetMuxerLocator.TryGetDotnetPathByArchitecture(targetArchitecture, out string? dotnetPath))

It tries to resolve dotnet by the _TestArchitecture defined.

Copy link
Member

Choose a reason for hiding this comment

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

#3703 that was introduced here, long time after the comment. So I stand corrected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Anyways, I think #5091 would be the ultimate path forward

<TestingPlatformShowTestsFailure Condition=" '$(TestingPlatformShowTestsFailure)' == '' ">False</TestingPlatformShowTestsFailure>
Expand Down