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

Enable Native AOT testing in System.Text.Json. #86975

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented May 31, 2023

Fixes #73431. Fixes #86973.

@ghost
Copy link

ghost commented May 31, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #73431.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone May 31, 2023
@eiriktsarpalis
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eiriktsarpalis
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).


[Theory]
[MemberData(nameof(GetAsyncEnumerableSources))]
public async Task WriteNestedAsyncEnumerable_DTO<TElement>(IEnumerable<TElement> source, int delayInterval, int bufferSize)
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 test was identical to the one above, so I deleted it.

@@ -1049,6 +1049,7 @@ public async Task InvalidJsonFails()
await Assert.ThrowsAsync<JsonException>(() => Serializer.DeserializeWrapper<Point_2D_Struct>("{true"));
}

#if !BUILDING_SOURCE_GENERATOR_TESTS // Anonymous types not supported in source gen
Copy link
Member Author

Choose a reason for hiding this comment

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

The test is hardcoding reflection, so I'm just disabling it for source gen.

private static void ExtensionProperty_SupportsWritingToCustomSerializerWithOptionsInternal<TDictionary, TConverter>()
where TDictionary : new()
where TConverter : JsonConverter, new()
[MemberData(nameof(GetCustomOverflowConverters))]
Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored the tests somewhat to reduce the number of generic parameters needed to drive the tests.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Looks good to me from NativeAOT perspective, but can't speak about the test changes in general. Thank you!

<Directives>
<Application>
<!-- NB Shared specializations must also be replicated in the roslyn3.11.rd.xml file -->
<Assembly Name="System.Text.Json.SourceGeneration.Roslyn4.4.Tests">
Copy link
Member

Choose a reason for hiding this comment

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

why are those needed? Do we know the reason they're trimmed out?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are helping the AOT compiler generate generic method specializations we know will be needed at runtime (but can't be inferred at compile time).

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 know why they can't be inferred?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the usual issue of trying to do Type.MakeGenericType/Method.MakeGenericMethod in AOT. We're working on a fix specifically for theories though: #86975 (comment)

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@eiriktsarpalis eiriktsarpalis merged commit 1558346 into dotnet:main Jun 2, 2023
@eiriktsarpalis eiriktsarpalis deleted the fix/stj-aot-testing branch June 2, 2023 09:59
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants