Skip to content

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

Merged
thomhurst merged 1 commit into
mainfrom
feature/delegate-timing-assertions
Feb 8, 2025
Merged

CompletesWithin(TimeSpan) and ThrowsWithin(TimeSpan) assertions#1797
thomhurst merged 1 commit into
mainfrom
feature/delegate-timing-assertions

Conversation

@thomhurst

@thomhurst thomhurst commented Feb 8, 2025

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

2 participants