Porting Serialization Tests in NetNative Repo - #20308
Conversation
| } | ||
|
|
||
| [Fact] | ||
| public static void DCS_ReadOnlyDictionaryCausingDuplicateInvalidDataContract() |
There was a problem hiding this comment.
This test failed on netfx as test DCS_ReadOnlyDictionary did because of #18312.
@stone-li can you please move the test next to DCS_ReadOnlyDictionary and put the following attribute on it?
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "dotnet/corefx #18312")]
There was a problem hiding this comment.
I move the DCJS part found that the rest of the code same with DCS_ReadOnlyDictionary, so I remove this test in DCS, just keep DCJS part test
| Assert.StrictEqual(value["Foo"], deserializedValue["Foo"]); | ||
| Assert.StrictEqual(value["Bar"], deserializedValue["Bar"]); | ||
|
|
||
| DataContractJsonSerializer dcjs = new DataContractJsonSerializer(typeof(ReadOnlyDictionary<string, int>)); |
There was a problem hiding this comment.
The test code is incomplete. The original test probably was also incomplete.
Can you please complete this test and move this part to the DCJS test project?
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
| public string Name { get; set; } |
There was a problem hiding this comment.
Nit: NewLine after the bracket.
| } | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Can you please leave one NewLine and remove the extra ones? Thanks!
| dict["Foo"] = 1; | ||
| dict["Bar"] = 2; | ||
| var value = new ReadOnlyDictionary<string, int>(dict); | ||
| var deserializedValue = SerializeAndDeserialize(value, "{\"_dictionary\":[{\"Key\":\"Foo\",\"Value\":1},{\"Key\":\"Bar\",\"Value\":2}]}", null, () => new DataContractJsonSerializer(typeof(ReadOnlyDictionary<string, int>))); |
There was a problem hiding this comment.
This test is failing. After #20247 merged, we need to change the field name to m_dictionary. Besides, we now should be able to remove [SkipOnTargetFramework].
|
LGTM. Thanks for the pull request, @stone-li . |
|
Looks like this PR has regressed our uapaot ILC tests since it added the dependency to NativeDll.dll which is not present. It broke around 5 test projects for that reason, so we will have to find a way of separating that NativeDll.dll dependency. |
|
Oh great, I had not seen that earlier, thanks for fixing it! |
6 participants
@huanwu @shmao
Please help review the code