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

Preserve necessary type and method #54932

Merged
merged 5 commits into from
Jul 1, 2021

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Jun 29, 2021

Fixes #50721
For test System.Text.Json.Serialization.Tests.CollectionTests.WriteHashSetTOfHashSetT,
the line of code which triggered System.NullReferenceException was https://github.com/xunit/assert.xunit/blob/3e9f13f2fae7ff8f379da0f19f72d56aea2dae46/Sdk/AssertEqualityComparer.cs#L249
Thus, CompareTypedSets was the missing method. It is a method of type Xunit.Sdk.AssertEqualityComparer, which belongs to assembly xunit.assert.dll

Fixes #50717
Rewrite the way of getting the type System.Runtime.InteropServices.IDispatchImplAttribute.

Fixes #53647
Fix for #50721 also fixed this.

Fixes #51911
Simply enable the tests. Not reproducible anymore.

@ghost
Copy link

ghost commented Jun 29, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #50721 and #50717

Author: fanyang-mono
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@radical radical added the arch-wasm WebAssembly architecture label Jun 30, 2021
@ghost
Copy link

ghost commented Jun 30, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #50721 and #50717

Author: fanyang-mono
Assignees: -
Labels:

arch-wasm, area-System.Text.Json

Milestone: -

@radical radical added the trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT label Jun 30, 2021
@lewing lewing changed the title [WASM]Preserve necessary type and method [mono] Preserve necessary type and method Jun 30, 2021
@lewing lewing changed the title [mono] Preserve necessary type and method Preserve necessary type and method Jun 30, 2021
@lewing
Copy link
Member

lewing commented Jun 30, 2021

cc @eerhardt

@radical
Copy link
Member

radical commented Jun 30, 2021

Could you add the reasoning for these changes, like what depends on it, and how/what fails?

@lewing lewing added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 30, 2021
@ghost
Copy link

ghost commented Jun 30, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #50721 and #50717

Author: fanyang-mono
Assignees: -
Labels:

arch-wasm, area-System.Text.Json, linkable-framework, trimming-for-aot

Milestone: -

@eerhardt
Copy link
Member

    private const string TypeName = "System.Runtime.InteropServices.IDispatchImplAttribute";

Is TypeName still used?


Refers to: src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/IDispatchImplAttributeTests.cs:11 in e67171c. [](commit_id = e67171c, deletion_comment = False)

@MichalStrehovsky
Copy link
Member

Is TypeName still used?

Might be worth inlining ValueName as well. It feels like something that fell victim to "introduce a constant for the sake of introducing a constant". It's detrimental to readability.

@fanyang-mono
Copy link
Member Author

Failures of Libraries Test Run release coreclr Linux_musl x64 Debug are unrelated - #54778
Failures of Build Android x64 Release AllSubsets_Mono_RuntimeTests are unrelated - dotnet/xharness#660
Failures of Mono llvmaot Pri0 Runtime Tests Run Linux x64 release are unrelated as well...

@fanyang-mono
Copy link
Member Author

@eerhardt and @MichalStrehovsky Are the changes looks okay with you?

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Jul 1, 2021

All tests have passed actually. Ready to be merged.

@fanyang-mono fanyang-mono merged commit a291fab into dotnet:main Jul 1, 2021
@stephentoub
Copy link
Member

@fanyang-mono, this test is still disabled against one of the issues this closes:

[ActiveIssue("https://github.com/dotnet/runtime/issues/53647", TestPlatforms.Browser)]

stephentoub added a commit that referenced this pull request Jul 1, 2021
This is reported to have been fixed by #54932.
stephentoub added a commit that referenced this pull request Jul 1, 2021
This is reported to have been fixed by #54932.
@fanyang-mono
Copy link
Member Author

@fanyang-mono, this test is still disabled against one of the issues this closes:

[ActiveIssue("https://github.com/dotnet/runtime/issues/53647", TestPlatforms.Browser)]

Oops, I didn't notice that there were more than one test methods associated with that issue. Will Open another PR to enable it. Thanks for your reminder.

@stephentoub
Copy link
Member

Already did :-)

@fanyang-mono
Copy link
Member Author

Already did :-)

Thank you :)

@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Text.Json linkable-framework Issues associated with delivering a linker friendly framework trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT
Projects
None yet
6 participants