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

Pick up a new build of Xunit #60044

Closed
wants to merge 6 commits into from
Closed

Pick up a new build of Xunit #60044

wants to merge 6 commits into from

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Oct 6, 2021

This is the latest pre-release of Xunit 2.x. It contains updated asserts library and manually updated to the latest analyzers.

The analyzers found some new issues with our tests, which I have fixed. There are a few issues I worked around.

xunit/xunit#2393
xunit/xunit#2394
xunit/xunit#2395

Reviewable commit-by-commit.

ericstj added 5 commits October 5, 2021 08:06
@ericstj
Update to Xunit build 2.4.2-pre.13
4367e42 
Also pick up latest pre-release of analyzers
@ericstj
Disambiguate calls to Assert.Equals(double,double,int)
9f1938d 
Xunit added a new Assert overload that caused a lot of ambiguous calls.
xunit/xunit#2393

Workaround by casting to double.
@ericstj
Fix new instances of xUnit2000 diagnostic
7637493
@ericstj
Workaround xUnit2002 issue with implicit cast
a0b9485 
Works around xunit/xunit#2395
@ericstj
Disable xUnit2014 diagnostic
63faf78 
This diagnostic forces the use of Assert.ThrowsAsync for any async method,
however in our case we may want to test that a method will throw
synchronously to avoid regressing that behavior by moving to the async
portion of the method.
@ghost
Copy link

ghost commented Oct 6, 2021

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

Issue Details

This is the latest pre-release of Xunit 2.x. It contains updated asserts library and manually updated to the latest analyzers.

The analyzers found some new issues with our tests, which I have fixed. There are a few issues I worked around.

xunit/xunit#2393
xunit/xunit#2394
xunit/xunit#2395

Reviewable commit-by-commit.

Author: ericstj
Assignees: -
Labels:

area-Infrastructure-libraries
Milestone: -

@ericstj ericstj requested review from ViktorHofer and a team October 6, 2021 00:05
safern
safern approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM

@ericstj
Copy link
Member Author

ericstj commented Oct 6, 2021

Looks like there was a breaking change in Assert.Throws<ArgumentException>. xunit/xunit#2396

Will need to see if we can workaround this or not.

@ericstj ericstj requested a review from maryamariyan October 7, 2021 16:14
@ericstj
Copy link
Member Author

ericstj commented Oct 7, 2021

I would appreciate a second opinion on disabling xUnit2014 and my justification:

This diagnostic forces the use of Assert.ThrowsAsync for any async method,
however in our case we may want to test that a method will throw
synchronously to avoid regressing that behavior by moving to the async
portion of the method.

I know we were very careful about this in the past, particularly with parameter validation. I considered making a new method like AssertExtensions.ThrowsSync<T>(Func<Task>) that just calls Assert.Throws<T> and suppresses the diagnostic, but it felt like using Assert.Throws was better, since that's naturally what folks will call and it will coincidentally catch the behavior when we throw synchronously and force folks to notice when we were to ever move that to the async portion of the method. @stephentoub

ViktorHofer
ViktorHofer approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

LGTM. Reminds me of when we had to do this back when v2 was actively developed.

@runfoapp runfoapp bot mentioned this pull request Oct 7, 2021
Closed
@ericstj
Copy link
Member Author

ericstj commented Oct 11, 2021

Latest round of failures appear to be due to re-enumeration of an IEnumerable in assert library. Need to track down that regression.

@ericstj
Copy link
Member Author

ericstj commented Oct 15, 2021

Ok, that's this issue: xunit/xunit#2402

Not seeing an easy workaround at the moment. I'll see if we can make a fix to xunit.

@ericstj ericstj self-assigned this Nov 15, 2021
@ericstj
Copy link
Member Author

ericstj commented Nov 15, 2021

Closing this for now, Will need to merge a fix and pick up a new build to proceed.

@ericstj ericstj closed this Nov 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern approved these changes

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@danmoseley danmoseley danmoseley approved these changes

@maryamariyan maryamariyan maryamariyan approved these changes

Assignees

@ericstj ericstj

Labels
area-Infrastructure-libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ericstj @maryamariyan @danmoseley @ViktorHofer @safern