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

Use the Xunit assert library instead of CoreCLRTestLibrary for most asserts #61226

Merged
merged 5 commits into from
Nov 6, 2021

Conversation

jkoritzinsky
Copy link
Member

Asserts not provided by Xunit are provided by an AssertExtensions class.

Tests that reference System.Private.Corelib directly will use a polyfill implementation based off the old CoreCLRTestLibrary asserts.

All assert methods provided by CoreCLRTestLibrary have been changed to follow Xunit conventions (this is why the change is so noisy).

This contribues to our move towards more xunit-style tests in the src/tests tree, just with a custom runner (as part of the test merging initiative).

cc: @trylek for review

cc: @elinor-fung and @AaronRobinsonMSFT as the Interop team owns or has worked on most of the tests that are modified by this change.

…sserts.

Asserts not provided by Xunit are provided by an AssertExtensions class.

Tests that reference System.Private.Corelib directly will use a polyfill implementation based off the old CoreCLRTestLibrary asserts.

All assert methods provided by CoreCLRTestLibrary have been changed to follow Xunit conventions.
@ghost
Copy link

ghost commented Nov 4, 2021

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

Issue Details

Asserts not provided by Xunit are provided by an AssertExtensions class.

Tests that reference System.Private.Corelib directly will use a polyfill implementation based off the old CoreCLRTestLibrary asserts.

All assert methods provided by CoreCLRTestLibrary have been changed to follow Xunit conventions (this is why the change is so noisy).

This contribues to our move towards more xunit-style tests in the src/tests tree, just with a custom runner (as part of the test merging initiative).

cc: @trylek for review

cc: @elinor-fung and @AaronRobinsonMSFT as the Interop team owns or has worked on most of the tests that are modified by this change.

Author: jkoritzinsky
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

src/tests/Common/Assert.cs Outdated Show resolved Hide resolved
src/tests/Common/Assert.cs Outdated Show resolved Hide resolved
jkoritzinsky and others added 2 commits November 4, 2021 15:33
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

src/tests/Interop/ReadMe.md Outdated Show resolved Hide resolved
CollectionEqual ended up in infinite recursion as the array variant
no longer accepts the third string argument (there's no use for it
as Assert.Equal no longer supports it; after all, a single test was
using it) so that the enumerable variant ended up calling itself
instead of the array variant. I have removed the message string
and I deleted it from the one ResolveUnmanagedDllTests source.

In ThrowsWithInnerException there was a typo, there should be
"is TInner" in the inner exception check, not "is T".

Thanks

Tomas
@trylek
Copy link
Member

trylek commented Nov 6, 2021

The bugs are unrelated and known (#61237, #60152), merging in.

@trylek trylek merged commit 544532a into dotnet:main Nov 6, 2021
@jkoritzinsky
Copy link
Member Author

@tylek I think there's a conflicting change that got merged in before this one. Can you check the Pri0 test build and make sure it isn't broken? (I'm not near my computer otherwise I'd do it myself)

@jkoritzinsky jkoritzinsky deleted the xunit-asserts-src-tests branch November 6, 2021 21:14
@EgorBo
Copy link
Member

EgorBo commented Nov 6, 2021

Is failure like this https://dev.azure.com/dnceng/public/_build/results?buildId=1458028&view=logs&jobId=e2abd64e-6d96-553c-c262-deb1bd067c99&j=e2abd64e-6d96-553c-c262-deb1bd067c99&t=1849d1b9-84d9-551e-f982-c77c09278369 related?
CoreCLR Common Pri0 Test Build AnyOS AnyCPU checked failed

WeakReferenceTest.cs(260,20): error CS0117: 'Assert' does not contain a definition for 'IsNull' 

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2021
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.

5 participants