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

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

Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
58471ac
New beginning for the TestReadyToRun for the libraries.
ivdiazsa Aug 28, 2023
d424419
EXPERIMENTING: Added script extensions, archive extensions, and other…
ivdiazsa Aug 28, 2023
cbae83d
EXPERIMENTING: Forgot to add some curly braces...
ivdiazsa Aug 28, 2023
bbcbda5
Recovered some variables that were being eaten and removed some other
ivdiazsa Aug 28, 2023
74c393d
EXPERIMENTING: Added OSX and Windows to see if the CI will deign run
ivdiazsa Aug 28, 2023
f39ec24
EXPERIMENT ONLY: Added some missing parameters for
ivdiazsa Aug 29, 2023
e76d7b2
Is this it?????
ivdiazsa Aug 29, 2023
800bf85
Now that we know the libraries tests can be built properly, let's add
ivdiazsa Aug 30, 2023
4888506
Discovered that when packs are involved, everything has to be in the
ivdiazsa Aug 30, 2023
f95be94
Disabled the R2R-incompatible tests when TestReadyToRun is present.
ivdiazsa Aug 31, 2023
ce0beb7
Changed 'TargetFrameworks' to use 'net8.0' instead of '$(NetCoreAppCu…
ivdiazsa Sep 5, 2023
084dcd2
CI versioning nonsense again...
ivdiazsa Sep 5, 2023
d6145d9
Merge branch 'main' into r2r-libs-take-3
ivdiazsa Sep 5, 2023
7a98a59
Still does not work. Just saving my changes.
ivdiazsa Sep 19, 2023
57a1ed0
Merge with main
ivdiazsa Sep 19, 2023
9c9845c
Merge with main
ivdiazsa Sep 27, 2023
201c744
Merge branch 'main' into r2r-libs-take-3
ivdiazsa Sep 29, 2023
0e5318e
Merge branch 'main' into r2r-libs-take-3
ivdiazsa Oct 4, 2023
94b209d
Merged with Main AND IT WORKED LOCALLY!
ivdiazsa Oct 19, 2023
baf03ff
Updated the 'extraSteps' and 'extraStepsTemplate' to the new
ivdiazsa Oct 20, 2023
bd083f9
Reenabled all the pipelines and moved the TestReadyToRun one to the
ivdiazsa Oct 20, 2023
70a5310
Removed repeated arguments from the post build steps.
ivdiazsa Oct 20, 2023
29bdc40
Reverted weird change in System.Runtime/Directory.Build.props
ivdiazsa Oct 23, 2023
0dafa7a
Removed unnecessary import in Build.proj
ivdiazsa Oct 23, 2023
0987abf
As per Jeremy's feedback, now trying to set the TestReadyToRun template
ivdiazsa Oct 23, 2023
c050621
Reenabled the problematic tests to see if they still fail. If yes, then
ivdiazsa Oct 23, 2023
21f95ef
Merge branch 'main' into r2r-libs-take-3
ivdiazsa Oct 24, 2023
59af6bb
Found a workaround for the libraries tests!
ivdiazsa Oct 30, 2023
52875a7
Forgot to specify the test group for the new pipelines.
ivdiazsa Oct 30, 2023
43eee05
Helix doesn't want to work so how about we try running them as we
ivdiazsa Oct 30, 2023
4666ac0
Disabled tests incompatible with TestReadyToRun.
ivdiazsa Oct 30, 2023
79a9028
Changed the test disabling mechanism to the already defined
ivdiazsa Oct 31, 2023
e95e20d
Moved the 'UseLocalAppHostPack' setting to eng/tests.singlefile.targets.
ivdiazsa Oct 31, 2023
e857281
Temporarily restored 'UseLocalAppHostPack' to
ivdiazsa Nov 1, 2023
d193ab5
Extended the TestReadyToRun pipelines timeout because building all
ivdiazsa Nov 2, 2023
7a3a1d5
Testing removing the .dbg files since those are responsible for most
ivdiazsa Nov 8, 2023
cd94d5a
Removed the debugging files from the "To Copy" lists, rather than
ivdiazsa Nov 10, 2023
3c54a84
Replaced Remove+Condition with Remove->WithMetadataValue for better
ivdiazsa Nov 10, 2023
879e5ea
Merge branch 'main' into r2r-libs-take-3
ivdiazsa Nov 14, 2023
11cdb6b
Tried adding to pretest setup as per Viktor's suggestion.
ivdiazsa Nov 14, 2023
19bb85f
Testing something...
ivdiazsa Nov 20, 2023
81cb9e3
Fixed the R2R-precompiled pack not built on time.
ivdiazsa Nov 21, 2023
47760b3
Moved the R2R-precompiled runtime generation from Subsets.props to pr…
ivdiazsa Nov 22, 2023
0728170
Added the ArchiveTests property to get the test artifacts packed
ivdiazsa Nov 27, 2023
4262460
Excluded dotnet-host installers in TestReadyToRun builds because we
ivdiazsa Nov 27, 2023
403a5f9
Disabled System.Runtime.Loader.Tests because they can't be run due to
ivdiazsa Nov 30, 2023
797f2c0
Disabled the other faulty System.Runtime.Loader* test, and linked to a
ivdiazsa Nov 30, 2023
048d35e
Fixed the live-ref-pack getting removed from publish files.
ivdiazsa Dec 5, 2023
9939472
Added a copy function
ivdiazsa Dec 6, 2023
033508d
Forgot to actually set the files to copy.
ivdiazsa Dec 6, 2023
54088ef
Merge branch 'main' into r2r-libs-take-3
ivdiazsa Dec 7, 2023
7a23788
Testing something...
ivdiazsa Dec 7, 2023
7c97aa7
Changed the TestReadyToRun pipeline dynamics to only enable the tests
ivdiazsa Dec 12, 2023
f54af82
Added comments to tests.singlefile.targets, and disabled the
ivdiazsa Dec 13, 2023
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: 2 additions & 0 deletions eng/pipelines/common/platform-matrix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ parameters:
jobs:

# Linux arm

trylek marked this conversation as resolved.
Show resolved Hide resolved
- ${{ if or(containsValue(parameters.platforms, 'linux_arm'), in(parameters.platformGroup, 'all', 'gcstress')) }}:
- template: xplat-setup.yml
parameters:
Expand All @@ -45,6 +46,7 @@ jobs:
${{ insert }}: ${{ parameters.jobParameters }}

# Linux armv6

- ${{ if containsValue(parameters.platforms, 'linux_armv6') }}:
- template: xplat-setup.yml
parameters:
Expand Down
17 changes: 17 additions & 0 deletions eng/pipelines/coreclr/crossgen2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,23 @@ extends:
displayNameArgs: R2R_CG2
liveLibrariesBuildConfig: Release

- template: /eng/pipelines/common/platform-matrix.yml
parameters:
jobTemplate: /eng/pipelines/common/global-build-job.yml
buildConfig: Release
platforms:
- linux_x64
- osx_x64
- windows_x64
jobParameters:
testGroup: innerloop
buildArgs: -s clr+libs+libs.tests
-c $(_BuildConfig) -test
Copy link
Member

Choose a reason for hiding this comment

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

This will run the test on the build machine. It is not how we run the tests. The tests should be sent to Helix to run instead.

It is also causing #94190. Build machines do not have all dependencies required by the tests.

/p:TestReadyToRun=true
/p: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.

RunSmokeTestsOnly will run exactly one test (System.Runtime.Tests) and nothing else. Each workload needs to define what the set of smoke tests looks like, or they only get System.Runtime.Tests. It almost doesn't seem worth to build the whole product just to run one test. Are there too many failures if RunSmokeTestsOnly is not specified? Or why do we restrict to smoke tests only? We typically run smoke tests on legs that run on all PRs by default. For on-demand legs we just run everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks for explaining this Michal. I set it to -p:RunSmokeTestsOnly=true because that's what the base templates I used to figure out how this works had. But I will remove it then in this case.

nameSuffix: TestReadyToRun_Libraries
timeoutInMinutes: 120

# Run pri0 tests with hot/cold splitting enabled (only supported on x64 at the moment)
# TODO: test on arm64 once supported
- template: /eng/pipelines/common/platform-matrix.yml
Expand Down
21 changes: 21 additions & 0 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

<!-- The test is looking for debugger attributes we would have stripped with NativeAOT -->
<IlcKeepManagedDebuggerSupport>true</IlcKeepManagedDebuggerSupport>
<UseLocalAppHostPack Condition="'$(TestReadyToRun)' == 'true'">true</UseLocalAppHostPack>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like a per-test setting - should this go to a place where we define what TestReadyToRun means in general?

</PropertyGroup>

<!--
Expand Down Expand Up @@ -332,6 +333,26 @@
<TrimmerRootDescriptor Include="$(MSBuildThisFileDirectory)ILLink.Descriptors.xml" />
</ItemGroup>

<!--
These tests are currently incompatible with TestReadyToRun.
We have a tracking issue herehttps://github.com/dotnet/runtime/issues/94189
-->
<ItemGroup Condition="'$(TestReadyToRun)' == 'true'">
<Compile Remove="System\Type\TypeTests.Get.cs" />
<Compile Remove="System\Runtime\JitInfoTests.cs" />
<Compile Remove="System\EnumTests.cs" />
</ItemGroup>

<!--
Timezone tests are not working on Linux on CI when TestReadyToRun is enabled.
It might be something missing with the machines. We have a tracking issue here:
https://github.com/dotnet/runtime/issues/94190
-->
<ItemGroup Condition="'$(TargetOS)' == 'linux' and '$(TestReadyToRun)' == 'true'">
<Compile Remove="System\TimeZoneInfoTests.cs" />
<Compile Remove="$(CommonTestPath)System\TimeProviderTests.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.

This question still remains for me. Why do we remove these tests at compile item instead of conditioning them out at run time? That's unusual.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry Viktor, it was the only way I knew of excluding tests. I will remove these item groups now that Michal showed me how we exclude libraries tests.


Copy link
Member

Choose a reason for hiding this comment

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

This is not how we do test exclusions in this repo.

We exclude failing tests with an ActiveIssue attribute. For example:

runtime/src/mono/wasm/debugger/DebuggerTestSuite/SetNextIpTests.cs

Line 116 in 848c729

[ActiveIssue("https://github.com/dotnet/runtime/issues/86496", typeof(DebuggerTests), nameof(DebuggerTests.WasmMultiThreaded))]

We need some way to detect the test was ReadyToRun compiled so that the ActiveIssue can be conditioned on that.

I would add a PlatformDetection.IsReadyToRunCompiled here: https://github.com/dotnet/runtime/blob/848c729ca44f3ae651ecd4672d40111508770450/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

As for the implementation, I don't know if we have a good API to detect this, but in the worst case, one could set an environment variable in the single file test runner and check for that:

  • Add a new #define constant somewhere around here that is set for TestReadyToRun only:

    runtime/eng/testing/tests.singlefile.targets

    Lines 66 to 68 in 848c729

    <PropertyGroup Condition="'$(PublishSingleFile)' == 'true' or '$(TestNativeAot)' == 'true'">
    <DefineConstants>$(DefineConstants);SINGLE_FILE_TEST_RUNNER</DefineConstants>
    </PropertyGroup>
  • Add a SetEnvironmentVariable call (under the above defined #ifdef) to somewhere around here:

    runtime/src/libraries/Common/tests/SingleFileTestRunner/SingleFileTestRunner.cs

    Lines 30 to 31 in 848c729

    // The current RemoteExecutor implementation is not compatible with the SingleFileTestRunner.
    Environment.SetEnvironmentVariable("DOTNET_REMOTEEXECUTOR_SUPPORTED", "0");
  • Implement PlatformDetection.IsReadyToRunCompiled by reading the environment variable.
  • Block the failing tests with [ActiveIssue("https://...", typeof(PlatformDetection), nameof(PlatformDetection.IsReadyToRunCompiled))]

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you very much for the detailed explanation Michal. I didn't know about this way of excluding tests, so I just went the route that we have in coreclr with issues.targets. I'm not sure we have a way of detecting the R2R-compilation of the tests, so I will take your suggestion of the environment variable. I will also then file a tracking issue to add this functionality.

<ItemGroup>
<Compile Include="$(CommonTestPath)System\Collections\IEnumerable.Generic.Serialization.Tests.cs" Link="Common\System\Collections\IEnumerable.Generic.Serialization.Tests.cs" />
<EmbeddedResource Include="System\Reflection\EmbeddedImage.png">
Expand Down
4 changes: 2 additions & 2 deletions src/tasks/Crossgen2Tasks/ShimFilesSimulatingLogicInSdkRepo/Microsoft.NET.CrossGen.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<!--
***********************************************************************************************
Microsoft.NET.CrossGen.targets
Microsoft.NET.CrossGen.props
ivdiazsa marked this conversation as resolved.
Show resolved Hide resolved

WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
created a backup copy. Incorrect changes to this file will make it
Expand All @@ -17,4 +17,4 @@ Copyright (c) .NET Foundation. All rights reserved.
<UsingTask TaskName="PrepareForReadyToRunCompilation" AssemblyFile="$(MSBuildThisFileDirectory)Crossgen2Tasks.dll" />
<UsingTask TaskName="ResolveReadyToRunCompilers" AssemblyFile="$(MSBuildThisFileDirectory)Crossgen2Tasks.dll" />
<UsingTask TaskName="RunReadyToRunCompiler" AssemblyFile="$(MSBuildThisFileDirectory)Crossgen2Tasks.dll" />
</Project>
</Project>
Loading