Runtime: Add tests for DataContractSerializer serializing System.Net.Cookie types

[huanwu]

We need have tests covering DataContractSerializer serializing the following types.
Refer https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Serialization.Xml/tests/DataContractSerializer.cs on how to add the DCS tests using the common helper method.
dotnet/corefx#20329

• [ ] System.Net.Cookie
• [ ] System.Net.CookieCollection // I expect issues here as netfx and core are using different types in serialized fields
• [ ] System.Net.CookieContainer

[joperezr]

cc: @ViktorHofer since you have been looking at serialization tests.

[huanwu]

@davidsh System.net should be responsible for ensuring the type can be serialized correctly. So the test need be added and maintained in system.net project.

[davidsh]

cc: @karelz

[ViktorHofer]

I'm not sure that's the right approach, i.e. for binary serialization we put all the type tests into System.Runtime.Serialization.Formatters.Tests to guarantee a complete end to end test scenario (serialization off all serializable types into a stream, serialization one-by-one, roundtripping, etc.). All the test data is already written and can be found here: https://source.dot.net/#System.Runtime.Serialization.Formatters.Tests/BinaryFormatterTestData.cs,100

I don't know the exact number of types that are currently serializable but I think it's roughly 300-350. Adding single tests for each type results in a huge work load. I suggest to follow the same pattern we took with binary serialization.

[huanwu]

cc: @mconnew

[karelz]

@huanwu @zhenlan cc @danmosemsft I like the idea to make it more centralized to save overall cost. Is it something we could consider?

[mconnew]

@karelz, I disagree. If a serialization bug is introduced in a code change in the CookieCollection class, which test project should fail due to the product change? Should 3 different test projects for 3 different serializers fail for 1 bug in a different project? I think that a bug introduced in the CookieCollection class should cause a failure in the test project for the CookieCollection class. This class has the [Serializable] attribute so it's the class which is declaring that it is serializable, not the individual serializer implementations. Whether a class is serializable or not is a feature of the class, not a feature of the individual serializer implementation.
Also if a bug is introduced, who has the responsibility to fix it? The owner of the failing test or the owner of the class where the bug is introduced?

[danmosemsft]

@mconnew we verify BinaryFormatter scenarios on everything [Serializable], including CookieCollection. Could you clarify where the bug might lie if BinaryFormatter succeeds but DCS does not?

BTW centralization of tests in one file need not imply the ownership model. We are just observing that the approach you see in https://source.dot.net/#System.Runtime.Serialization.Formatters.Tests/BinaryFormatterTestData.cs,100 has worked very well. Note that each type is a separate test (they are a list processed by a [Theory])

[mconnew]

@danmosemsft, there's a bug in BinaryFormatter which we didn't fix on the full framework because it could have some really difficult to track down compat issues. The OnDeserialization callback won't get called sometimes if someone has overridden GetHashCode and Equals and two separate instances say they are the same object when taking into account the result of those two methods. In the tracking of whether an in stance of an object has been serialized or not we use RuntimeHelpers.GetHashCode and Object.ReferenceEquals which will say two objects are different objects. In the tracking of whether we've called OnDeserialized yet, we using object.GetHashCode and object.Equals which can both be overridden. So if you override those two methods in a type to say two different instances of an object are the same (such as would happen with a string) and you are using the OnDeserialized attribute, then the second instance deserialized from BinaryFormatter won't call the callback. DCS will call the callback. There's also some variations around OnDeserialized being called twice with structs in DCS. So you could rounttrip the same object with BinaryFormatter and DCS and in some situations BinaryFormatter might have the wrong result, and in others DCS might have the wrong result. And this is driven by implementation details of the type you are round tripping.

[danmosemsft]

@mconnew historically, how common are those cases? My concern is that it doesn't scale to have type owners also own testing they work with the various Microsoft serializers.

Presumably there are also cases where individual types throw up bugs in DCS itself.

The BinaryFormatter model is that my team owns https://source.dot.net/#System.Runtime.Serialization.Formatters.Tests/BinaryFormatterTestData.cs,100 for all [Serializable] types in .NET Core -- some types like InvalidDataContractException are owned by other teams. Each type only takes a few lines, and new types are just a few lines of copy/paste. Perhaps this model would work well for DCS.

[mconnew]

Ultimately I think it comes down to the question of who is responsible to ensure that a type attributed with Serializable can be serialized? I believe it's the person who is responsible for putting that attribute there.
I understand what you're saying about how simple the tests can be, but DCS is a little more complicated as the size of our output is a lot bigger so our tests can't be one liners (unless you are okay with 2000+ column text). Xml contains a lot of structural data which is just absent with BinaryFormatter. I have some questions:

1. If serialization tests starts failing for a particular type after a code change in that type, who is responsible for fixing the issue? The serializer owners or the type owner?
2. Is it expected multiple teams do multiple investigations into multiple breaking tests in multiple test projects before it's decided who will fix the issue? That's multiple people having to quickly gain context on an implementation which isn't their own and context on recent changes.
3. Are you expecting that someone iterating on changes to a single type runs the entirety of corefx tests for each modify/compile/test cycle to catch failing serialization tests or are you suggesting that a developer should wait until they are about done with their change to discover any breaking serialization changes? Neither of these seems very optimal to me from a developer efficiency standpoint.
4. When existing full framework types which aren't yet in .Net Core are added to corefx, if they are attributed with [Serializable], who is responsible for the tests? Is there a checklist as part of the api review which includes opening multiple issues against each serializer to add it to their respective test projects?
5. Who is responsible for any new types ability to be serialized? For example, there's a strong case that could be made to make Memory serializable. If the [Serializable] attribute were added to Memory<t>, who's responsibility is it to add tests to 4 different projects? (DCS, DCJS, XmlSerializer, BinaryFormatter).

At this point, most failing tests will be caused by a change in the implementing class and that's where a bug should be caught, not in an ancillary project. I believe that it should be expected that the tests for a particular class should be sufficient to find bugs in that class and there shouldn't be an expectation that other teams tests find your bugs.
Also it doesn't scale very well either. The number of classes will grow but the number of people working on serialization has been shrinking. There's a bandwidth issue here.
I also remembered a simpler scenario for when tests pass on one serializer but not on another. The class TimeZoneInfo serializes fine with BinaryFormatter but DCS can't serialize it. This is because of a bug in how TimeZoneInfo implemented ISerializable. Is this the fault of DCS for lack of DCS tests for a type not owned by those who own DCS? Or is this the fault of TimeZoneInfo for declaring that their type can be serialized and providing custom serialization logic which is broken?

[danmosemsft]

Sounds like it makes sense that the test(s) reside in CoreFX. It seems fine to me they are in one test library, that works well for binary formatter, and nothing can get committed without those tests passing. So the question is (1) who creates such tests for DCS, DCJS, XmlSerializer (2) who maintains them.

What do you think of you guys laying down in CoreFX similar skeleton tests for each of your serializers, including a few types, and then over time actual owners of types can add them to the test.

Because these tests would run in CI, any break would naturally be fixed by whoever broke it, without you needing to be involved.

[karelz]

Triage: Would be nice - we should bundle it with our larger effort to add more tests and cover test gaps we have.

cc @eiriktsarpalis