Fix typo in determining _TestArchitecture
#5087
Conversation
Youssef1313
requested review from
MarcoRossignoli and
Evangelink
as code owners
| @@ -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> | |||
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5087 +/- ##
==========================================
+ Coverage 66.11% 66.16% +0.05%
==========================================
Files 605 605
Lines 36810 36810
==========================================
+ Hits 24337 24357 +20
+ Misses 12473 12453 -20
Flags with carried forward coverage won't be shown. Click here to find out more. |
| <!-- If everyting else fails, fallback to x64. --> | ||
| <_TestArchitecture Condition="'$(_TestArchitecture)' == '' or '$(_TestArchitecture)' == 'AnyCpu'">x64</_TestArchitecture> |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@MarcoRossignoli do you recall?
There was a problem hiding this comment.
#5091 suggests a larger change to how InvokeTestingPlatformTask work.
There was a problem hiding this comment.
Merging for now as-is and we can address this concern later if #5091 isn't going to happen soon.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we don't use it to influence which architecture will run
The current implementation does that.
It tries to resolve dotnet by the _TestArchitecture defined.
There was a problem hiding this comment.
#3703 that was introduced here, long time after the comment. So I stand corrected.
There was a problem hiding this comment.
Makes sense. Anyways, I think #5091 would be the ultimate path forward
|
/backport to rel/3.8 |
|
Started backporting to rel/3.8: https://github.com/microsoft/testfx/actions/runs/13499506395 |
@Evangelink Worth backporting? Not sure if it can affect some arm64 scenarios