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

Mock throwing an exception for async methods #609

Closed
a-l-e-x-n-k opened this issue Jan 30, 2020 · 7 comments · Fixed by #663
Closed

Mock throwing an exception for async methods #609

a-l-e-x-n-k opened this issue Jan 30, 2020 · 7 comments · Fixed by #663
Labels
feature-request Request for a new NSubstitute feature

Comments

@a-l-e-x-n-k
Copy link

The problem is described here in comments under the answer: https://stackoverflow.com/questions/38338906/nsubstitute-mock-throwing-an-exception-in-method-returning-task

The problem with this is that the exception is thrown at the wrong time.
var t = AsyncMethod(); (actual behavior: exception will be raised here)
await t; (expected behavior: exception will be raised here)

With an async method the exception should be thrown when awaiting the task, Current approach throws the exception immediately.

Current behavior little bit unexpected in some cases. It would be great to correct it. Or may there is more elegant way to solve the problem.

@dtchepak
Copy link
Member

Hi @A-l-e-x-e-y ,

Thanks for raising this issue. I think this answer is correct: we need to use Task.FromException to wrap an exception in a task and return that.

For why this is necessary, consider these two implementations of AsyncMethod:

 public interface IExample {
    Task<string> AsyncMethod(bool fail);
}

public class Example : IExample {
    public async Task<string> AsyncMethod(bool fail) {
        if (fail) throw new NotImplementedException();
        // do async stuff
        return "hi";
    }
}

public class OtherExample : IExample {
    public Task<string> AsyncMethod(bool fail) {
        if (fail) throw new NotImplementedException();
        return Task.Run(() => {
            // do async stuff
            return "hi";
        });
    }
}

Both throwing synchronously and throwing asynchronously both meet the required contracts, so we have to support both.

To make this easier, how would you feel about adding ThrowsAsync() to NSubstitute.ExceptionExtensions to make this approach easier?

It could mirror the existing Throws overloads, starting with:

public static ConfiguredCall ThrowsAsync(this Task value, Exception ex) { 
    // return using `Task.FromException`
}

@dtchepak dtchepak added the feature-request Request for a new NSubstitute feature label Jan 30, 2020
@a-l-e-x-n-k
Copy link
Author

a-l-e-x-n-k commented Jan 31, 2020

Hi @dtchepak ,

Thank you for detailed explanation.

I understand the problem. And I agree that it would be more convenient and easier to use something like ThrowAsync () in this case. I planned to suggest the same variant.

@dtchepak
Copy link
Member

@A-l-e-x-e-y Would you have time to send a PR through with this change?
If not let me know and I'll add it when I can. 😃

@zvirja
Copy link
Contributor

zvirja commented Jan 31, 2020

ThrowsAsync looks like a great addition to the library! Don't forget to also add overloads for ValueTask we support everywhere 😉

@a-l-e-x-n-k
Copy link
Author

@dtchepak Ok. I'll find time for this.
@zvirja Thank you for reminder.

@Socolin
Copy link
Contributor

Socolin commented Aug 27, 2021

Hello, I'm also interested by ThrowAsync() I think it will help avoid mistake and make it quicker to write :)

Are you accepting PR for this ?

@dtchepak
Copy link
Member

Hi @Socolin , yes I'd definitely be happy to accept a PR for this! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for a new NSubstitute feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants