Use source generator in JSON trimming tests - #133651
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. |
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.