Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
tmds opened this issue Sep 9, 2024 · 11 comments · May be fixed by #107549
Open

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

tmds opened this issue Sep 9, 2024 · 11 comments · May be fixed by #107549
Labels
area-System.Runtime binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner

Comments

@tmds
Copy link
Member

tmds commented Sep 9, 2024

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
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 9, 2024
@am11 am11 added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 9, 2024
@adamsitnik
Copy link
Member

@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
Copy link
Member

adamsitnik commented Sep 9, 2024 •
edited
Loading

cc @bartonjs

@tmds
Copy link
Member Author

tmds commented Sep 9, 2024

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
Copy link
Member

am11 commented Sep 9, 2024 

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
Copy link
Member

am11 commented Sep 9, 2024 •
edited
Loading

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
Copy link
Member Author

tmds commented Sep 9, 2024

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.

@adamsitnik adamsitnik mentioned this issue Sep 9, 2024
Draft
@tmds tmds linked a pull request Sep 9, 2024 that will close this issue
Open
@dotnet-policy-service dotnet-policy-service bot added in-pr There is an active PR which will close this issue when it is merged labels Sep 9, 2024
@ericstj ericstj added area-System.Runtime binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it and removed area-Infrastructure-libraries labels Sep 9, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@ericstj
Copy link
Member

ericstj commented Sep 9, 2024

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
Copy link
Member Author

tmds commented Sep 9, 2024

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
Copy link
Member

ericstj commented Sep 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-System.Runtime binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Disable binary formatter tests when DotNetBuildSourceOnly. tmds/runtime
4 participants
@tmds @am11 @adamsitnik @ericstj