-
Notifications
You must be signed in to change notification settings - Fork 731
add extension to assert TaskCompletionSource #1267
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
Conversation
* added new TaskCompletionSourceAssertions * clearified the usage of the FakeClock
Src/FluentAssertions/Specialized/TaskCompletionSourceAssertions.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Specialized/TaskCompletionSourceAssertions.cs
Outdated
Show resolved
Hide resolved
dennisdoomen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and also please update the https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md and maybe add a new specialized.md to document the new assertions under https://github.com/fluentassertions/fluentassertions/tree/develop/docs/_pages
In this file all assertions from |
Well, if you add yours, we'll take it from there. |
* set c# language version to 8.0 * add documentation for new feature * code cleanup
|
Anything still missing here? |
jnyrup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I wanted check out the branch and fiddle a bit around with it as I'm not that familiar with using TaskCompletionSource.
Tests/Shared.Specs/Specialized/TaskCompletionSourceAssertionSpecs.cs
Outdated
Show resolved
Hide resolved
Tests/Shared.Specs/Specialized/TaskCompletionSourceAssertionSpecs.cs
Outdated
Show resolved
Hide resolved
|
Another gem by @lg2de 👌 |
|
@lg2de Forgot to complement you for the the thorough analysis of the consequences of introducing the new |
This is a new extension to create assertions on
TaskCompletionSource<T>(TCS).This PR creates a breaking change, because for instances of TCS the assertions of the currently catching
ObjectAssertionsextension will not be available anymore.But FA is going to version 6.0, so it should be acceptable.
Further, I tried to analyze the assertions which may get lost. Here is my answer to the question "Do we need such assertion?":
ObjectAssertionsBe/NotBe: No, why should we check equivalency?BeEquivalentTo/NotBeEquivalentTo: No, why should we check equivalency?ReferenceTypeAssertionsBeNull/NotBeNull: Unsure, I do not see any relevance.BeSameAs/NotBeSameAs: Unsure, could be relevant if TCS is part of an API.BeOfType/NotBeOfType: No, I do not see any relevance.BeAssignableTo/NotBeAssignableTo: No, I do not see any relevance.Match: No, I do not see any relevance.Please help to answer these questions and check whether you agree.
While creating unit tests using the clock API I was a bit confused whether the methods
CompletesBeforeTimeoutandRunsIntoTimeoutinfluences the behavior ofDelayAsync. I found, it did not. So I introduced new methods and changed existing usages. Hope you agree.If not, we should think, whether
DelayAsyncor even more theIClockAPI can be adopted to work similar toFakeClock.Wait. MaybeDelayAsyncshould be replaced by aWaitAsyncreturningbooleantoo.