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 #1013

Closed
wants to merge 5 commits into from

Conversation

@lg2de
Copy link
Contributor

@lg2de lg2de commented Mar 23, 2019

This PR implements #1001.
In contrast to PR #1011 this attempt provides only the extensions on Task/Task<T> and should not be a breaking change.

As already noted, I found timing issues in my and other tests. Today I added a prototype to abstract real timing in the tests. What do you think?

Unfortunately I could not make ITimer internal as planned. FluentAssertions is a strong named assembly. So, the test assembly must use strong named too, if it should refer to ITimer. (Otherwise the InternalVisiblesTo attribute will fail.) This is not possible because the Chill package is not strong named.
Please help to find solution to make ITimer internal.

@lg2de lg2de mentioned this pull request Mar 23, 2019
@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Mar 24, 2019

You can't make ITimer internal because it's a parameter of the TaskAssertions class. And we kept all those assertion classes (the ones returned by the Should methods) because this allows people to extend them.

@lg2de
Copy link
Contributor Author

@lg2de lg2de commented Mar 24, 2019

Src/FluentAssertions/Common/TaskTimer.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Specialized/TaskAssertions.cs Outdated Show resolved Hide resolved
TimeSpan timeSpan, string because = "", params object[] becauseArgs)
{
var delayTask = this.timer.DelayAsync(timeSpan);
var completedTask = await Task.WhenAny(Subject, delayTask);
Copy link
Member

@dennisdoomen dennisdoomen Apr 20, 2019

If the Task represented by Subject completes before the timeout, you must cancel the timer. Something like https://github.com/liquidprojections/LiquidProjections/blob/master/Src/LiquidProjections.Testing/TaskExtensions.cs#L9

Copy link
Contributor Author

@lg2de lg2de May 5, 2019

Why do you think, that the delay task needs to cancelled. It will just complete in background without reasonable effort, I think.
Introducing new items like a CancellationToken will also create effort.
I think this is in balance.

Copy link
Member

@dennisdoomen dennisdoomen May 5, 2019

Just imagine a situation with thousands of tests running concurrently. If the timeout expires before the task completes, you might end up with a lot of unfinished background activity. This is a situation that you need to avoid.

Tests/Shared.Specs/TestingTimer.cs Outdated Show resolved Hide resolved
@dennisdoomen dennisdoomen changed the title assertions for Task and Task<T> with CompleteWithin checks Assertions for Task and Task<T> with CompleteWithin checks Apr 20, 2019
[Pure]
public static TaskAssertions Should(this Task task)
{
return new TaskAssertions(task, new TaskTimer());
Copy link
Collaborator

@jnyrup jnyrup May 5, 2019

🤔 If we assert on a Task hasn't the execution already started?
That would mean we aren't measuring the execution time from the beginning of the execution.

Copy link
Contributor Author

@lg2de lg2de May 5, 2019

Yes, that may happen.
Do you have any suggestion?

Copy link
Collaborator

@jnyrup jnyrup May 5, 2019

How about reusing the existing Func<Task> and Func<Task<T>> extension methods?

Copy link
Contributor Author

@lg2de lg2de May 5, 2019

Could you be a little more specific, please?
Further I think that such a problem cannot be solved completely precisely: The timing starts either before the start of the task and measures too much time. Or the measurement starts after the task and measures too much time.

Copy link
Collaborator

@jnyrup jnyrup May 5, 2019

Of course, I would reuse these two existing overloads of Should that takes Func<Task> and Func<Task<T>>

public static AsyncFunctionAssertions Should(this Func<Task> action)
public static AsyncFunctionAssertions Should<T>(this Func<Task<T>> action)

Measuring execution times is inherently difficult, for unit tests without warm-up, is downright unreliable.
That requires dedicated benchmark tools such as BenchmarkDotNet.
E.g. the order in which the unit tests are executed now matters, due to GC, JITting, threads, etc.
As we can only execute a single statement at a time, we cannot start measuring at the exact same time the execution starts.
All other places (I hope so) we start the clock before starting the execution.
See e.g. ExecutionTimeAssertions.cs

Copy link
Contributor Author

@lg2de lg2de May 5, 2019

Within this kind of extension we have no chance to start timing before starting the Task. It is an extension for the task (instance) itself.
We can either start timing after task is running or we can stop here. Isn't it?

Copy link
Collaborator

@jnyrup jnyrup May 5, 2019

My thought is to avoid these two overloads of Should and use the existing ones.
The reason is to avoid fragmenting the API.
Then CompleteWithin could be added to AsyncFunctionAssertions?

Copy link
Member

@dennisdoomen dennisdoomen May 6, 2019

I agree with @jnyrup. And it's not just about when your timer starts running. It's purely to ensure we have a consistent API. After reading our response on Slack, I really hope you're willing to finalize this and the other comments. I'd love to have this in the next release.

Tests/Shared.Specs/TestingTimer.cs Outdated Show resolved Hide resolved
@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented May 25, 2019

Replaced by #1048

@lg2de lg2de deleted the TaskExtensions branch Mar 8, 2020
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