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

Fix System.BinaryData tests failing in AOT. #89381

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

eiriktsarpalis
Copy link
Member

Fixes test failures reported in #88982 (comment) that have been caused by #88480.

The failing tests could have easily been converted to source gen, however this is currently blocked by #81702. Given that this is blocking use of BinaryData in source gen we should consider an RC1 fix (which requires change in public API) cc @eerhardt @ericstj

@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 Jul 24, 2023
@ghost ghost assigned eiriktsarpalis Jul 24, 2023
@ghost
Copy link

ghost commented Jul 24, 2023

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

Issue Details

Fixes test failures reported in #88982 (comment) that have been caused by #88480.

The failing tests could have easily been converted to source gen, however this is currently blocked by #81702. Given that this is blocking use of BinaryData in source gen we should consider an RC1 fix (which requires change in public API) cc @eerhardt @ericstj

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Memory, needs-area-label

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jul 24, 2023
@eiriktsarpalis eiriktsarpalis removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 24, 2023
@@ -2,6 +2,7 @@

<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent);$(NetFrameworkMinimum)</TargetFrameworks>
<JsonSerializerIsReflectionEnabledByDefault>true</JsonSerializerIsReflectionEnabledByDefault>
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add a comment here saying "Convert tests to source gen when #81702 is fixed"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll follow up with a new PR addressing this now, following the recommendation in #89381 (comment)

@ericstj
Copy link
Member

ericstj commented Jul 24, 2023

IMO we should just fast-track the API change. Moving from internal to public when the type merely implements an abstract class should be something we review fast (over email?) - only thing to review is the name and namespace.

@eiriktsarpalis eiriktsarpalis merged commit 8f53646 into dotnet:main Jul 25, 2023
100 of 103 checks passed
@eiriktsarpalis eiriktsarpalis deleted the fix/binarydata-tests branch July 25, 2023 11:37
@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2023
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.

3 participants