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

Assertions for Task and Task<T> with CompleteWithin checks #1048

Merged
merged 1 commit into from May 28, 2019

Conversation

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented May 25, 2019

Replaces #1013 by applying some more refactoring on #1013.

Thanks to @lg2de for the original implementation.

@lg2de
Copy link
Contributor

@lg2de lg2de commented May 27, 2019

Today I looked deeper in your PR. In addition to my review comments below, please note the following:

  • You named the PR similar to #1013 but you implemented it similar to #1011.
  • You closed #1013 as replaced. But my intention there was the extending Task directly. You extended Func<Task>.

Copy link
Contributor

@lg2de lg2de left a comment

I'm not in the position to "request" changes, but I recommend at least thinking about...


namespace FluentAssertions.Specs
{
public class TaskAssertionSpecs

This comment has been minimized.

@lg2de

lg2de May 27, 2019
Contributor

You are testing here Func<Task> not Task.
So this tests should be placed in the file for the other Func tests.

This comment has been minimized.

@dennisdoomen

dennisdoomen May 27, 2019
Author Member

Which one do you mean?

This comment has been minimized.

@lg2de

lg2de May 28, 2019
Contributor

AsyncFunctionExceptionAssertionSpecs.cs

This comment has been minimized.

@dennisdoomen

dennisdoomen May 28, 2019
Author Member

I suspected that one, but that one acts on exceptions, not tasks.

This comment has been minimized.

@lg2de

lg2de May 28, 2019
Contributor

Then I recommend AsyncMethodAssertionSpecs.cs here and AsyncFunctionAssertionSpecs.cs below.
Or - following your implementation classes GenericAsyncFunctionAssertionsSpecs.cs and NonGenericAsyncFunctionAssertionsSpecs.cs.

BTW: I myself prefer positive logic. This is why I do not that like "NonGeneric".


namespace FluentAssertions.Specs
{
public class TaskOfTAssertionSpecs

This comment has been minimized.

@lg2de

lg2de May 27, 2019
Contributor

So above!

introduce ITimer to abstract real timing in the tests

rework after review

rework after review

Another pass of review.
@dennisdoomen dennisdoomen force-pushed the dennisdoomen:Rework1013 branch from eae4a19 to d546396 May 27, 2019
@dennisdoomen
Copy link
Member Author

@dennisdoomen dennisdoomen commented May 27, 2019

I'm not in the position to "request" changes, but I recommend at least thinking about...

Of course you are. And you were even right.

  • You named the PR similar to #1013 but you implemented it similar to #1011.
  • You closed #1013 as replaced. But my intention there was the extending Task directly. You extended Func<Task>.

I took your PR and implemented it according to the review comment @jnyrup made here and with which I agreed.

@dennisdoomen
Copy link
Member Author

@dennisdoomen dennisdoomen commented May 27, 2019

@dennisdoomen dennisdoomen merged commit 4009f5e into fluentassertions:master May 28, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@dennisdoomen dennisdoomen deleted the dennisdoomen:Rework1013 branch May 28, 2019
/// <param name="becauseArgs">
/// Zero or more objects to format using the placeholders in <see cref="because" />.
/// </param>
public AndWhichConstraint<GenericAsyncFunctionAssertions<TResult>, Task<TResult>> CompleteWithin(

This comment has been minimized.

@jnyrup

jnyrup May 28, 2019
Collaborator

Could we return TResult instead of Task<TResult>?
That would remove the need in tests to call .Result.

@dennisdoomen
Copy link
Member Author

@dennisdoomen dennisdoomen commented May 28, 2019

Crap. I was too fast.

/// <param name="becauseArgs">
/// Zero or more objects to format using the placeholders in <see cref="because" />.
/// </param>
public AndWhichConstraint<AsyncFunctionAssertions, Task> CompleteWithin(

This comment has been minimized.

@jnyrup

jnyrup Jun 7, 2019
Collaborator

If the other CompleteWithin is changed to
AndWhichConstraint<GenericAsyncFunctionAssertions<TResult>, TResult>
then this should be changed to
AndConstraint<AsyncFunctionAssertions<TResult>>

This comment has been minimized.

@dennisdoomen

dennisdoomen Jun 7, 2019
Author Member

I don't get it. They both return AndWhichConstraint. One just wraps it in Task.

@jnyrup
Copy link
Collaborator

@jnyrup jnyrup commented Jun 7, 2019

I was thinking about using AndWhichConstraint<...,TResult> when the subject is e.g. Func<Task>
and AndConstraint<...> when the Subject is e.g. Func.

I'm vacationing until Monday without a computer, so my responses might be delayed and inferior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants