Add tests for reflection based serialization of coretypes #20247

ViktorHofer · 2017-05-24T15:52:46Z

Relates to https://github.com/dotnet/corefx/issues/19119

Follow up work

100% Core <--> Netfx serialization support
Adding ValueTuples netfx471 hashes
Renaming "binary serialization" comment to just "serialization" in coreclr and corefx.
Refine DataTable and DataSet equality comparison
Assert.Fail instead of exceptions

danmoseley · 2017-05-24T15:54:40Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs

+        public void ValidateAgainstFile(object original, string coreBase64, string netfxBase64)
+        {
+            CheckForAnyEquals(original, LoadFromFile(coreBase64));
+            //CheckForAnyEquals(originalType, LoadFromFile(netfxBase64));


intentional for this PR. I rushed to finish core <--> core serialization and also moved test data to code as @stephentoub suggested.

danmoseley · 2017-05-24T15:55:09Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs

+                }
+                else
+                {
+                    // Check if object.Equals(object) is overriden and if not check if there is a more concrete equality check implementation


typo, 'overridden'

ViktorHofer · 2017-05-24T15:57:33Z

I added all necessary equality checks as extension methods. Some of them could be ported directly into the respective types. Let's discuss that after everything else.

danmoseley · 2017-05-24T15:58:27Z

src/System.Runtime.Serialization.Formatters/tests/SerializationTypes.cs

                T[] sub1 = array1[i], sub2 = array2[i];
-                if (sub1 == null || sub2 == null && (sub1 != sub2)) return false;
-                if (sub1.Length != sub2.Length) return false;
+                if (sub1 == null || sub2 == null && (sub1 != sub2))


Please use parentheses when mixing || and && so I don't have to remember precedence order.

Also, if sub1 == sub2 == null then this will fail on the length check. Should it be

if (sub1 == null || sub2 == null) return sub1 == sub2;

danmoseley · 2017-05-24T15:59:07Z

src/System.Runtime.Serialization.Formatters/tests/SerializationTypes.cs

+    [Serializable]
+    internal sealed class PointEqualityComparer : IEqualityComparer<Point>
+    {
+        public bool Equals(Point x, Point y) => x.X == y.X && x.Y == y.Y;


nit, parens would be good

danmoseley · 2017-05-24T16:02:28Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs

+        /// <summary>
+        /// First: Object
+        /// Second: Core Base64 serialized string
+        /// Third: Netfx Base64 serialized string 


At some point in the future we'll have to update these base64 values. Eg we added a field in hopefully a non breaking way. At that point we will need to have blobs for both old and new and test both directions. I guess that is a problem to solve at that time.

I thought a lot about what the best data structure could be for this case. Putting the base64 strings into a different container for every configuration brings the hassle of references to the objects. The base64 hash of e.g. byte.MaxValue must reference the value byte.MaxValue. Therefore I decided to put them together in one place.
If we e.g. need a dedicated hash for net471 we could just add another object to the array.

danmoseley · 2017-05-24T19:01:59Z

@krwq

danmoseley · 2017-05-24T19:54:20Z

fyi @zhenlan @shmao - DCS tests may build on this foundation

krwq · 2017-05-24T21:41:36Z

EDIT @ViktorHofer: Updated list as it contained too many types. Moved list to top.

morganbr · 2017-05-25T02:32:44Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs

        private static int Identity(int i) => i;

+        [Fact]
+        public void Roundtrip_Delegates_NoTarget()


Delegates aren't supposed to be serializable.

I seem to recall deleting these. You might need to rebase.

morganbr · 2017-05-25T02:44:00Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs

+        {
+            if (extendedType.IsGenericType)
+            {
+                return typeof(EqualityExtensions).GetMethods()


Using reflection this complex would probably require writing rd.xml for aot. If you don't want to figure that out, a switch statement or dictionary of delegates might be easiest.

morganbr · 2017-05-25T03:29:43Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs

+{
+    public partial class BinaryFormatterTests
+    {
+        /// <summary>


Please add a comment on how to generate these strings.

morganbr · 2017-05-25T03:31:59Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs

+            //yield return ValueTuple.Create(1, "2", 3u, 4L, 5.6, 7.8f, 9m };
+            //yield return ValueTuple.Create(1, "2", 3u, 4L, 5.6, 7.8f, 9m, Tuple.Create(10) };
+            yield return new object[] { new BigInteger(), "AAEAAAD/////AQAAAAAAAAAMAgAAAFpTeXN0ZW0uUnVudGltZS5OdW1lcmljcywgVmVyc2lvbj00LjEuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWIwM2Y1ZjdmMTFkNTBhM2EFAQAAABpTeXN0ZW0uTnVtZXJpY3MuQmlnSW50ZWdlcgIAAAAFX3NpZ24FX2JpdHMABwgPAgAAAAAAAAAKCw==", "AAEAAAD/////AQAAAAAAAAAMAgAAAFJTeXN0ZW0uTnVtZXJpY3MsIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5BQEAAAAaU3lzdGVtLk51bWVyaWNzLkJpZ0ludGVnZXICAAAABV9zaWduBV9iaXRzAAcIDwIAAAAAAAAACgs=" };
+            yield return new object[] { new BigInteger(10324176), "AAEAAAD/////AQAAAAAAAAAMAgAAAFpTeXN0ZW0uUnVudGltZS5OdW1lcmljcywgVmVyc2lvbj00LjEuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWIwM2Y1ZjdmMTFkNTBhM2EFAQAAABpTeXN0ZW0uTnVtZXJpY3MuQmlnSW50ZWdlcgIAAAAFX3NpZ24FX2JpdHMABwgPAgAAANCInQAKCw==", "AAEAAAD/////AQAAAAAAAAAMAgAAAFJTeXN0ZW0uTnVtZXJpY


I had to also add System.Collections.Specialized.NameObjectCollectionBase

ViktorHofer · 2017-05-25T08:32:45Z

@karelz This is 2.0 work and must be finished by tomorrow...

The MemberData is enumerated promptly, such that even though we're yielding the graph object, subsequent modifications to that same object are manifested prior to that object being tested, and it doesn't match the serialized version.

shmao · 2017-05-31T20:28:09Z

src/System.Runtime.Serialization.Xml/tests/DataContractSerializer.cs

        dict["Bar"] = 2;
        ReadOnlyDictionary<string, int> value = new ReadOnlyDictionary<string, int>(dict);
-        var deserializedValue = SerializeAndDeserialize(value, @"<ReadOnlyDictionaryOfstringint xmlns=""http://schemas.datacontract.org/2004/07/System.Collections.ObjectModel"" xmlns:i=""http://www.w3.org/2001/XMLSchema-instance""><_dictionary xmlns:a=""http://schemas.microsoft.com/2003/10/Serialization/Arrays""><a:KeyValueOfstringint><a:Key>Foo</a:Key><a:Value>1</a:Value></a:KeyValueOfstringint><a:KeyValueOfstringint><a:Key>Bar</a:Key><a:Value>2</a:Value></a:KeyValueOfstringint></_dictionary></ReadOnlyDictionaryOfstringint>");
+        var deserializedValue = SerializeAndDeserialize(value, @"<ReadOnlyDictionaryOfstringint xmlns=""http://schemas.datacontract.org/2004/07/System.Collections.ObjectModel"" xmlns:i=""http://www.w3.org/2001/XMLSchema-instance""><m_dictionary xmlns:a=""http://schemas.microsoft.com/2003/10/Serialization/Arrays""><a:KeyValueOfstringint><a:Key>Foo</a:Key><a:Value>1</a:Value></a:KeyValueOfstringint><a:KeyValueOfstringint><a:Key>Bar</a:Key><a:Value>2</a:Value></a:KeyValueOfstringint></m_dictionary></ReadOnlyDictionaryOfstringint>");


Can you please remove the [SkipOnTargetFramework] on the test? The test is being skipped on netfx. Removing the attribute would enable the test on netfx so that we get desktop<-->core coverage..

The reason for the attribute is already gone?

The test failed on netfx because of the naming difference between core and desktop. It should pass after your fix.

Besides, you just closed #18312, which is the issue being tracked by the attribute. :)

shmao · 2017-05-31T20:28:17Z

src/System.Runtime.Serialization.Json/tests/DataContractJsonSerializer.cs

        dict["Bar"] = 2;
        ReadOnlyDictionary<string, int> value = new ReadOnlyDictionary<string, int>(dict);
-        var deserializedValue = SerializeAndDeserialize(value, @"{""_dictionary"":[{""Key"":""Foo"",""Value"":1},{""Key"":""Bar"",""Value"":2}]}");
+        var deserializedValue = SerializeAndDeserialize(value, @"{""m_dictionary"":[{""Key"":""Foo"",""Value"":1},{""Key"":""Bar"",""Value"":2}]}");


Can you please remove the [SkipOnTargetFramework] on the test? The test is being skipped on netfx. Removing the attribute would enable the test on netfx so that we get desktop<-->core coverage..

shmao · 2017-05-31T21:47:40Z

src/System.Runtime.Serialization.Formatters/tests/SerializationTypes.cs

        public int X;
        public int Y;

+        public Point()


Question: why adding this public default ctor? This may have changed the test scenario. Before the change, the test was for type without public default ctor; after the fix, the test is for type having public constructor. DCS deal with these two cases differently. I'm not sure how BinaryFormatter would deal with this.

@krwq is following up on your question and testing locally without the default constructor. I think there was a reason why I had to add it.

…ntCulture

krwq · 2017-05-31T22:11:42Z

@dotnet-bot test OSX x64 Debug Build please

…tes hashes

krwq · 2017-05-31T23:16:54Z

@morganbr @ViktorHofer I've changed culture inside the test and also saw two other test cases failing and have fixed them with setting Locale in one of their fields:

new DataSet("Dataset") { Locale = CultureInfo.InvariantCulture }
new DataTable("Datatable", "corens") { Locale = CultureInfo.InvariantCulture }

I couldn't repro the issue from the CI but the hashes were likely not updated due to RegEx replacing only lines which had yield and hash on the same line - I've changed the test data so that this happens

morganbr · 2017-06-01T00:03:28Z

@dotnet-bot test Linux x64 Tests - Release - Ubuntu.1404.Amd64.Open

morganbr · 2017-06-01T00:03:45Z

@dotnet-bot test Linux x64 Tests - Release - Ubuntu.1404.Amd64.Open please

danmoseley · 2017-06-01T00:10:05Z

The remaining failure is https://github.com/dotnet/corefx/issues/20245. It is not worth rerunning again int this case.Merging.

Add tests for reflection based serialization of coretypes

ViktorHofer added the area-System.Runtime label May 24, 2017

ViktorHofer added this to the 2.0.0 milestone May 24, 2017

ViktorHofer self-assigned this May 24, 2017

ViktorHofer requested review from danmoseley, morganbr and stephentoub May 24, 2017 15:52

dnfclas added the cla-already-signed label May 24, 2017

danmoseley reviewed May 24, 2017

View reviewed changes

ViktorHofer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 24, 2017

danmoseley reviewed May 24, 2017

View reviewed changes

ViktorHofer changed the title ~~Add core to core serialization tests~~ [WIP] Add tests for serialization May 24, 2017

ViktorHofer removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 24, 2017

karelz modified the milestones: 2.1.0, 2.0.0 May 25, 2017

morganbr approved these changes May 25, 2017

View reviewed changes

ViktorHofer modified the milestones: 2.0.0, 2.1.0 May 25, 2017

ViktorHofer force-pushed the SerializationTests branch 3 times, most recently from 1d58e56 to e520c99 Compare May 25, 2017 12:09

karelz modified the milestones: 2.1.0, 2.0.0 May 25, 2017

ViktorHofer and others added 6 commits May 31, 2017 12:17

SortVersion & BindingList

9715d0e

Fix Graph test

1de4eb9

The MemberData is enumerated promptly, such that even though we're yielding the graph object, subsequent modifications to that same object are manifested prior to that object being tested, and it doesn't match the serialized version.

fix hashes and equality comparers

274551b

do not run code which generates test data

86a6a53

IsEqual override for DictionaryEntry, disable some netfx test cases

ff7df2d

fix intermittent failures related to StringDictionary

99bc058

krwq force-pushed the SerializationTests branch from ef7d2a5 to 99bc058 Compare May 31, 2017 19:18

krwq and others added 4 commits May 31, 2017 12:52

disable failing TimeZoneInfo test case

2023ee0

merge

135bec3

Fix ReadOnlyDictionary DCS test

aca76e6

Fix ReadOnlyDictionary JSON DCS test

9e759bd

shmao reviewed May 31, 2017

View reviewed changes

Renabling ReadOnlyDictionary tests on netfx

ba0f3ad

This was referenced May 31, 2017

Merge serialization changes to 2.0.0 #20529

Closed

Merge serialization changes to 2.0.0 #20530

Merged

shmao reviewed May 31, 2017

View reviewed changes

krwq added 2 commits May 31, 2017 14:54

Update core serialization data after changing CurrentCulture->Invaria…

65d690c

…ntCulture

remove default constructor from the Point test class

722332a

move some data to single line with yield so that regex correctly upda…

6758c2b

…tes hashes

danmoseley merged commit 7cb3daa into dotnet:master Jun 1, 2017

krwq pushed a commit to krwq/corefx that referenced this pull request Jun 1, 2017

Merge pull request dotnet#20247 from ViktorHofer/SerializationTests

259506a

Add tests for reflection based serialization of coretypes

shmao mentioned this pull request Jun 1, 2017

Porting Serialization Tests in NetNative Repo #20308

Merged

ViktorHofer deleted the SerializationTests branch June 1, 2017 12:10

krwq pushed a commit to krwq/corefx that referenced this pull request Jun 1, 2017

Merge pull request dotnet#20247 from ViktorHofer/SerializationTests

53968d8

Add tests for reflection based serialization of coretypes

shmao mentioned this pull request Jan 31, 2020

Add tests for DCS Serializing System Types dotnet/runtime#21982

Closed

43 tasks

Add tests for reflection based serialization of coretypes #20247

Add tests for reflection based serialization of coretypes #20247

Uh oh!

Conversation

ViktorHofer commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Follow up work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented May 24, 2017

Uh oh!

danmoseley May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented May 24, 2017

Uh oh!

danmoseley commented May 24, 2017

Uh oh!

krwq commented May 24, 2017 • edited by ViktorHofer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq commented May 31, 2017

Uh oh!

krwq commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganbr commented Jun 1, 2017

Uh oh!

morganbr commented Jun 1, 2017

Uh oh!

danmoseley commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

ViktorHofer commented May 24, 2017 •

edited

Loading

danmoseley May 24, 2017 •

edited

Loading

krwq commented May 24, 2017 •

edited by ViktorHofer

Loading

krwq commented May 31, 2017 •

edited

Loading