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

CompletesWithin(TimeSpan) and ThrowsWithin(TimeSpan) assertions #1797

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

thomhurst
Copy link
Owner

@thomhurst thomhurst commented Feb 8, 2025

@thomhurst thomhurst enabled auto-merge (squash) February 8, 2025 16:46
@thomhurst thomhurst merged commit 291ff94 into main Feb 8, 2025
9 checks passed
@thomhurst thomhurst deleted the feature/delegate-timing-assertions branch February 8, 2025 16:55
@AlexandreBossard
Copy link

Thank you !
Looking at your code, it's not clear to me that the Task will be aborted if it exceed the TimeSpan ?

@thomhurst
Copy link
Owner Author

Thank you ! Looking at your code, it's not clear to me that the Task will be aborted if it exceed the TimeSpan ?

You're right, it wouldn't. The assertions library isn't currently set up to abort tasks. I'd need to pass a cancellation token to the delegate used in the Assert.That and you'd need to use that in your task. Is that how FA did it?

@AlexandreBossard
Copy link

The assertions library isn't currently set up to abort tasks

Yeah that's what I figured when I tried it myself.

FA don't bother with cancellation token. It creates a new Task with a Timer and use Task.WhenAny(target, timeoutTask) for timeouts and manually cancel the operation. The code is here.

@thomhurst
Copy link
Owner Author

The assertions library isn't currently set up to abort tasks

Yeah that's what I figured when I tried it myself.

FA don't bother with cancellation token. It creates a new Task with a Timer and use Task.WhenAny(target, timeoutTask) for timeouts and manually cancel the operation. The code is here.

From what I can tell, that won't cancel that task, it'll just leave it in the background. But it does solve the problem of not waiting for it which TUnit currently would. I'll have to have a think. Might be a chunky refactor.

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

Successfully merging this pull request may close these issues.

None yet

2 participants