Overall LGTM, but I think we should improve the two tests I mentioned if we are in this code.

Very good point. I'll add that shortly.
On 6 Feb 2015 07:42, "Glenn Block" [email protected] wrote:

Overall LGTM, but I think we should improve the two tests I mentioned if
we are in this code.

—
Reply to this email directly or view it on GitHub
#920 (comment).

@glennblock the serialization of Type methods is enormous. Are we sure we want to do this? IIRC this test was added after I witnessed typeof(Type).GetMethods(); blow up on stage for Jon Skeet at NDC London and ensuring an exception wasn't thrown was it's specific purpose.

The serialization of the Action and Func is of a manageable size.

Ugh... there's also a problem with asserting the Action and Func. The names of the hoisted methods are not deterministic. E.g. if I run just the single test method, the hoisted method is named <ShouldSerializeDelegates>b__3. If I run the entire test class, the hoisted method is named <ShouldSerializeDelegates>b__6.

I think we'll have to leave these tests as they are, but I've refactored them to 3A's, instead of using the Assert.Throws().

Ultimately, we're not trying to assert the serialization of all object types in this test class, but there are specific cases for which we originally had errors, i.e. the methods of Type and any Action or Func, so at least those tests are locking in the serialization of those types without errors.

OK, sounds good.