paulegradie / Sailfish

Sailfish - a production friendly performance benchmark runner for .NET
https://paulgradie.com/Sailfish/
MIT License
11 stars 4 forks source link

Cancellation token cannot be provided for methods that are not marked async #139

Closed droyad closed 4 months ago

droyad commented 4 months ago

A Parameter injection is not supported for void methods is raised when you have a setup or test method in the form

[SailfishMethod]
public Task Run(CancellationToken ct) => RunTheTest(ct)

The message is confusing as the return type isn't void. Looking at the code, it assumes that if the async attribute isn't present it's a void return type.

It is also unclear which method is being talked about.

Perhaps it should look to see if the method return Task instead of looking for the async attribute. This is likely more reliable as I suspect the async attribute will be present for ValueTask as well, which will cause the code to fail when casing to Task.

The message could be clarified to say The 'Run' method in 'MyTestClass' must have a return type of 'Task' if it has parameters

paulegradie commented 4 months ago

Hey Rob! :D

Hmm, yes this impl is a bit silly. A clearer msg will be good - and I'm reconsidering the decision to discriminate between sync and async methods when allowing the ct to be injected, so I think the way forward on this is to clean up the invocation reflection a bit (consolidate and handle ValueTask), update the exception messages as you suggest, and then allow cancellation token injection into any lifecycle method.

https://github.com/paulegradie/Sailfish/pull/141

I appreciate you dude.

Note: Beware of breaking changes when upgrading. Went through an instability period in the library between 1.3-1.6. Going forward there will be fewer breaking changes and a ChangeLog.