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

Assertion that asynchronous operation completes within time span #1001

Closed
lg2de opened this issue Feb 28, 2019 · 15 comments
Closed

Assertion that asynchronous operation completes within time span #1001

lg2de opened this issue Feb 28, 2019 · 15 comments

Comments

@lg2de
Copy link
Contributor

lg2de commented Feb 28, 2019

Testing asynchronous operations is currently available like this:

this.myServer.Invoking(s => s.OpenAsync).Should().NotThrow();

I'm missing the option just check, that the operation will complete (and not throw) within specific time range. Therefore a new assertion could be created:

this.myServer.Invoking(s => s.OpenAsync).Should().Complete(TimeSpan.FromMilliseconds(100));

Such an idea I already mentioned in #990.

While thinking about the implementation I found a different approach.
Why to I need the indirection using "Invoking"? Why not asserting the Task directly:

this.myServer.OpenAsync().Should().Complete(TimeSpan.FromMilliseconds(100));
@bitbonk
Copy link

bitbonk commented Mar 12, 2019

I have come across this problem too. Sometimes those asynchronous operations that are being tested may never complete or may take an unusual long time to complete. As a result, the whole test run takes forever to complete.

Sometimes I just want a test to fail if the asynchronous operation does not complete within a given amount of time. In most cases, there actually is something wrong if a task takes forever to complete.

I would love to see assertions that help me to express the following:

  1. The task should complete within a certain time without throwing an exception.
    this.myServer.Invoking(s => s.GetCustomerAsync).Should().CompleteWithin(TimeSpan.FromSeconds(10);

  2. The task should complete within a certain time without throwing an exception and the returned TResult should meet some expectations.

    this.myServer.Invoking(s => s.GetCustomerAsync).Should()
     .CompleteWithin(TimeSpan.FromSeconds(10)
     .AndResult().Should().BeEquivalentTo(new { Name = "Kurt Russel" });
    
  3. The task should throw an exception within a certain time.
    this.myServer.Invoking(s => s.GetCustomerAsync).Should.ThrowWithin(TimeSpan.FromSeconds(10);

@bitbonk
Copy link

bitbonk commented Mar 12, 2019

One more thing: What should happen to those tasks that did not complete in case the assertions failed? Is it a problem if tasks stay alive after the test is done? If there is a CancellationToken, can FA take care of cancelling those tasks?

@lg2de
Copy link
Contributor Author

lg2de commented Mar 12, 2019

So, @bitbonk, you prefer following the Invoking pattern?
Thinking about again, the timing may be more exact than working on the Task directly.
But, the approach without Invoking would be shorter.

Regarding the CancellationToken I do not see an option for FA.
But using e.g. xUnit the testing framework can take care, isn't it?

@dennisdoomen
Copy link
Member

If there is a CancellationToken, can FA take care of cancelling those tasks?

The CompleteWithin could take the cancellation token as a parameter, but it won't be very intuitive.

@lg2de
Copy link
Contributor Author

lg2de commented Mar 12, 2019

@dennisdoomen, which approach you prefer? Based on Invoking or directly on the Task?

@dennisdoomen
Copy link
Member

Both ;-)

@lg2de
Copy link
Contributor Author

lg2de commented Mar 12, 2019

Both ;-)

äähhmmm, oooh...
I'll try to do my best (incl. #990, may be).

@ArndtMichael
Copy link

I would prefer handling with the Task directly because:
The assertion is related to exactly that task and not that one Invoking returns. It's more clear what happens.

@lg2de
Copy link
Contributor Author

lg2de commented Sep 13, 2019

Hi @dennisdoomen and @jnyrup, you remember my attempt to contribute this extension?

Today we have simlar extension with the Invoking feature. Fine, so far.
In my team will still have a private extension for the assertion directly on Task. I would like to discuss again with you, whether and how to get this in the official FA.

We like the very short version I mentioned initially:

this.myServer.OpenAsync().Should().CompleteWithin(100.MilliSeconds());

You did not want to introduce this because it is an overlay of the ObjectAssertions.
With this approach some methods will be unavailable. Lets have a look in detail:

Methods Comment
(Not)Be How to assert equality of tasks?
(Not)BeEquivalentTo How to assert similarity of tasks?
HaveFlag Tasks are not enums.
(Not)BeNull Using asynchronous programming you should never have null Tasks. This should be checked implicitly.
(Not)BeSameAs Why should I ever expect that two Tasks are the same instance?
(Not)BeOfType/BeAssignableTo Using asynchronous programming you should only use Task and Task<T>, isn't it?
Match This could be a useful method which we could add.

So, I can identify only a single method which can be used used reasonable on Tasks in the current API. This one can be added to be compatible.
Additionally I would be possible to inherit from ObjectAssertions or ReferenceTypeAssertions.

Further you mentioned the timing may be inaccurate because the task is already running when check timer is started.
For me this is not an issue. This extension is intended as timeout for a single operation without creating CancellationTokenSource every time. It should just ensure that e.g. complex tests (mainly integration tests) will fail early with clear information on blocking step.

What do you think today? Additional questions to discuss?

@dennisdoomen
Copy link
Member

If I recall correctly, the main problem was the discussion around Task vs Func<Task>, right?

@lg2de
Copy link
Contributor Author

lg2de commented Sep 13, 2019

Func<Task> is already implemented.
In addition I would like to have Task.

@jnyrup
Copy link
Member

jnyrup commented Sep 14, 2019

The timing accuracy is a minor problem of assertion directly on Task.
A bigger problem with asserting directly on Task is that not every assertion available on Func<Task> makes sense for Task.
E.g. NotThrow() as myTask may throw before we get a change to await it.

myTask.Should().NotThrow();

While we could leave the exception testing assertions out of the API for Task, that would (in my opinion) give a fragmented API where most assertions are available on Task but some only on Func<Task>.
I don't doubt that the people commenting in this thread are experienced enough to know when to use which type of assertion, but I think it would make it harder for newcomers.

@dennisdoomen
Copy link
Member

If we support it, we would have to do it in a consistent way. So every assertion that you can run on a Func<Task> should also become available on Task.

@bitbonk
Copy link

bitbonk commented Sep 23, 2019

To be honest I don't really see a problem if the assertions available on Task differ from the ones available on Func<Task>. It is not that a novice user would be able to use assertions that do not work. It is just that some are just not available. Novice users are still guided 100% by intellisense.
Currently, novice users might even be more misguided by all those ObjectAssertions on Task (from the above table) that do not make any sense.

More importantly: Similar differences already exist elsewhere. For example I can't just do SutMethod.Should().NotThrow<Exception>(). I have to wrap it into an Action: new Action(_ => SutMethod()).Should().NotThrow<Exception>() to get the Throw assertions.

If we would do the same thing for tasks (having to wrap it into a Func<Task>) it would seem pretty consistent to me.

@lg2de
Copy link
Contributor Author

lg2de commented Mar 21, 2020

I think, discussion is completed so far.

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

No branches or pull requests

5 participants