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

Add new Pipeline for Running Libs Tests with TestReadyToRun #91229

Merged
merged 54 commits into from
Dec 14, 2023
Merged

Add new Pipeline for Running Libs Tests with TestReadyToRun #91229

merged 54 commits into from
Dec 14, 2023

Conversation

ivdiazsa
Copy link
Member

Follows issue #85417. This is a follow up to PR #80946, which added the flag -p:TestReadyToRun for running libraries tests with a crossgen'd runtime. This PR takes this new functionality and creates a new pipeline to have a way of regularly and consistently testing the libraries using ReadyToRun, extending our R2R/Crossgen2 coverage.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 28, 2023
@ghost ghost assigned ivdiazsa Aug 28, 2023
@ivdiazsa ivdiazsa added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 28, 2023
@ghost
Copy link

ghost commented Aug 28, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Follows issue #85417. This is a follow up to PR #80946, which added the flag -p:TestReadyToRun for running libraries tests with a crossgen'd runtime. This PR takes this new functionality and creates a new pipeline to have a way of regularly and consistently testing the libraries using ReadyToRun, extending our R2R/Crossgen2 coverage.

Author: ivdiazsa
Assignees: ivdiazsa
Labels:

area-Infrastructure-libraries

Milestone: -

@ivdiazsa ivdiazsa added this to the 9.0.0 milestone Aug 28, 2023
@ivdiazsa ivdiazsa linked an issue Aug 28, 2023 that may be closed by this pull request
Build.proj Outdated Show resolved Hide resolved
eng/targetingpacks.targets Outdated Show resolved Hide resolved
Comment on lines 288 to 303
<!--
These tests are currently not R2R-Compatible, so we disable them whenever
we are using the TestReadyToRun flag.
-->
<ItemGroup Condition="'$(TestReadyToRun)' == 'true'">
<Compile Remove="System\Runtime\JitInfoTests.cs" />
<Compile Remove="System\Type\TypeTests.Get.cs" />
</ItemGroup>

<!--
This test surprisingly only fails on Linux and Windows when TestReadyToRun
is present. OSX runs just fine.
-->
<ItemGroup Condition="'$(TestReadyToRun)' == 'true' and ('$(TargetPlatformIdentifier)' == 'linux' or '$(TargetPlatformIdentifier)' == 'windows')">
<Compile Remove="System\EnumTests.cs" />
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

Do those tests fail to compile or successfully run? If the latter, do we have a way to determine at run time if we are testing crossgend bits?

@ivdiazsa
Copy link
Member Author

ivdiazsa commented Dec 5, 2023

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivdiazsa
Copy link
Member Author

ivdiazsa commented Dec 6, 2023

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -103,6 +107,79 @@
</ItemGroup>
</Target>

<Target Name="ExcludeExistingR2RBinaries"
Copy link
Member

Choose a reason for hiding this comment

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

Please describe these targets via xml comments. I don't understand what the following targets are doing or why we need them so a comment would be appreciated: ExcludeExistingR2RBinaries, AddExistingR2RBinariesReferencesForCrossgen2, CopyExistingR2RBinaries and CopyLiveRefPackIfPresent.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @ViktorHofer here - copies like this can cause extra work and even create incremental build issues when they use wildcards like this. It'd be good to understand them.

@ivdiazsa
Copy link
Member Author

ivdiazsa commented Dec 7, 2023

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

we know work, rather than trying to find all that don't work and
disable them.
@ivdiazsa
Copy link
Member Author

/azp run runtime-coreclr crossgen2

@ivdiazsa
Copy link
Member Author

/azp run runtime-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

</ItemGroup>

<ItemGroup Condition="'$(RunSmokeTestsOnly)' != 'true' and '$(RunGrpcTestsOnly)' != 'true' and '$(RunHighAOTResourceRequiringTestsOnly)' != 'true' and '$(RunLimitedSetOfTests)' != 'true'">
<ItemGroup Condition="'$(RunSmokeTestsOnly)' != 'true'
Copy link
Member

Choose a reason for hiding this comment

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

Please do not make this more complicated than it already is.

The existing ProjectExclusions ItemGroup should be exactly what you need here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok will change that. Just worth mentioning I thought you all didn't want to change that further because I originally disabled the bad tests with ProjectExclusions, and was told that was not the way to do it...

Copy link
Member

Choose a reason for hiding this comment

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

I originally disabled the bad tests with ProjectExclusions, and was told that was not the way to do it...

Can you point me to the comment asking not to use ProjectExclusion? I want to understand it.

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, it was about disabling them in System.Runtime.Tests.csproj, not about ProjectExclusions. So many loops and very similar changes since this work was started I got confused for a second there.

not-working tests with ProjectExclusions.
@ivdiazsa
Copy link
Member Author

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivdiazsa
Copy link
Member Author

ivdiazsa commented Dec 14, 2023

The failure in the Windows pipeline is about something else after the test has run (i.e. unrelated to this PR). Details about these failures are described and tracked in issue #96035. The rest is clean, so merging this now.

@ivdiazsa ivdiazsa merged commit 07b6e33 into dotnet:main Dec 14, 2023
212 of 214 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

Add TestReadyToRun Leg to CI
8 participants