sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Expand osx-arm64 test coverage

Open v-wuzhai opened this issue 1 year ago • 8 comments

v-wuzhai avatar Oct 09 '24 07:10 v-wuzhai

@MiYanni Is helixTargetContainer still needed here?

v-wuzhai avatar Oct 09 '24 10:10 v-wuzhai

@MiYanni will be back on Friday. Looking at the logs, I see it ran the tests on osx.13.amd64.open so another change is needed somewhere in here to target the arm64 helix images. I can try to look tomorrow or we can wait for him or if you see where it should get updated, you can try that tonight.

marcpopMSFT avatar Oct 09 '24 23:10 marcpopMSFT

It looks like the latest change got us to run the tests on arm64 successfully as far as i can tell. From the results, the most common failure is in R2R and illink tests when downlevel targeting (net5 and net3.1) error NETSDK1095: Optimizing assemblies for performance is not supported for the selected target platform or architecture. Please verify you are using a supported runtime identifier, or set the PublishReadyToRun property to false.

A lot of these tests use EnvironmentInfo.GetCompatibleRid so I wonder if we should have some special logic there that returns an x64 rid when targeting <netN.0 and the default would be arm64.

@agocke @elinor-fung Would ya'll want to keep around the downlevel targeting tests and is my proposal to change GetCompatibleRid reasonable or would you like to just get rid of the 3.1/5.0 targeting tests entirely? Alternatively, we could just modify the tests to return when run on arm64.

marcpopMSFT avatar Oct 10 '24 16:10 marcpopMSFT

cc @dotnet/illink for whether 3.1/5.0 targeting tests are still valuable

If we do keep them, I'd suggest skipping downlevel on arm64 by switching the tests from InlineData to MemberData and only returning those downlevel versions when running on platforms that support it. I don't know that special-casing GetCompatibleRid for <netN.0 on osx-arm64 provides much interesting coverage.

elinor-fung avatar Oct 11 '24 01:10 elinor-fung

We can get rid of 3/5 tests

agocke avatar Oct 13 '24 17:10 agocke

@v-wuzhai see agocke's comment above. Can you go through the tests that are failing on 3.1 and 5.0 in this PR and simply remove that coverage? Thanks.

marcpopMSFT avatar Oct 14 '24 18:10 marcpopMSFT

It seems that the value of helixTargetQueue cannot be overridden.

[dotnet-sdk-public-ci](https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=845800) / [[ 🚧 Report infrastructure issue]](https://helix.dot.net/ki/new/build=&pr=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F44008&build-leg=Pipeline%20Definition%20Validation&repository=dotnet%2Fdnceng&template=z-build-break-infrastructure-issue-template.yml&labels=Known%20Build%20Error%2CFirst%20Responder%2CDetected%20By%20-%20Customer) [[ 📄 Report repository issue]](https://helix.dot.net/ki/new/build=&pr=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F44008&build-leg=Pipeline%20Definition%20Validation&repository=dotnet%2Fsdk&labels=Known%20Build%20Error%2Cblocking-clean-ci) ❌/eng/pipelines/templates/jobs/sdk-job-matrix.yml (Line: 113, Col: 5): 'helixTargetQueue' is already defined

So I customized a variable helixQueue as a specific queue name, and added a script step to dynamically set the CustomHelixTargetQueue variable based on the value of runtimeIdentifier, so that we can see the returned queue value in the pipeline for debugging purposes.

image

v-wuzhai avatar Oct 17 '24 07:10 v-wuzhai

Thanks for trying out my proposal and coming up with an alternative. Let's just wait until next week when @MiYanni is back as he's the real expert here and maybe he'll have a better suggestion. Your solution seems reasonable to me though.

marcpopMSFT avatar Oct 18 '24 20:10 marcpopMSFT

The Arm64 leg for macOS is appropriately appearing after my changes: image

MiYanni avatar Oct 24 '24 17:10 MiYanni

@MiYanni Thank you for your additions and optimizations. These changes look good to me.

v-wuzhai avatar Oct 25 '24 02:10 v-wuzhai