-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
Add exception property to IInvocation #1077
Conversation
…urnValue and IInvocation.Exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stakx Can you please have a look at my solution? Does this come close to what you had in mind?
src/Moq/Invocation.cs
Outdated
@@ -69,16 +69,25 @@ public MethodInfo MethodImplementation | |||
|
|||
public Type ProxyType => this.proxyType; | |||
|
|||
/// <inheritdoc cref="IInvocation.ReturnValue"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is needed/supported and I have not checked whether it is working or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think <inheritdoc />
would be sufficient, but in fact you can drop this completely as Invocation
is an internal type, and user code will only ever see this property as IInvocation.ReturnValue
(which is already documented).
(Some types and type members do carry documentation despite being internal, and that's OK if those comments add info for developers that isn't found anywhere else.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, this is some great work so far! 👍
I do have a few change requests, mostly regarding details. Apart from the comments below, could you also add an entry to the CHANGELOG.md
again? (You can also change your previous entry for ReturnValue
to now include Exception
, if you'd like. In that case, just make sure to mention both PR numbers at the line's end.)
src/Moq/Invocation.cs
Outdated
@@ -69,16 +69,25 @@ public MethodInfo MethodImplementation | |||
|
|||
public Type ProxyType => this.proxyType; | |||
|
|||
/// <inheritdoc cref="IInvocation.ReturnValue"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think <inheritdoc />
would be sufficient, but in fact you can drop this completely as Invocation
is an internal type, and user code will only ever see this property as IInvocation.ReturnValue
(which is already documented).
(Some types and type members do carry documentation despite being internal, and that's OK if those comments add info for developers that isn't found anywhere else.)
// Base-call exceptions are not recorded | ||
Assert.Empty(mock.Invocations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good of you for thinking of this test case! (I didn't.)
Something's odd here—invocations should get recorded even when mock.CallBase == true
. I think the reason why no invocations get recorded here is that List<>.GetRange
is not virtual.
Could you change this test so it calls some overridable method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't think of the virtual requirement. I will add another test and change the code comment accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added another test 09816d0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this test completely. It really targets List<>.GetRange
, and nothing from Moq. What's more, it contains a factually incorrect statement in the comment, // Base-call exceptions are not recorded
. All things considered, this test as it stands will potentially raise more questions than it answers.
(The newly added test is fine, though!)
@stakx I think I addressed all your comments (nice feedback, btw). I think the last open topic is whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for making the changes!
I'm happy to merge this PR as it stands, however I do have two more comments (below) if you wish to refine things a little further. That's purely optional, though... otherwise, I'll merge your PR some time tomorrow.
// Base-call exceptions are not recorded | ||
Assert.Empty(mock.Invocations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this test completely. It really targets List<>.GetRange
, and nothing from Moq. What's more, it contains a factually incorrect statement in the comment, // Base-call exceptions are not recorded
. All things considered, this test as it stands will potentially raise more questions than it answers.
(The newly added test is fine, though!)
I addressed your feedback and the PR should be done now. If I should do anything further, just let me know. |
@MaStr11, excellent work, thanks for contributing once again! |
Bumps [Moq](https://github.com/moq/moq4) from 4.14.7 to 4.15.2. #Changelog *Sourced from [Moq's changelog](https://github.com/moq/moq4/blob/master/CHANGELOG.md).* > ## 4.15.2 (2020-11-26) > > #### Changed > > * Upgraded `System.Threading.Tasks.Extensions` dependency to version 4.5.4 (@JeffAshton, [#1108](devlooped/moq#1108)) > > > ## 4.15.1 (2020-11-10) > > #### Added > > * New method overloads for `It.Is`, `It.IsIn`, and `It.IsNotIn` that compare values using a custom `IEqualityComparer<T>` (@weitzhandler, [#1064](devlooped/moq#1064)) > * New properties `ReturnValue` and `Exception` on `IInvocation` to query recorded invocations return values or exceptions (@MaStr11, [#921](devlooped/moq#921), [#1077](devlooped/moq#1077)) > * Support for "nested" type matchers, i.e. type matchers that appear as part of a composite type (such as `It.IsAnyType[]` or `Func<It.IsAnyType, bool>`). Argument match expressions like `It.IsAny<Func<It.IsAnyType, bool>>()` should now work as expected, whereas they previously didn't. In this particular example, you should no longer need a workaround like `(Func<It.IsAnyType, bool>)It.IsAny<object>()` as originally suggested in [#918](devlooped/moq#918). (@stakx, [#1092](devlooped/moq#1092)) > > #### Changed > > * Event accessor calls (`+=` and `-=`) now get consistently recorded in `Mock.Invocations`. This previously wasn't the case for backwards compatibility with `VerifyNoOtherCalls` (which got implemented before it was possible to check them using `Verify{Add,Remove}`). You now need to explicitly verify expected calls to event accessors prior to `VerifyNoOtherCalls`. Verification of `+=` and `-=` now works regardless of whether or not you set those up (which makes it consistent with how verification usually works). (@80O, @stakx, [#1058](devlooped/moq#1058), [#1084](devlooped/moq#1084)) > * Portable PDB (debugging symbols) are now embedded in the main library instead of being published as a separate NuGet symbols package (`.snupkg) (@kzu, [#1098](devlooped/moq#1098)) > > #### Fixed > > * `SetupProperty` fails if property getter and setter are not both defined in mocked type (@stakx, [#1017](devlooped/moq#1017)) > * Expression tree argument not matched when it contains a captured variable – evaluate all captures to their current values when comparing two expression trees (@QTom01, [#1054](devlooped/moq#1054)) > * Failure when parameterized `Mock.Of<>` is used in query comprehension `from` clause (@stakx, [#982](devlooped/moq#982)) > > > ## 4.15.0 > > This version was accidentally published as 4.15.1 due to an intermittent problem with NuGet publishing. #Commits - [`f2aa090`](devlooped/moq@f2aa090) ...
Fixes #1070