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.

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

Remove package references from library tests #106737

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Aug 20, 2024

These tests should be referencing the product assemblies so that they test latest and not old bits.

We should also port this to 9.0. Found this when looking at #106729. None of these caused that issue, but the usage was wrong.

We should only be using PackageReferences to other things which build in dotnet/runtime when we need to reference something not built in the same subset. It must also not need to execute during the build. I wish there was some enforcement for this - right now we just have to try and catch it in code review.

These tests should be referencing the product assemblies so that they
test latest and not old bits.
Copy link
Contributor

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

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the P2Ps to BinaryFormatter resolve the NetCoreAppCurrent assembly (PNSE)? We need the NetCoreAppMinimum one (the functioning one). You could add SetTargetFramework=$(NetCoreAppMinimum) to the P2Ps to solve that.

@ericstj
Copy link
Member Author

ericstj commented Aug 21, 2024

Won't the P2Ps to BinaryFormatter resolve the NetCoreAppCurrent assembly (PNSE)? We need the NetCoreAppMinimum one (the functioning one). You could add SetTargetFramework=$(NetCoreAppMinimum) to the P2Ps to solve that.

I had suspected to see failures here but didn't. I'll take a look - also see if I can add assertions to force a failure if it breaks.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the P2Ps to BinaryFormatter resolve the NetCoreAppCurrent assembly (PNSE)? We need the NetCoreAppMinimum one (the functioning one). You could add SetTargetFramework=$(NetCoreAppMinimum) to the P2Ps to solve that.

@ViktorHofer Is correct. The tfm trick can be found here:

<FunctioningBinaryFormatter Condition="'$(TargetFramework)' == '$(NetCoreAppMinimum)' and '$(DotNetBuildSourceOnly)' != 'true'">true</FunctioningBinaryFormatter>

I had suspected to see failures here but didn't.

All BF tests are marked as conditional. The assembly version is used to determine whether they can be executed:

Assembly assembly = typeof(System.Runtime.Serialization.Formatters.Binary.BinaryFormatter).Assembly;
AssemblyName name = assembly.GetName();
Version assemblyVersion = name.Version;
bool isSupported = true;
// Version 8.1 is the version in the shared runtime (.NET 9+) that has the type disabled with no config.
// Assembly versions beyond 8.1 are the fully functional version from NuGet.
// Assembly versions before 8.1 probably won't be encountered, since that's the past.
if (assemblyVersion.Major == 8 && assemblyVersion.Minor == 1)
{
isSupported = false;
}

Personally I was using the number of skipped tests as an indicator whether they are actually being run.

@ericstj
Copy link
Member Author

ericstj commented Aug 21, 2024

@adamsitnik would it be OK if I removed the use of IsBinaryFormatterSupported in every project where we specifically want to test the behavior with a functioning BinaryFormatter? That will make sure this doesn't regress in the future.

@ericstj
Copy link
Member Author

ericstj commented Aug 21, 2024

@adamsitnik would it be OK if I removed the use of IsBinaryFormatterSupported in every project where we specifically want to test the behavior with a functioning BinaryFormatter? That will make sure this doesn't regress in the future.

I gave this a try, but then realized that property is doing double duty. It's also covering cases where the platform doesn't support it, and it predates the removal:

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

public static bool IsBinaryFormatterSupported => IsNotMobile && !IsNativeAot;

So I'll not make this change now, but I do think it would good to separate that state to avoid accidental regression. Make it clear in the test source when you expect to have an actual implementation vs not.

@adamsitnik
Copy link
Member

I do think it would good to separate that state to avoid accidental regression.

I agree. I have an idea how it should look like with your changes, let me take a quick stab at it.

@adamsitnik
Copy link
Member

#106858 I have tested it locally for Windows and it works as expected. I don't have more time today because I've just logged it to take a day off, will get back to it on Monday (if further changes are required).

adamsitnik pushed a commit to adamsitnik/runtime that referenced this pull request Sep 9, 2024
* Remove package references from library tests

These tests should be referencing the product assemblies so that they
test latest and not old bits.

* Reference the OOB version of SRSF and make sure it's copied
github-actions bot pushed a commit that referenced this pull request Sep 17, 2024
* Remove package references from library tests

These tests should be referencing the product assemblies so that they
test latest and not old bits.

* Reference the OOB version of SRSF and make sure it's copied
carlossanlop pushed a commit that referenced this pull request Sep 17, 2024
* Remove package references from library tests (#106737)

* Remove package references from library tests

These tests should be referencing the product assemblies so that they
test latest and not old bits.

* Reference the OOB version of SRSF and make sure it's copied

* BinaryFormatter tests should be skipped only on AOT, WASM and Mobile (#106858)

* respect AppContext switch (which is currently enabled for all projects in the root Directory.Build.props file)

* add project reference to all test projects that need working BF (and were being skipped for a while)

* adjust to changes from #104202: EqualityComparer<string>.Default is mapped to StringEqualityComparer, but serialized as GenericEqualityComparer<string>

* Don't use WeakReferences in the round trip test, as the target may get freed in the meantime, fixes #104905 (#106967)

* Enable more BinaryFormatter tests (#107408)

* enable the BinaryFormatter tests in System.Runtime.Serialization.Formatters.Tests

* add new test project, where the flag is disabled and it runs only 3 tests in total that ensure that

* The SerializationGuard is no longer activated since BF was moved to the OOB package, the tests need to reflect that.

* Disable binary formatter tests when DotNetBuildSourceOnly. (#107549)

* [mono][tvos] Do not treat assembly.pdb/xml files as native files to bundle when AOTing on Helix (#107079)

* Do not treat assembly.pdb/xml files as native files to bundle

* Bundle satellite assemblies as well

* [mono][ci] Include PDBs from runtime pack when building on Helix if required (#107348)

---------

Co-authored-by: Eric StJohn <ericstj@microsoft.com>
Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
Co-authored-by: Ivan Povazan <55002338+ivanpovazan@users.noreply.github.com>
carlossanlop pushed a commit that referenced this pull request Sep 17, 2024
…107903)

* Remove package references from library tests (#106737)

* Remove package references from library tests

These tests should be referencing the product assemblies so that they
test latest and not old bits.

* Reference the OOB version of SRSF and make sure it's copied

* BinaryFormatter tests should be skipped only on AOT, WASM and Mobile (#106858)

* respect AppContext switch (which is currently enabled for all projects in the root Directory.Build.props file)

* add project reference to all test projects that need working BF (and were being skipped for a while)

* adjust to changes from #104202: EqualityComparer<string>.Default is mapped to StringEqualityComparer, but serialized as GenericEqualityComparer<string>

* Don't use WeakReferences in the round trip test, as the target may get freed in the meantime, fixes #104905 (#106967)

* Enable more BinaryFormatter tests (#107408)

* enable the BinaryFormatter tests in System.Runtime.Serialization.Formatters.Tests

* add new test project, where the flag is disabled and it runs only 3 tests in total that ensure that

* The SerializationGuard is no longer activated since BF was moved to the OOB package, the tests need to reflect that.

* Disable binary formatter tests when DotNetBuildSourceOnly. (#107549)

* [mono][tvos] Do not treat assembly.pdb/xml files as native files to bundle when AOTing on Helix (#107079)

* Do not treat assembly.pdb/xml files as native files to bundle

* Bundle satellite assemblies as well

* [mono][ci] Include PDBs from runtime pack when building on Helix if required (#107348)

---------

Co-authored-by: Eric StJohn <ericstj@microsoft.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
Co-authored-by: Ivan Povazan <55002338+ivanpovazan@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants