Use source generator in JSON trimming tests - #133651
ApparentlyPlus wants to merge 3 commits into
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
There was a problem hiding this comment.
🟡 Changes recommended
Two critical serializer tests must flush Utf8JsonWriter before reading the stream.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR migrates JSON trimming tests to source-generated metadata for reflection-disabled and Native AOT coverage.
Changes:
- Adds generated serializer-overload and collection tests.
- Restores stack, queue, and object-converter coverage.
- Updates trimming-test wiring and assertion exit codes.
File summaries
| File | Summary | Status |
|---|---|---|
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/System.Text.Json.TrimmingTests.proj |
Registers trimming test applications and execution settings. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/SerializeAsync.ToStream.TypedObject.cs |
Tests generated async stream serialization for typed objects. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/SerializeAsync.ToStream.BoxedObject.cs |
Tests generated async stream serialization for boxed objects. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToString.TypedObject.WithWriter.cs |
Tests typed serialization through a writer; writer must be flushed before stream inspection. | Changes required: critical flush issue |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToString.TypedObject.cs |
Tests typed string serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToString.BoxedObject.WithWriter.cs |
Tests boxed serialization through a writer; writer must be flushed before stream inspection. | Changes required: critical flush issue |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToString.BoxedObject.cs |
Tests boxed string serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToByteArray.TypedObject.cs |
Tests typed byte-array serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToByteArray.BoxedObject.cs |
Tests boxed byte-array serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/DeserializeAsync.FromStream.TypedObject.cs |
Tests typed async stream deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/DeserializeAsync.FromStream.BoxedObject.cs |
Tests boxed async stream deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Deserialize.FromString.TypedObject.cs |
Tests typed string deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Deserialize.FromString.BoxedObject.cs |
Tests boxed string deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Deserialize.FromSpan.TypedObject.cs |
Tests typed span deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Deserialize.FromSpan.BoxedObject.cs |
Tests boxed span deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Deserialize.FromReader.TypedObject.cs |
Tests typed reader deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Deserialize.FromReader.BoxedObject.cs |
Tests boxed reader deserialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/ObjectConvertersTest.cs |
Tests generated object converters. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Helper.cs |
Provides shared generated collection-test helpers. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/StackOfT.cs |
Covers generated generic stack serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/Stack.cs |
Covers generated stack serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/QueueOfT.cs |
Covers generated generic queue serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/Queue.cs |
Covers generated queue serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ListOfT.cs |
Covers generated list serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ISetOfT.cs |
Covers generated set-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IReadOnlyDictionaryOfTKeyTValue.cs |
Covers generated read-only dictionary serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IListOfT.cs |
Covers generated generic list-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IList.cs |
Covers generated list-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IEnumerableOfT.cs |
Covers generated generic enumerable serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IEnumerable.cs |
Covers generated enumerable serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IDictionaryOfTKeyTValue.cs |
Covers generated generic dictionary-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/IDictionary.cs |
Covers generated dictionary-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ICollectionOfT.cs |
Covers generated generic collection-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ICollection.cs |
Covers generated collection-interface serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/Hashtable.cs |
Covers generated Hashtable serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/HashSetOfT.cs |
Covers generated hash-set serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/DictionaryOfTKeyTValue.cs |
Covers generated dictionary serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ConcurrentStack.cs |
Covers generated concurrent-stack serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ConcurrentQueue.cs |
Covers generated concurrent-queue serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/ConcurrentDictionary.cs |
Covers generated concurrent-dictionary serialization. | Reviewed |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/Collections/Array.cs |
Covers generated array serialization. | Reviewed |
Review details
Suppressed comments (2)
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToString.BoxedObject.WithWriter.cs:36
- The writer still owns the serialized POCO bytes when
stream.ToArray()is called; the serializer does not flush the suppliedUtf8JsonWriter. This makes the assertion observe an empty or incomplete stream. Flush the writer before reading the stream.
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/SerializerEntryPoint/Serialize.ToString.TypedObject.WithWriter.cs:36 - The writer still owns the serialized POCO bytes when
stream.ToArray()is called; the serializer does not flush the suppliedUtf8JsonWriter. This makes the assertion observe an empty or incomplete stream. Flush the writer before reading the stream.
- Files reviewed: 41/41 changed files
- Comments generated: 2
- Review effort level: Lite
teo-tsirpanis
left a comment
There was a problem hiding this comment.
Left some comments; they also apply to the other test files.
There was a problem hiding this comment.
🟡 Changes recommended
Restore the helper’s type-compatibility assertion and address the repeated exit-code issue.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 41/41 changed files
- Comments generated: 2
- Review effort level: Lite
|
Hi, an important thing to note about the reflection-based trimming tests is that they were introduced intentionally to support such use cases (most prominently in the case of blazor). Even though that arrangement has its problems, I wouldn't want to see coverage gone at the moment. |
|
@eiriktsarpalis the problem is that these tests can break by seemingly unrelated changes, like in #132115. If there are scenarios where reflection-based serialization is guaranteed to be supported with trimming (the only thing I can imagine is POD types with primitive fields/properties and arrays thereof, after we root the assembly in question), maybe the trimming tests can be restricted down to these. |
|
Everything is green besides the known failures. |
Repurposing test suites is generally not good practice, since it may result in unintended regressions down the line. I would prefer it if the improvements were incremental-only. |
|
I'm not sure I understand. The existing purpose of the tests is to exercise a flaky and unsupported scenario that gives trim warnings. Isn't this repurposing what #53437 is for? |
I'm sceptical because of the blazor issue. Unless @dotnet/dotnet-maui-blazor-eng can confirm that they have moved their core types to the source generator I don't think we should action this. |
|
I largely agree with @teo-tsirpanis. My case is that this adds coverage rather than removing it.
Dropping The proposed fix was the same both times too. The 2021 skip in #53235 says "These tests should be converted to use the source generator. See #53437". Teo's commit says "Disable it until it gets updated to use the source generator. It's not the first; see also Reflection is still exercised, too. Blazor coverage today is Still, if you wish @eiriktsarpalis, we could restrict the reflection tests to the subset expected to survive. |
|
I don't think we should switch the trimming tests to exclusively target the source-generated serializer at this point. Blazor still relies on reflection-based STJ serialization in trimmed apps today (at least for .NET 11), and moving the tests entirely to source generation could allow regressions in that scenario to go undetected. |
My impression has been that reflection-based serialization is not supported with trimming whatsoever. What are the actual supported scenarios? Is it "POD types with primitive fields/properties and arrays thereof, after we root the assembly in question" that I mentioned before, or are there more? Can we scope the trimming tests down to a supported subset that is not prone to breaks from unrelated changes to the BCL (like my aforementioned PR to Reflection.Emit)? |
Yes and no. It isn't guaranteed to work, but nevertheless blazor runs on the fact that certain scenaria it relies on do not break. |
|
@javiercn I had a look at where Blazor depends on this today, to work out what a test here could usefully pin. Most of it is user types the framework never sees at build time. JS interop only chains in a reflection resolver when the switch is on: if (JsonSerializer.IsReflectionEnabledByDefault)
{
JsonSerializerOptions.TypeInfoResolverChain.Add(CreateReflectionResolver());
}and The framework's own types mostly go through the source generator, between That last one is the only case I found worth keeping reflection tests for, a framework POCO with primitive and collection members serialized reflectively with the assembly rooted, and it's very close to the subset @teo-tsirpanis suggested. A Let me know if I've missed a path, otherwise I don't quite understand why this can't move forward. |
Fixes #53437.
The existing collection trimming tests now use the source generator, and I rewrote the remaining 16
SerializerEntryPointapps (one perJsonSerializeroverload) along withObjectConvertersTest. Like the collection tests, they run with reflection disabled and on the Native AOT test leg.These were removed in #53235, the same PR that deleted the
DynamicallyAccessedMembersannotations they validated (#52268), so the source generated versions verify the overloads themselves instead. The same goes forStack,Queue,Queue<T>andConcurrentStack, restored here and supported by the source generator since #53393.A nice consequence is that
Hashtableno longer needs itsbrowser-wasmskip. The generatedObjectCreatorroots the constructor statically, and that is what was getting trimmed.