Add polymorphic tests for all root APIs #33391
Conversation
steveharter
added
area-System.Text.Json
increase-code-coverage
|
|
||
| public abstract class PolymorphicTests | ||
| { | ||
| private SerializationWrapper Serializer { get; } |
There was a problem hiding this comment.
nit: use a more verbose name rather than the general Serializer (helps with future find/replace refactorings).
Something like: CustomAllSerializer
There was a problem hiding this comment.
Maybe SerializeMethodOverload.
There was a problem hiding this comment.
You can still search for "Serializer." via whole word, same case. i.e. it doesn't collide with "JsonSerializer."
There was a problem hiding this comment.
I think the tests look good like this (maybe a bit confusing for first 5 seconds however)
json = Serializer.Serialize<object>(array);versus
json = CustomAllSerializer.Serialize<object>(array);or
json = Serializer.SerializeMethodOverload<object>(array);| var value = new { x = 1, y = true }; | ||
|
|
||
| // Strongly-typed. | ||
| string json = Serializer.Serialize(value); |
There was a problem hiding this comment.
Nice test. Could this apply on deserialization too? What about using value.GetType() to call the non-generic method?
There was a problem hiding this comment.
This is not supported as anonymous types don't have parameterless ctors (only a single parameterized ctor).
This is an area to test in the ctors PR #33444, although these types would have to be special cased as they violate the requirement that ctor parameter names should be the camelCase equivalent of the property names. Instead, they are the same string i.e a property named Prop would map to a ctor parameter named Prop, while prop would map to prop.
| namespace System.Text.Json.Serialization.Tests | ||
| { | ||
| public static class PolymorphicTests | ||
| public class PolymorphicTests_String : PolymorphicTests |
There was a problem hiding this comment.
Is there a way to also add the method overload permutations? Generic call vs one where we pass in type directly?
There was a problem hiding this comment.
Looks like all the string and Stream overloads are tested here.
@steveharter, how about adding the byte[] overloads as well (and return Encoding.UTF8.GetString(result) in the Serialize[<T>] overloads)?
|
@layomia @ahsonkhan any further changes requested? Otherwise please approve. |
layomia
left a comment
layomia
left a comment
There was a problem hiding this comment.
Great serializer method abstraction that'll be super useful for other test areas.
ghost
locked as resolved and limited conversation to collaborators
3 participants
Adds tests to cover #31464.
Previously the polymorphic tests only focused on the string-based serialization methods (not Stream, byte[] and Writer scenarios) and the issue above occurred during Stream scenarios.
The actual issue was recently fixed with #32669.
The "SerializationWrapper" helper class that was added here should be used in the future for other cases to increase code coverage, especially for Stream cases. At that time we will likely also add a "DeserializationWrapper" -- that deserialization wrapper wasn't added here since polymorphism is currently only supported during serialization.