-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Run trimming tests as AOT tests #101229
Run trimming tests as AOT tests #101229
Conversation
Not everything is passing, so I baselined this. Some we'll probably exclude permanently, others are more concerning and we need to determine if it's test issues or product issues.
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/tests.proj
Outdated
<DisabledNativeAotTestAppProjects Include="$(MSBuildThisFileDirectory)\System.Runtime\tests\System.Runtime.Tests\TrimmingTests\System.Runtime.TrimmingTests.proj" /> | ||
<DisabledNativeAotTestAppProjects Include="$(MSBuildThisFileDirectory)\System.Text.Json\tests\System.Text.Json.Tests\TrimmingTests\System.Text.Json.TrimmingTests.proj" /> | ||
|
||
<NativeAotTestAppProjects Include="$(MSBuildThisFileDirectory)*\tests\**\*.NativeAotTests.proj;$(MSBuildThisFileDirectory)*\tests\**\*.TrimmingTests.proj" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just get rid of the NativeAotTests.proj
extension (there is only 1 currently in the repo) and instead just have TrimmingTests.proj
, which are run as both PublishTrimmed and PublishAot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't have concerns about running the NativeAotTests with trimming as well, that sounds good to me too.
Trimming is generally a subset of AOT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only NativeAotTests today:
Lines 45 to 84 in 4760cf0
public static int Main() | |
{ | |
DiagnosticSource diagnosticSource = new DiagnosticListener("TestDiagnosticListener"); | |
using (var listener = new TestEventListener()) | |
{ | |
var data = new EventData() | |
{ | |
Id = Guid.NewGuid(), | |
}; | |
Write(diagnosticSource, "Test.Start", data); | |
if (!(listener.LogDataPayload?.Count == 3 && | |
(string)listener.LogDataPayload[0] == "TestDiagnosticListener" && | |
(string)listener.LogDataPayload[1] == "Test.Start")) | |
{ | |
return -1; | |
} | |
object[] args = (object[])listener.LogDataPayload[2]; | |
if (args.Length != 2) | |
{ | |
return -2; | |
} | |
IDictionary<string, object> arg = (IDictionary<string, object>)args[0]; | |
if (!((string)arg["Key"] == "Id" && (string)arg["Value"] == data.Id.ToString())) | |
{ | |
return -3; | |
} | |
arg = (IDictionary<string, object>)args[1]; | |
if (!((string)arg["Key"] == "*Enumerate" && (string)arg["Value"] == "1,2,3")) | |
{ | |
return -4; | |
} | |
return 100; | |
} | |
} |
It should work just fine with PublishTrimmed. It was written to test the MakeGenericType deep inside of DiagnosticSourceEventSource works with NativeAOT.
I think it would be best to just have 1 "kind of test" and it runs for both PublishTrimmed and PublishAot. It will make it easier to write these, and it will add a bit of coverage to both scenarios.
@@ -142,7 +142,7 @@ | |||
|
|||
<MSBuild Projects="@(TestConsoleApps)" | |||
Targets="Publish" | |||
Properties="Configuration=$(Configuration);BuildProjectReferences=false;TargetOS=$(TargetOS);TargetArchitecture=$(TargetArchitecture)" /> | |||
Properties="Configuration=$(Configuration);BuildProjectReferences=false;TargetOS=$(TargetOS);TargetArchitecture=$(TargetArchitecture);_IsPublishing=true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests weren't building because they couldn't find AppHost. This is running the Publish target directly, which is very problematic because it bypasses logic in the SDK. We have been having a conversation with the SDK team for months. The fix is to do what real publish does and that's setting _IsPublishing. We do that in multiple places in this repo even though it's used/defined by the SDK.
This reverts commit 156a359.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for making this happen.
@@ -81,14 +81,17 @@ | |||
<_additionalPropertiesString>@(_propertiesAsItems->'<%(Identity)>%(Value)</%(Identity)>', '%0a ')</_additionalPropertiesString> | |||
</PropertyGroup> | |||
|
|||
<!-- RunNativeAotTestApps trumps TestTrimming, same as PublishAot trumps PublishTrimmed --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) this comment doesn't align with the code. The comment says one trumps the other. The code says if they are both true it is an error.
/ba-g slow mac |
Not everything is passing, so I baselined this. Some we'll probably exclude permanently, others are more concerning and we need to determine if it's test issues or product issues.
Not everything is passing, so I baselined this. Some we'll probably exclude permanently, others are more concerning and we need to determine if it's test issues or product issues.
Cc @dotnet/ilc-contrib