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

Enable the fundamentals behind libraries tests compiled via crossgen2 #75230

Closed
wants to merge 3 commits into from
Closed

Enable the fundamentals behind libraries tests compiled via crossgen2 #75230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cfca759
Enable the fundamentals behind libraries tests compiled via crossgen2
davidwrighton Sep 8, 2022
6d7a7f3
Fix issues noted by code reviewers, and CI
davidwrighton Sep 8, 2022
4d9c0de
Actually use live built crossgen2, and workaround issues around proje…
davidwrighton Sep 8, 2022
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
19 changes: 18 additions & 1 deletion docs/workflow/testing/libraries/testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,21 @@ dotnet build /t:Test /p:Outerloop=true

### Running tests on a different target framework

Each test project can potentially have multiple target frameworks. There are some tests that might be OS-specific, or might be testing an API that is available only on some target frameworks, so the `TargetFrameworks` property specifies the valid target frameworks.
Each test project can potentially have multiple target frameworks. There are some tests that might be OS-specific, or might be testing an API that is available only on some target frameworks, so the `TargetFrameworks` property specifies the valid target frameworks.

### Running tests in custom compilation modes
Copy link
Member Author

Choose a reason for hiding this comment

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

@ViktorHofer I'm interested in any particular details I missed here. We really need some documentation on this sort of thing, as the behaviors of our tests are not particularly clear to folks who don't look into these systems often.


There are several custom compilation modes for tests. These are enabled by setting a switch during the config.
Copy link
Member

Choose a reason for hiding this comment

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

What you mean by "during the config"? I assume you are talking about the test invocation? Might make sense to clarify that part.


| Mode | Description | Prerequisites
| --- | --- | --- |
| TestSingleFile | Test compilation using the single file compilation mode | libs+clr |
| TestNativeAot | Test by compiling the test using NativeAOT | libs+clr.aot |
| TestReadyToRun | Test compilation of the test/libraries into R2R binaries | libs+clr |

To run a test in specific mode, simply build the tests after building the prerequisite subsets, and specify the test mode on a command line such as:
```cmd
Copy link
Member

@ivdiazsa ivdiazsa Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

NIT: Try to keep lists and code examples surrounded by blank lines, as per the recommended Markdown guidelines, so that we keep our md files as clear of warnings as we can.

dotnet build /t:Test /p:Configuration=Release /p:TestReadyToRun=true
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
dotnet build /t:Test /p:Configuration=Release /p:TestReadyToRun=true
dotnet build /t:Test -c Release /p:TestReadyToRun=true

```

These tests do not use the standard XUnit test runner, instead they use [SingleFileTestRunner.cs](../../../../src/libraries/Common/tests/SingleFileTestRunner/SingleFileTestRunner.cs). The set of available command line options is limited to `-xml`, `-notrait`, `-class`, `-class-`, `-noclass`, `-method`, `-method-`, `-nomethod`, `-namespace`, `-namespace-`, `-nonamespace`, and `-parallel`.
1 change: 1 addition & 0 deletions eng/testing/tests.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<VSTestNoLogo>true</VSTestNoLogo>
<ILLinkDescriptorsPath>$(MSBuildThisFileDirectory)ILLinkDescriptors\</ILLinkDescriptorsPath>
<TestSingleFile Condition="'$(TestNativeAot)' == 'true'">true</TestSingleFile>
<TestSingleFile Condition="'$(TestReadyToRun)' == 'true'">true</TestSingleFile>
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking because I guess it works, but ReadyToRun is not single file even if you squint really hard and having to opt out of a bunch of things in tests.singlefile.targets is now making things quite confusing. We'll likely have to add more hacks around it in the coming months/years. It's not great that we have to set it.

</PropertyGroup>

<PropertyGroup Condition="'$(TargetsMobile)' == 'true'">
Expand Down
25 changes: 23 additions & 2 deletions eng/testing/tests.singlefile.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
<PropertyGroup>
<OutputType>Exe</OutputType>

<DefineConstants>$(DefineConstants);SINGLE_FILE_TEST_RUNNER</DefineConstants>

<BundleDir>$([MSBuild]::NormalizeDirectory('$(OutDir)', 'publish'))</BundleDir>
<RunScriptOutputPath>$([MSBuild]::NormalizePath('$(BundleDir)', '$(RunScriptOutputName)'))</RunScriptOutputPath>
<RuntimeIdentifier>$(PackageRID)</RuntimeIdentifier>
Expand All @@ -18,6 +16,11 @@
<SelfContained>true</SelfContained>
</PropertyGroup>

<PropertyGroup Condition="'$(TestReadyToRun)' == 'true'">
<PublishReadyToRun>true</PublishReadyToRun>
<PublishSingleFile>false</PublishSingleFile>
</PropertyGroup>

<PropertyGroup Condition="'$(TestNativeAot)' == 'true'">
<IlcToolsPath>$(CoreCLRILCompilerDir)</IlcToolsPath>
<IlcToolsPath Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)'">$(CoreCLRCrossILCompilerDir)</IlcToolsPath>
Expand All @@ -35,6 +38,10 @@
<SelfContained>true</SelfContained>
</PropertyGroup>

<PropertyGroup Condition="'$(PublishSingleFile)'=='true' or '$(TestNativeAot)' == 'true'">
<DefineConstants>$(DefineConstants);SINGLE_FILE_TEST_RUNNER</DefineConstants>
</PropertyGroup>

<Import Project="$(CoreCLRBuildIntegrationDir)Microsoft.DotNet.ILCompiler.SingleEntry.targets" Condition="'$(TestNativeAot)' == 'true'" />

<ItemGroup Condition="'$(TestNativeAot)' == 'true'">
Expand Down Expand Up @@ -83,6 +90,20 @@
</ItemGroup>
</Target>

<Target Name="__ReplaceCrossgen2ExecutableWithFreshlyBuiltOne" BeforeTargets="_PrepareForReadyToRunCompilation">
<PropertyGroup>
<Crossgen2ArtifactPath>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)','crossgen2'))crossgen2$(ExeSuffix)</Crossgen2ArtifactPath>
</PropertyGroup>
<ItemGroup>
<Crossgen2CurrentTool Include="@(Crossgen2Tool->'$(Crossgen2ArtifactPath)')" />
<Crossgen2Tool Remove="@(Crossgen2Tool)" />
</ItemGroup>

<ItemGroup>
<Crossgen2Tool Include="@(Crossgen2CurrentTool)" />
</ItemGroup>
</Target>

<Target Name="PublishTestAsSingleFile"
Condition="'$(IsCrossTargetingBuild)' != 'true'"
AfterTargets="Build"
Expand Down
51 changes: 51 additions & 0 deletions src/libraries/Common/tests/SingleFileTestRunner/SingleFileTestRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public static int Main(string[] args)
var asm = typeof(SingleFileTestRunner).Assembly;
Console.WriteLine("Running assembly:" + asm.FullName);

// The current RemoteExecutor implementation is not compatible with the SingleFileTestRunner
Environment.SetEnvironmentVariable("DOTNET_REMOTEEXECUTOR_SUPPORTED", "0");
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a tracking issue for that, to link to?


var diagnosticSink = new ConsoleDiagnosticMessageSink();
var testsFinished = new TaskCompletionSource();
var testSink = new TestMessageSink();
Expand Down Expand Up @@ -74,10 +77,58 @@ public static int Main(string[] args)
noTraits.Add(traitKeyValue[0], values = new List<string>());
}
values.Add(traitKeyValue[1]);
i++;
}
if (args[i].Equals("-xml", StringComparison.OrdinalIgnoreCase))
{
xmlResultFileName=args[i + 1].Trim();
i++;
}
if (args[i].Equals("-class", StringComparison.OrdinalIgnoreCase))
{
filters.IncludedClasses.Add(args[i + 1].Trim());
i++;
}
if (args[i].Equals("-noclass", StringComparison.OrdinalIgnoreCase) || args[i].Equals("-class-", StringComparison.OrdinalIgnoreCase))
{
filters.ExcludedClasses.Add(args[i + 1].Trim());
i++;
}
if (args[i].Equals("-method", StringComparison.OrdinalIgnoreCase))
{
filters.IncludedMethods.Add(args[i + 1].Trim());
i++;
}
if (args[i].Equals("-nomethod", StringComparison.OrdinalIgnoreCase) || args[i].Equals("-method-", StringComparison.OrdinalIgnoreCase))
{
filters.ExcludedMethods.Add(args[i + 1].Trim());
i++;
}
if (args[i].Equals("-namespace", StringComparison.OrdinalIgnoreCase))
{
filters.IncludedNamespaces.Add(args[i + 1].Trim());
i++;
}
if (args[i].Equals("-nonamespace", StringComparison.OrdinalIgnoreCase) || args[i].Equals("-namespace-", StringComparison.OrdinalIgnoreCase))
{
filters.ExcludedNamespaces.Add(args[i + 1].Trim());
i++;
}
if (args[i].Equals("-parallel", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for extending this! Especially the -parallel option is something that I've been doing locally pretty much forever.

I would like us to get rid of this runner eventually (#70432). The xunit runner from Arcade used to support single file testing (including remote executor) but all of that was deleted when UWP support was deleted from Arcade. I'd like to restore it at some point.

I wonder though whether crossgen2 testing needs the single file runner - could it get by with the real runner?

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 suspect it could use the normal file runner, but this looks like it will work, and I don't understand the libraries testing enough to understand how exactly to make this work with the normal runner without doing more research. With the additional arguments, I don't see a huge benefit to making this different from SingleFile or NativeAOT testing. As a side benefit, enabling R2R singlefile testing here becomes extremely easy should we want to do so.

{
string parallelismArg = args[i + 1].ToLower();
var (parallelizeAssemblies, parallelizeTestCollections) = parallelismArg switch
{
"all" => (true, true),
"assemblies" => (true, false),
"collections" => (false, true),
"none" => (false, false),
_ => throw new ArgumentException("unknown parallelism option")
};

assemblyConfig.ParallelizeAssembly = parallelizeAssemblies;
assemblyConfig.ParallelizeTestCollections = parallelizeTestCollections;
i++;
}
}

Expand Down
2 changes: 2 additions & 0 deletions .../System.Diagnostics.FileVersionInfo.Tests/System.Diagnostics.FileVersionInfo.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,7 @@
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</ProjectReference>
<ProjectReference Include="..\System.Diagnostics.FileVersionInfo.TestAssembly\System.Diagnostics.FileVersionInfo.TestAssembly.csproj" Condition="'$(TargetOS)' == 'Browser'" />
<!-- R2R testing does not tolerate the combination of a regular project reference and a content reference. -->
<PublishReadyToRunExclude Include="System.Diagnostics.FileVersionInfo.TestAssembly.dll" />
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 actually a bug in the SDK which should be fixed there.

</ItemGroup>
</Project>
2 changes: 2 additions & 0 deletions src/libraries/System.Diagnostics.Process/tests/System.Diagnostics.Process.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,7 @@
<OutputItemType>Content</OutputItemType>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</ProjectReference>
<!-- R2R testing does not tolerate the combination of a regular project reference and a content reference. -->
<PublishReadyToRunExclude Include="LongPath.dll" />
Comment on lines +71 to +72
Copy link
Member

@ViktorHofer ViktorHofer Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Curious, why does R2R testing not allow a ProjectReference that contributes to the Content item? Is there a tracking issue for that?

</ItemGroup>
</Project>