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

[release/9.0] BinaryFormatter tests improvements #107540

Merged
merged 7 commits into from
Sep 17, 2024
Merged

[release/9.0] BinaryFormatter tests improvements #107540

merged 7 commits into from
Sep 17, 2024

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Sep 9, 2024 •
edited
Loading

With what we have right now in release/9.0, most of the BinaryFormatter tests are skipped.

Backporting the following PRs is going to fix that:

ericstj and others added 4 commits September 9, 2024 15:39
@ericstj @adamsitnik
Remove package references from library tests (dotnet#106737)
8ecb9d4 
* 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
@adamsitnik
BinaryFormatter tests should be skipped only on AOT, WASM and Mobile (d…
2565dd7 
…otnet#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 dotnet#104202: EqualityComparer<string>.Default is mapped to StringEqualityComparer, but serialized as GenericEqualityComparer<string>
@adamsitnik
Don't use WeakReferences in the round trip test, as the target may ge…
044d6a0 
…t freed in the meantime, fixes dotnet#104905 (dotnet#106967)
@adamsitnik
Enable more BinaryFormatter tests (dotnet#107408)
c0b7ded 
* 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.
@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
@ericstj
Copy link
Member

ericstj commented Sep 9, 2024

I agree we want better coverage of these in 9.0. If the fixes are test/infra only then we can treat as tell mode.

I am going to mark it as a DRAFT because #107534 tells me that we need to fix it first.

Yeah, that's my biggest concern here - is that we batch up all the related test or infra changes necessary. Once ready, make sure you run the optional pipelines that broke before.

@teo-tsirpanis teo-tsirpanis added area-Infrastructure-libraries test-enhancement Improvements of test source code and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 9, 2024
@teo-tsirpanis teo-tsirpanis added this to the 9.0.0 milestone Sep 9, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@adamsitnik
Copy link
Member Author

/azp list

@azure-pipelines Azure Pipelines
Copy link

CI/CD Pipelines for this repository:

This was referenced Sep 11, 2024
Open
Open
Open
Open
@adamsitnik adamsitnik marked this pull request as ready for review September 16, 2024 16:42
@ericstj ericstj mentioned this pull request Sep 16, 2024
Closed
@carlossanlop carlossanlop added the Servicing-approved Approved for servicing release label Sep 16, 2024
carlossanlop
carlossanlop approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Approving as tell-mode.

@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 16, 2024
@carlossanlop
Copy link
Member

@adamsitnik unfortunately the CI just finished with failures in tvos and ios arm64 that seem related to this change:

/private/tmp/helix/working/AAB50990/w/B50D0974/e/publish/ProxyProjectForAOTOnHelix.proj(116,5): error MSB3030: Could not copy the file "/tmp/helix/working/AAB50990/p/build/microsoft.netcore.app.runtime.tvos-arm64/runtimes/tvos-arm64/native/System.Runtime.Serialization.Formatters.xml" because it was not found.

Can you please address the failure and retarget this PR to the release/9.0-rc2 branch? We can still treat this as tell-mode when retargeted (no Tactics involvement needed).

carlossanlop
carlossanlop requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Related CI failure.

@ericstj
Copy link
Member

ericstj commented Sep 17, 2024

We need
#107079
#107348

cc @ivanpovazan

I will add those to this PR.

@ivanpovazan @ericstj
[mono][tvos] Do not treat assembly.pdb/xml files as native files to b…
d527ce1 
…undle when AOTing on Helix (dotnet#107079)

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

* Bundle satellite assemblies as well
@carlossanlop carlossanlop removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 17, 2024
@carlossanlop
Copy link
Member

/backport to release/9.0-rc2

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/9.0-rc2: https://github.com/dotnet/runtime/actions/runs/10894273351

@github-actions github-actions bot mentioned this pull request Sep 17, 2024
Merged
@adamsitnik
Copy link
Member Author

Related CI failure.

Great catch, apologies for missing it.

I will add those to this PR.

Thanks a lot @ericstj !

ivanpovazan
ivanpovazan approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

LGTM!

Maybe just for reference included the needed mono CI changes in the PR description for clarity (so it is not lost in the comments)

carlossanlop
carlossanlop approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks @ericstj ! :shipit:

@carlossanlop carlossanlop merged commit 34b0d9d into dotnet:release/9.0 Sep 17, 2024
160 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ivanpovazan ivanpovazan ivanpovazan approved these changes

@ericstj ericstj ericstj approved these changes

@carlossanlop carlossanlop carlossanlop approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@adamsitnik adamsitnik

Labels
area-Infrastructure-libraries Servicing-approved Approved for servicing release test-enhancement Improvements of test source code
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@adamsitnik @ericstj @carlossanlop @ivanpovazan @teo-tsirpanis @tmds