BinaryFormatter tests are no longer skipped when ran against source-built runtime

[tmds]

When building runtime under the source-build configuration, no functional BinaryFormatter is being built.

Tests that require a functional binary formatter are skipped based on:

runtime/src/libraries/System.ObjectModel/tests/ReadOnlyDictionary/ReadOnlyDictionary_SerializationTests.cs

Line 19 in aa418fc

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsBinaryFormatterSupported))]

These tests are no longer being skipped on the main branch. This causes 1000s of test failures across different test projects.

@adamsitnik I suspect this regressed in #106858 by removing the assembly version logic in DetermineBinaryFormatterSupport.

cc @ViktorHofer @omajid

[adamsitnik]

@ViktorHofer is offline, tagging @ericstj and @carlossanlop who should be able to help, as I am not sure why functional BinaryFormatter is not being built for the source-build configuration (I know very little about this config and need help myself).

[adamsitnik]

cc @bartonjs

[tmds]

as I am not sure why functional BinaryFormatter is not being built for the source-build configuration

This is intentional because BinaryFormatter has no use in source-build. We only build the in-box implementation that throws PNSE.

[am11]

diff --git a/Directory.Build.props b/Directory.Build.props
index e887d066f5c..ab8adda9fb2 100644
--- a/Directory.Build.props
+++ b/Directory.Build.props
@@ -473,6 +473,7 @@
   <PropertyGroup Condition="'$(IsTestProject)' == 'true' or '$(IsTestSupportProject)' == 'true' or '$(IsTrimmingTestProject)' == 'true'">
     <!-- we need to re-enable BinaryFormatter within test projects since some tests exercise these code paths to ensure compat -->
     <EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization>
+    <EnableUnsafeBinaryFormatterSerialization Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</EnableUnsafeBinaryFormatterSerialization>
     <!-- don't warn on usage of BinaryFormatter or legacy serialization infrastructure from test projects -->
     <NoWarn>$(NoWarn);SYSLIB0011;SYSLIB0050;SYSLIB0051</NoWarn>
     <!-- don't warn about unnecessary trim warning suppressions. can be removed with preview 6. -->

Maybe this should fix it for source bulid?

[am11]

Or this:

diff --git a/eng/testing/tests.props b/eng/testing/tests.props
index 2c555d1abaf..d81fc91ab85 100644
--- a/eng/testing/tests.props
+++ b/eng/testing/tests.props
@@ -9,6 +9,7 @@
     <ILLinkDescriptorsPath>$(MSBuildThisFileDirectory)ILLinkDescriptors\</ILLinkDescriptorsPath>
     <TestSingleFile Condition="'$(TestNativeAot)' == 'true'">true</TestSingleFile>
     <TestSingleFile Condition="'$(TestReadyToRun)' == 'true'">true</TestSingleFile>
+    <DefineConstants Condition="'$(DotNetBuildSourceOnly)' == 'true'">$(DefineConstants);DOTNET_SOURCE_BUILD</DefineConstants>
   </PropertyGroup>

   <PropertyGroup Condition="'$(TargetsMobile)' == 'true'">
diff --git a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs
index d35885ae255..ca79cc1c92a 100644
--- a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs
+++ b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs
@@ -728,6 +728,10 @@ private static bool GetSupportsSha3()

         private static bool DetermineBinaryFormatterSupport()
         {
+#if DOTNET_SOURCE_BUILD
+            return false; // binary formatter, by design, throws PNSE with source-build
+#endif
+
             if (IsNetFramework)
             {
                 return true;

[tmds]

Thanks for the suggestions @am11!

+    <EnableUnsafeBinaryFormatterSerialization Condition="'$(DotNetBuildSourceOnly)' == 'true'">false</EnableUnsafeBinaryFormatterSerialization>

This would be a good fix if it works. Let me give it a try.

#if DOTNET_SOURCE_BUILD

Let's try to avoid going this way.

[dotnet-policy-service]

Tagging subscribers to 'binaryformatter-migration': @adamsitnik, @bartonjs, @jeffhandley, @terrajobst

[dotnet-policy-service]

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

[ericstj]

I think we need consistency with how the expectation for support is expressed.

In the past it used to be binary: a platform supported it or it didn't.

Now there are 3 states:

1. Platform doesn't support it.
2. Platform supports it, but the OOB implementation isn't referenced.
3. Platform supports it and the OOB implementation is referenced.

What you don't want to happen is accidentally losing test coverage because you fall into state 2 and treat it like 1.

Can you state how you want these to be expressed, then adopt them consistently? I feel like applying different conditions in many places will lead you to a fragile solution.

[tmds]

For System.Numerics.Tensors there is a separate test project to validate the .NET 8 sources. Perhaps something similar could allow to test the different implementations in upstream CI.

For source-build configuration, we accept lower test coverage. If the OOB implementation isn't buildable, then the tests that depend on its behavior should be skipped.

[ericstj]

At least right now there are dozens of projects that still test the BinaryFormatter. I agree consolidating those to a single project might be a good idea. That way we don't accidentally depend on behavior that's only present in the compat package.