Number handling with converters #57525
Conversation
|
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue Detailsnull
|
| // Ensure custom converter is honored. | ||
| var obj = JsonSerializer.Deserialize<ClassWith_NumberHandlingOn_Property_WithCustomConverter>(json); | ||
| Assert.Equal(25, obj.Prop); | ||
| Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(obj)); |
There was a problem hiding this comment.
I was initially confused by this assertion, but then I realized it's an exception thrown by the custom converter. Do we need to test for this here? Or if we do, shouldn't we be testing an implemented converter?
There was a problem hiding this comment.
The read side is implemented to return 25, so I believe it is implied the write side will of course get called and thus is kept simple by just throwing instead of writing 25.
There are tests later that has both read- and write- sides
...libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NumberHandlingTests.cs
Outdated
Show resolved
Hide resolved
| Assert.Contains(typeof(ConverterForMyType).ToString(), exAsStr); | ||
| Assert.Contains(typeof(ClassWith_NumberHandlingOn_Type_WithCustomConverter).ToString(), exAsStr); | ||
| // Assert regular Read/Write methods on custom converter are called. | ||
| Assert.Throws<NotImplementedException>(() => JsonSerializer.Deserialize<ClassWith_NumberHandlingOn_Type_WithCustomConverter>(json)); |
There was a problem hiding this comment.
Similar to my comment above, throwing NotImplementedException to test whether a custom converter is being exercised is hard to parse. I think we need to test for the happy path (if anything because we might have an undetected regression because another part of the testing infrastructure throws the same exception type, or because there might be another bug hiding downstream of the thrown exception).
There was a problem hiding this comment.
I've added additional comments to clarify this, and also returned a literal in the exception text to verify it was coming from the custom converter. We can discuss further with @layomia in a follow-up.
...libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NumberHandlingTests.cs
Outdated
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
Replacement PR for #42239. It has the same semantics as that PR; just merged to latest code and added a test\sample.
Fixes #51685