fix(66900): Adding test for confirm bug fix. #87167
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsCurrent PR simply adding test for confirm that my previous PR also make fixes for #66900
|
|
|
||
| instanceOfSecondClassWithSameName = new Class2(); | ||
|
|
||
| Assert.NotEmpty(JsonSerializer.Serialize(instanceOfSecondClassWithSameName)); |
There was a problem hiding this comment.
Could you
- Update the assertion to validate the content of the output.
- Validate that it deserializes back to the expected value?
There was a problem hiding this comment.
Fixed by amend: 2d20edd
Maximys
force-pushed
the
bugfix/66900-invalid-processing-of-hidden-properties
branch
from
c2fbafa to
2d20edd
Compare
| @@ -91,6 +92,24 @@ public static void AppendCharacterWhenSerializingField() | |||
| Assert.Equal(originalObj.TestProperty, deserialized.TestProperty); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public static void CorrectlyIgnorePropertyWithNewModifier() | |||
There was a problem hiding this comment.
I thought I had left this feedback earlier, but I don't see it now. I don't believe this is the appropriate test suite for this type of test. Could you please move it to PropertyVisibilityTests.cs? Thanks
There was a problem hiding this comment.
Moved by amend: a9ca71c
eiriktsarpalis
added
the
needs-author-action
Maximys
force-pushed
the
bugfix/66900-invalid-processing-of-hidden-properties
branch
from
2d20edd to
a9ca71c
Compare
ghost
removed
the
needs-author-action
| namespace System.Text.Json.Serialization.Tests | ||
| { | ||
| public sealed partial class PropertyVisibilityTestsDynamic : PropertyVisibilityTests | ||
| { | ||
| public PropertyVisibilityTestsDynamic() : base(JsonSerializerWrapper.StringSerializer) { } | ||
|
|
||
| [Fact] | ||
| public void CorrectlyIgnorePropertyWithNewModifier() |
There was a problem hiding this comment.
The test should run the same for both reflection and the source generator. Would it be possible to move the test to the base PropertyVisibilityTests class? Note that this would additionally require the following two changes:
- Replace any direct calls to
JsonSerializer.Serialize/Deserializewith theSerializer.SerializeWrappertest abstraction. - For the case of the source generator, add the test classes to the relevant JsonSerializerContext declaration.
Thanks!
Maximys
force-pushed
the
bugfix/66900-invalid-processing-of-hidden-properties
branch
2 times, most recently
from
fd54759 to
c49cef7
Compare
....Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyVisibilityTests.cs
Show resolved
Hide resolved
Maximys
force-pushed
the
bugfix/66900-invalid-processing-of-hidden-properties
branch
from
6e3f009 to
68a426e
Compare
|
Test failures are due to #87680 and unrelated to this change. |
ghost
locked as resolved and limited conversation to collaborators
Current PR simply adding test for confirm that my previous PR also make fixes for #66900