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

Add assert.xunit subtree and make it AOT-safe #83933

Closed
wants to merge 6 commits into from

Conversation

agocke
Copy link
Member

@agocke agocke commented Mar 26, 2023

Adds a subtree from https://github.com/xunit/assert.xunit and modifies it to make it AOT-safe. This assert clone is used in the libraries tests for AOT-compatibility.

Recommended to review commit-by-commit

Contributes to #79316

@ghost ghost assigned agocke Mar 26, 2023
@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 Mar 26, 2023
@agocke agocke added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 26, 2023
@ghost
Copy link

ghost commented Mar 26, 2023

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

Issue Details

Adds a subtree from https://github.com/xunit/assert.xunit and modifies it to make it AOT-safe. This assert clone is used in the libraries tests for AOT-compatibility.

Recommended to review commit-by-commit

Contributes to #79316

Author: agocke
Assignees: agocke
Labels:

NO-MERGE, area-Infrastructure-libraries, needs-area-label

Milestone: -

@ghost
Copy link

ghost commented Mar 26, 2023

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

Issue Details

Adds a subtree from https://github.com/xunit/assert.xunit and modifies it to make it AOT-safe. This assert clone is used in the libraries tests for AOT-compatibility.

Recommended to review commit-by-commit

Contributes to #79316

Author: agocke
Assignees: agocke
Labels:

NO-MERGE, area-Infrastructure, needs-area-label

Milestone: -

@agocke agocke removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 26, 2023
@MartyIX
Copy link
Contributor

MartyIX commented Mar 26, 2023

Is it planned to backport useful changes upstream, please?

@ViktorHofer
Copy link
Member

Wouldn't a better place for this code base be arcade which already hosts other parts of our xunit story, i.e. the forked Microsoft.DotNet.XUnitConsoleRunner and the Microsoft.DotNet.XUnitExtensions? Asking as other repositories in our stack would benefit from an AOT friendly xunit.assert library as well.

@agocke
Copy link
Member Author

agocke commented Mar 27, 2023

Wouldn't a better place for this code base be arcade which already hosts other parts of our xunit story, i.e. the forked Microsoft.DotNet.XUnitConsoleRunner and the Microsoft.DotNet.XUnitExtensions? Asking as other repositories in our stack would benefit from an AOT friendly xunit.assert library as well.

I did actually anticipate putting this up to arcade eventually. My plan was to flesh this out in the runtime repo, then move it up once the concept had proven itself, but I'm now seeing that there are arcade packages (RemoteExecutor) that depend on xunit.assert, so I might as well start there now if I'm going to be changing Arcade anyway.

@MartyIX Is it planned to backport useful changes upstream, please?

Unfortunately this will come with some breaking changes. In particular, the usages of MakeGenericType won't work. I'm still open to upstreaming whatever's possible or reasonable, but I'm not sure this would be appropriate to fold directly into XUnit. Perhaps a separate set of AOT-specific packages instead.

@jkotas
Copy link
Member

jkotas commented Mar 27, 2023

I'm still open to upstreaming whatever's possible or reasonable

Use pre-processor directive to isolate the behavior changes? xunit has number of them today: https://github.com/dotnet/runtime/pull/83933/files#diff-a1e598da48ce193e937e5508ae771cea4bf83160386ee00ce65fbd305ddd3039R11-R37

@agocke
Copy link
Member Author

agocke commented Mar 28, 2023

I'm actually considering a fairly substantial rework. One of the reasons XUnit has to use so much reflection is that they define

Assert.Equal<T>(T left, T right);

In overload resolution, that's preferred to almost any other overload. But an unconstrained generic has almost no information, so reflection is used to find and handle corner cases.

However, if we adjust the overloads, we could have

Assert.Equal<T>(T left, T right) where T : IEquatable<T>;
Assert.Equal<T>(IDictionary<T> left, IDictionary<T> right) where T : IEquatable<T>;
Assert.Equal<T>(IEnumerable<T> left, IEnumerable<T> right) where T : IEquatable<T>;
...
Assert.Equal(object left, object right);

Now, overload resolution should be able to actually help us here by picking the "most appropriate" overload.

However, this is a big change -- possibly too big to do as just an if-def.

@agocke
Copy link
Member Author

agocke commented Mar 28, 2023

Closing this to re-open in arcade.

@agocke agocke closed this Mar 28, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants