Consolidate STJ polymorphism tests into a shared abstract suite

[eiriktsarpalis]

Test-only refactoring to expand test coverage ahead of #129041 being implemented.

Moves PolymorphicTests.CustomTypeHierarchies.cs and PolymorphicTests.TypeClassifier.cs from System.Text.Json.Tests/Serialization/ into tests/Common/ so they run under both reflection (11 wrappers) and source-gen (4 wrappers) via the SerializerWrapper pattern, removes the duplicate PolymorphismTests.cs in the source-gen test project, and wires the consolidated suite through a new context file under System.Text.Json.SourceGeneration.Tests/Serialization/PolymorphicTests.cs.

The test-only nature should make this straightforward to review.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR refactors System.Text.Json polymorphism tests into a shared tests/Common abstract suite so the same scenarios can be exercised across both the reflection-based test wrappers and the source-generated wrappers.

Changes:

• Consolidates polymorphism test implementations into src/libraries/System.Text.Json/tests/Common/PolymorphicTests*.cs and links them into both test projects.
• Replaces the reflection project’s Serialization/PolymorphicTests.cs with wrapper-only derived test classes.
• Removes the source-gen project’s duplicate PolymorphismTests.cs and wires the shared suite via a new Serialization/PolymorphicTests.cs with source-gen contexts.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj	Links the consolidated Common/PolymorphicTests*.cs into the reflection test project and removes project-local duplicates.
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PolymorphicTests.cs	Leaves only wrapper-specific derived classes that run the shared abstract suite.
src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/System.Text.Json.SourceGeneration.Tests.targets	Links the consolidated Common/PolymorphicTests*.cs into the source-gen test project and swaps in the new Serialization/PolymorphicTests.cs.
src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PolymorphicTests.cs	Adds source-gen wrappers and contexts to execute the shared polymorphism suite under metadata/default source-gen modes.
src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PolymorphismTests.cs	Removes now-redundant source-gen-only polymorphism tests.
src/libraries/System.Text.Json/tests/Common/SerializerTests.cs	Makes GenericPoco<T>internal to support broader reuse from the consolidated suites.
src/libraries/System.Text.Json/tests/Common/PolymorphicTests.cs	New shared base suite (previously reflection-only) that now runs under both wrapper families.
src/libraries/System.Text.Json/tests/Common/PolymorphicTests.CustomTypeHierarchies.cs	Adjusts model accessibility and adds reflection-only guards for scenarios not supported/representable in source-gen.
src/libraries/System.Text.Json/tests/Common/PolymorphicTests.TypeClassifier.cs	Expands shared classifier/polymorphism-option parity coverage and uses wrapper helpers for option creation/modification.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Text.Json/tests/Common/SerializerTests.cs:206

• In the JsonNode branch, JsonTypeInfo/JsonValue are created before the expected-exception check. If GetTypeInfo(...) or JsonValue.Create(...) throws (which is likely for scenarios where expectedExceptionType != null), the exception escapes the test helper and won't be validated by Assert.ThrowsAsync, causing spurious test failures.

Move the typeInfo/jsonObject construction inside the ThrowsAsync lambda for the expected-exception case (and keep the current fast path for the success case).

 if (contexts.HasFlag(SerializedValueContext.JsonNode))
{
const string key = "key";
JsonTypeInfo<TValue> typeInfo = Serializer.GetTypeInfo<TValue>(options);
var jsonObject = new JsonObject { [key] = JsonValue.Create(value, typeInfo) };
if (expectedExceptionType != null)
{
await Assert.ThrowsAsync(expectedExceptionType, () => Serializer.SerializeWrapper(jsonObject, options));
}

[github-actions]

Copilot Code Review

Holistic Assessment

Motivation: Justified — consolidating polymorphism tests into the shared Common/ suite ensures both reflection (11 wrappers) and source-gen (4 wrappers) paths run the same test logic. This directly enables upcoming #129041 work.

Approach: Sound. Follows the established SerializerWrapper + shared abstract base pattern used by other STJ test suites (e.g., UnionTests, CollectionTests). The CustomPolymorphismResolver refactoring from inheritance to composition is a correct adaptation for source-gen compatibility.

Summary: ✅ LGTM. Clean, mechanical test-only refactoring with no production code changes. The 4 tests from the deleted PolymorphismTests.cs are properly absorbed using new shared model types. Source-gen skip guards use [ConditionalFact] + SkipTestException correctly. Build file wiring is consistent. All previous reviewer feedback has been addressed.

---

Detailed Findings

Detailed Findings

✅ Test absorption — All deleted tests properly migrated

All 4 tests from the old PolymorphismTests.cs (PolymorphismOptions_AreGenerated, CollectionPolymorphismOptions_AreGenerated, PolymorphicTypeClassifier_IsGeneratedAndVisibleToModifier, OpenGenericDerivedType_PartiallyConcrete_RoundTrips) are present in PolymorphicTests.TypeClassifier.cs, adapted from static methods to instance methods using the shared Serializer wrapper. The old SourceGen-prefixed model types (SourceGenPolymorphicBase, SourceGenClassifiedAnimal, etc.) are replaced by new shared models (PolymorphicBaseWithCustomDiscriminator, FactoryClassifiedAnimal) with identical attribute configurations.

✅ CustomPolymorphismResolver refactoring — Correct composition pattern

Changing from DefaultJsonTypeInfoResolver inheritance to IJsonTypeInfoResolver with a composed _inner field is the right fix for source-gen compatibility. The GetTypeInfo return type correctly becomes JsonTypeInfo? (nullable), and the null-check (jsonTypeInfo is not null &&) is added before the base-type comparison.

✅ SerializerTests.cs changes — Correct API adaptation

• JsonValue.Create<TValue>(value) → JsonValue.Create(value, typeInfo) — necessary because the generic overload relies on reflection; the explicit typeInfo variant works correctly under both reflection and source-gen contexts.
• GenericPoco<T> changed from private to internal — required since the source-gen context needs to reference it in [JsonSerializable] attributes from the other test assembly.

✅ Source-gen skip guards — Properly implemented

All 11 IsSourceGeneratedSerializer guards (10 in CustomTypeHierarchies.cs + 1 AnonymousType in PolymorphicTests.cs) use [ConditionalFact] + throw new SkipTestException(...) with descriptive messages explaining why the source-gen path cannot reach the same runtime error path. This ensures xunit reports them as Skipped.

✅ Helper method refactoring — Consistent use of Serializer wrapper

CreateOptionsWithStructuralClassifier<TBase>(), CreateOptionsWithClassifier<TBase>(), and the ReferenceHandler.Preserve test all refactored from direct new JsonSerializerOptions { TypeInfoResolver = new DefaultJsonTypeInfoResolver { Modifiers = ... } } to Serializer.GetDefaultOptionsWithMetadataModifier(...). This ensures the options inherit the correct resolver (reflection or source-gen) from the wrapper.

✅ Context registrations — Comprehensive

The [JsonSerializable] registrations in the new source-gen PolymorphicTests.cs cover all test model types including the new shared types (PolymorphicBaseWithCustomDiscriminator, FactoryClassifiedAnimal, TestNode, PolymorphicIntList, etc.). Both Metadata and Default mode contexts are provided.

Note

This review was generated by GitHub Copilot.

Generated by Code Review for issue #129755 · ◷

Generated by Code Review for issue #129755 · ● 57.1M · ◷

[hez2010]

I think this change might break nativeaot-outerloop: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1480580&view=ms.vss-test-web.build-test-results-tab

cc: @MichalStrehovsky

[eiriktsarpalis]

I'm actually surprised to see System.Text.Json.SourceGeneration tests being run using Native AOT. At least in the past some of them were using unsupported reflection (e.g. because the source generator outputs were being compared against the reflection baseline).

[MichalStrehovsky]

ObjectConverterFactory looks to be marked RequiresDynamicCode.

It would be nice if the source generated tests could stick to source generated code only.

We have means to enforce this statically, but I had to disable it for the source generator tests because they still have tons of trim/AOT safety warnings:

runtime/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/System.Text.Json.SourceGeneration.Roslyn4.4.Tests.csproj

Lines 3 to 8 in 818cfd5

	<PropertyGroup>
	<TestedRoslynVersion>4.4</TestedRoslynVersion>
	<!-- https://github.com/dotnet/runtime/issues/126862 -->
	<EnableTrimAnalyzer>false</EnableTrimAnalyzer>
	<EnableAotAnalyzer>false</EnableAotAnalyzer>
	</PropertyGroup>

 at System.Reflection.Runtime.TypeInfos.RuntimeTypeInfo.get_TypeHandle() in /_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.cs:line 540
at Internal.Reflection.Core.Execution.ExecutionEnvironment.GetMethodInvoker(RuntimeTypeInfo, QMethodDefinition, RuntimeTypeInfo[], MemberInfo, Exception&) in /_/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Reflection/Core/Execution/ExecutionEnvironment.cs:line 105
at System.Reflection.Runtime.MethodInfos.NativeFormat.NativeFormatMethodCommon.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo, Exception&) in /_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/MethodInfos/NativeFormat/NativeFormatMethodCommon.cs:line 30
at System.Reflection.Runtime.MethodInfos.RuntimePlainConstructorInfo`1.get_UncachedMethodInvoker() in /_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/MethodInfos/RuntimePlainConstructorInfo.cs:line 170
at System.ActivatorImplementation.CreateInstance(Type, BindingFlags, Binder, Object[], CultureInfo, Object[]) in /_/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ActivatorImplementation.cs:line 140
at System.Text.Json.Serialization.Converters.ObjectConverterFactory.CreateConverter(Type, JsonSerializerOptions) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverterFactory.cs:line 126
at System.Text.Json.Serialization.JsonConverterFactory.GetConverterInternal(Type, JsonSerializerOptions) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs:line 39
at System.Text.Json.JsonSerializerOptions.ExpandConverterFactory(JsonConverter, Type) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs:line 122
at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.GetConverterForType(Type, JsonSerializerOptions, Boolean) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Converters.cs:line 169
at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.GetTypeInfo(Type, JsonSerializerOptions) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs:line 59
at System.Text.Json.Serialization.Tests.PolymorphicTests.CustomPolymorphismResolver.GetTypeInfo(Type type, JsonSerializerOptions options) in /_/src/libraries/System.Text.Json/tests/Common/PolymorphicTests.CustomTypeHierarchies.cs:line 3866
at System.Text.Json.JsonSerializerOptions.GetTypeInfoNoCaching(Type) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs:line 1030

[eiriktsarpalis]

It would be nice if the source generated tests could stick to source generated code only.

See my earlier comment

At least in the past some of them were using unsupported reflection (e.g. because the source generator outputs were being compared against the reflection baseline).

I think that's generally a desirable trait for source generator tests. If we want to run tests using AOT we should consider using a representative subset using smoke testing.

[MichalStrehovsky]

It would be nice if the source generated tests could stick to source generated code only.

See my earlier comment

#86975

These tests regularly find some obscure stress issue. Just last week: #129010

We have an active issue too:

runtime/src/libraries/tests.proj

Lines 385 to 387 in 818cfd5

	<!-- https://github.com/dotnet/runtime/issues/119380 -->
	<ProjectExclusionsInclude="$(MSBuildThisFileDirectory)System.Text.Json\tests\System.Text.Json.SourceGeneration.Tests\System.Text.Json.SourceGeneration.Roslyn4.4.Tests.csproj"
	Condition="'$(TargetOS)' == 'osx' and '$(TargetArchitecture)' == 'arm64'" />

The tests are so huge they're running into Apple linker bugs.

[MichalStrehovsky]

(The stress issues are also representative of real customer code. I've seen not one, not two,... more customer apps where System.Text.Json serialization code is more than half of their app.)

[eiriktsarpalis]

I don't think we can ever avoid the source gen tests having a huge footprint. They generally need to able to test every conceivable type shape under the sun, so thousands upon thousands of POCOs. We can have a conversation about customer apps being large, but I think that's largely a distinct issue.

[eiriktsarpalis]

If you want this test suite to be a source of stress testing for the AOT toolchain, that's fine, but it's clear there exist conflicting goals here. Perhaps having a stress suite that generates synthetic types might be a good alternative?

[MichalStrehovsky]

If you want this test suite to be a source of stress testing for the AOT toolchain, that's fine, but it's clear there exist conflicting goals here. Perhaps having a stress suite that generates synthetic types might be a good alternative?

The original motivation was things like #73431 (comment)

I thought you were on board with that when you did the work to make these compatible in the first place: #86975.

I would be personally happy if we stopped running these because it's a huge pain to troubleshoot any bugs this finds due to the sheer size of the tests and it taking 7+ minutes to AOT compile. However this did find many product bugs that we didn't see anywhere else. I can't think of any other test that found so many issues (here's another from the past 6 months or so: #122582). So I'm conflicted. It would be good for me. But it would be worse for the product and customers.