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

IInvocation should expose Exception along with ReturnValue #1070

Closed
stakx opened this issue Oct 12, 2020 · 1 comment · Fixed by #1077
Closed

IInvocation should expose Exception along with ReturnValue #1070

stakx opened this issue Oct 12, 2020 · 1 comment · Fixed by #1077
Assignees
Milestone

Comments

@stakx
Copy link
Contributor

stakx commented Oct 12, 2020

#921 which just got merged added a ReturnValue property to IInvocation, which allows user code to find out what got returned from a mocked invocation. The author of that PR (@MaStr11) suggested that an Exception property also be added for similar reasons; if an invocation throws, user code might want to find out what it threw.

I think we ought to add Exception Exception { get; } to IInvocation for completeness. Incidentally, we'd then reach parity with Moq 5 in this particular regard.

Some implementation notes

  • I mentioned in that PR that the addition of an Exception property wouldn't make Invocation heavier (by adding a corresponding field). The existing field Invocation.returnValue could serve double duty (holding either a return value, or an exception). We'd need to ensure that methods returning an exception as a regular return value should not report that via the new Exception property.

  • Also [mentioned in the PR] is the likely Moq code location where Exception would be set.

@stakx stakx added this to the 4.15.0 milestone Oct 12, 2020
@MaStr11
Copy link
Contributor

MaStr11 commented Oct 14, 2020

P.S. @MaStr11: In case you were wondering where we stand regarding thrown exceptions, I didn't forget about it, but thought it would be cleaner to do that in a separate PR, to keep this one as straightforward as it was.

If you would like to submit a PR that adds an Exception Exception { get; } property to IInvocation (as you described in (1) above), please feel free to do so, and I'll merge that, too.

It shouldn't be too hard to do, setting the Invocation.Exception property would likely happen in a catch block being added there. (Ideally, we would make this addition without adding a new field to Invocation—to keep it as lightweight as possible–but that's not essential, it'd be more of an optimization. The existing returnValue field could serve double duty since an invocation cannot both return a value and throw. We'd only have to make sure that returned Exception objects won't leak out through the Exception property; this could be done by wrapping thrown exceptions in an internal type that only we can check for.)

From #921 (comment)

@stakx I will give it a try.

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

Successfully merging a pull request may close this issue.

2 participants