BinaryFormatter tests should be skipped only on AOT, WASM and Mobile

[adamsitnik]

A follow up to the discussion we have started at #106737

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[adamsitnik]

The initial change I’ve sent with this PR has allowed me to discover that there were 19 projects where BF-related tests were skipped since #103255 was merged.

@ericstj I’ve re-enabled those tests by adding a project reference similar to what you did in #106737. We don’t want to run these tests on Mobile, WASM and NativeAOT (not supported by design). How adding such project reference is going to affect the build time for those configurations? Should we somehow make it conditional? If so, should we introduce an MSBuild property (or any other setting) and handle all that logic is some .targets file so we would not need to copy it everywhere and update all occurrences when changes are needed?

@bartonjs There are 5 System.Collections.Tests tests that fail due to a product change. I think you have mentioned it at some point, but I just want to make sure that we are aware of that change, it’s documented and expected.

    Starting:    System.Collections.Tests (parallel test collections = on [24 threads], stop on fail = off)
      System.Collections.Tests.HashSet_Generic_Tests_string.ComparerSerialization [FAIL]
        Assert.IsType() Failure: Value is not the exact type
        Expected: typeof(System.Collections.Generic.StringEqualityComparer)
        Actual:   typeof(System.Collections.Generic.GenericEqualityComparer<string>)
        Stack Trace:
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(891,0): at System.Collections.Tests.HashSet_Generic_Tests`1.<ComparerSerialization>g__TestComparerSerialization|64_0[TCompare
  d](IEqualityComparer`1 equalityComparer, String internalTypeName)
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(859,0): at System.Collections.Tests.HashSet_Generic_Tests`1.ComparerSerialization()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\projects\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      System.Collections.Tests.Dictionary_Tests.ComparerSerialization [FAIL]
        Assert.IsType() Failure: Value is not the exact type
        Expected: typeof(System.Collections.Generic.StringEqualityComparer)
        Actual:   typeof(System.Collections.Generic.GenericEqualityComparer<string>)
        Stack Trace:
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\Dictionary\Dictionary.Tests.cs(457,0): at System.Collections.Tests.Dictionary_Tests.TestComparerSerialization[T](IEqualityComparer`1 equalityComparer, String
  internalTypeName)
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\Dictionary\Dictionary.Tests.cs(426,0): at System.Collections.Tests.Dictionary_Tests.ComparerSerialization()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\projects\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      System.Collections.Tests.HashSet_Generic_Tests_int.ComparerSerialization [FAIL]
        Assert.IsType() Failure: Value is not the exact type
        Expected: typeof(System.Collections.Generic.StringEqualityComparer)
        Actual:   typeof(System.Collections.Generic.GenericEqualityComparer<string>)
        Stack Trace:
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(891,0): at System.Collections.Tests.HashSet_Generic_Tests`1.<ComparerSerialization>g__TestComparerSerialization|64_0[TCompare
  d](IEqualityComparer`1 equalityComparer, String internalTypeName)
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(859,0): at System.Collections.Tests.HashSet_Generic_Tests`1.ComparerSerialization()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\projects\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      System.Collections.Tests.HashSet_Generic_Tests_int_With_Comparer_WrapStructural_SimpleInt.ComparerSerialization [FAIL]
        Assert.IsType() Failure: Value is not the exact type
        Expected: typeof(System.Collections.Generic.StringEqualityComparer)
        Actual:   typeof(System.Collections.Generic.GenericEqualityComparer<string>)
        Stack Trace:
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(891,0): at System.Collections.Tests.HashSet_Generic_Tests`1.<ComparerSerialization>g__TestComparerSerialization|64_0[TCompare
  d](IEqualityComparer`1 equalityComparer, String internalTypeName)
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(859,0): at System.Collections.Tests.HashSet_Generic_Tests`1.ComparerSerialization()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\projects\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      System.Collections.Tests.HashSet_Generic_Tests_int_With_Comparer_WrapStructural_Int.ComparerSerialization [FAIL]
        Assert.IsType() Failure: Value is not the exact type
        Expected: typeof(System.Collections.Generic.StringEqualityComparer)
        Actual:   typeof(System.Collections.Generic.GenericEqualityComparer<string>)
        Stack Trace:
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(891,0): at System.Collections.Tests.HashSet_Generic_Tests`1.<ComparerSerialization>g__TestComparerSerialization|64_0[TCompare
  d](IEqualityComparer`1 equalityComparer, String internalTypeName)
          D:\projects\runtime\src\libraries\System.Collections\tests\Generic\HashSet\HashSet.Generic.Tests.cs(859,0): at System.Collections.Tests.HashSet_Generic_Tests`1.ComparerSerialization()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\projects\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
    Finished:    System.Collections.Tests

[bartonjs]

There are 5 System.Collections.Tests tests that fail due to a product change. I think you have mentioned it at some point, but I just want to make sure that we are aware of that change, it’s documented and expected.

If I did, I have no recollection of it. The BF compat package is just BF. Nothing should be different between using it in 9 and using BF in 8.

[ericstj]

How adding such project reference is going to affect the build time for those configurations? Should we somehow make it conditional?

It shouldn't add any time to a root level build. This project already builds and it will continue to only build once - MSBuild will get the cached value. As for leaf builds of individual projects - it will increase the time, however if folks want to avoid that they can always pass --no-dependencies to skip project references.

I wonder if we really want to have all these binary-formatter tests spread across the repo. Maybe we should consolidate them to limit the number of tests that use the OOB?

[bartonjs]

I don't understand why you're taking the assembly version logic out.

[adamsitnik]

I don't understand why you're taking the assembly version logic out.

Because it has hidden the fact that 19 projects were using the "throwing" version of BF (the tests were always skipped so any test failures would go unnoticed). With this change such tests are going to throw if they reference the wrong version.

[adamsitnik]

@bartonjs you were right, the failures were caused by changes from #104202. I've fixed them, the tests should now pass.

The best explanation can be found here:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs

Lines 277 to 295 in c05e9d1

	// This type is added as an internal implementation detail in .NET 9. Even though as of .NET 9 BinaryFormatter has been
	// deprecated, for back compat we still need to support serializing this type, especially when EqualityComparer<string>.Default
	// is used as part of a collection, like Dictionary<string, TValue>.
	//
	// BinaryFormatter treats types in the core library as being special, in that it doesn't include the assembly as part of the
	// serialized data, and then on deserialization it assumes the type is in mscorlib. We could make the type public and type forward
	// it from the mscorlib shim, which would enable roundtripping on .NET 9+, but because this type doesn't exist downlevel, it would
	// break serializing on .NET 9+ and deserializing downlevel. Therefore, we need to serialize as something that exists downlevel.
	// We could serialize as OrdinalComparer, which does exist downlevel, and which has the nice property that it also implements
	// IAlternateEqualityComparer<ReadOnlySpan<char>, string>, which means serializing an instance on .NET 9+ and deserializing it
	// on .NET 9+ would continue to support span-based lookups. However, OrdinalComparer is not an EqualityComparer<string>, which
	// means the type's public ancestry would not be retained, which could lead to strange casting-related errors, including downlevel.
	// Instead, we can serialize as a GenericEqualityComparer<string>. This exists downlevel and also derives from EqualityComparer<string>,
	// but doesn't implement IAlternateEqualityComparer<ReadOnlySpan<char>, string>. This means that upon deserializing on .NET 9+,
	// the comparer loses its ability to handle span-based lookups. As BinaryFormatter is deprecated on .NET 9+, this is a readonable tradeoff.
	info.SetType(typeof(GenericEqualityComparer<string>));

[adamsitnik]

I wonder if we really want to have all these binary-formatter tests spread across the repo. Maybe we should consolidate them to limit the number of tests that use the OOB?

I like this idea, but due to time constraints I would prefer to do it after we ship 9. Is that acceptable?

[adamsitnik]

@ericstj @bartonjs the PR is ready for review, PTAL