Add exception property to IInvocation #1077

MaStr11 · 2020-10-14T16:22:53Z

…urnValue and IInvocation.Exception.

MaStr11

@stakx Can you please have a look at my solution? Does this come close to what you had in mind?

src/Moq/IInvocation.cs

MaStr11 · 2020-10-14T16:53:20Z

src/Moq/Invocation.cs

@@ -69,16 +69,25 @@ public MethodInfo MethodImplementation

 		public Type ProxyType => this.proxyType;

+		/// <inheritdoc cref="IInvocation.ReturnValue"/>


I don't know if this is needed/supported and I have not checked whether it is working or not.

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.)

src/Moq/ProxyFactories/CastleProxyFactory.cs

stakx

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/IInvocation.cs

stakx · 2020-10-14T17:31:00Z

src/Moq/Invocation.cs

@@ -69,16 +69,25 @@ public MethodInfo MethodImplementation

 		public Type ProxyType => this.proxyType;

+		/// <inheritdoc cref="IInvocation.ReturnValue"/>


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.)

src/Moq/Invocation.cs

src/Moq/InvocationExceptionWrapper.cs

src/Moq/ProxyFactories/CastleProxyFactory.cs

tests/Moq.Tests/InvocationsFixture.cs

stakx · 2020-10-14T18:06:59Z

tests/Moq.Tests/InvocationsFixture.cs

+			// Base-call exceptions are not recorded
+			Assert.Empty(mock.Invocations);


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?

Didn't think of the virtual requirement. I will add another test and change the code comment accordingly.

I added another test 09816d0

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!)

…MockException)

MaStr11 · 2020-10-14T20:43:47Z

@stakx I think I addressed all your comments (nice feedback, btw). I think the last open topic is whether MockExceptions should be assigned to Exception or not: https://github.com/moq/moq4/pull/1077/files#r504932581

stakx

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.

src/Moq/ProxyFactories/CastleProxyFactory.cs

stakx · 2020-10-14T20:53:19Z

tests/Moq.Tests/InvocationsFixture.cs

+			// Base-call exceptions are not recorded
+			Assert.Empty(mock.Invocations);


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!)

tests/Moq.Tests/InvocationsFixture.cs

MaStr11 · 2020-10-15T12:38:32Z

I addressed your feedback and the PR should be done now. If I should do anything further, just let me know.

stakx · 2020-10-15T12:45:17Z

@MaStr11, excellent work, thanks for contributing once again!

@JeffAshton

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) ...

MaStr11 added 8 commits October 14, 2020 15:59

Add Exception property to IInvocation

cc90a0f

Introduce InvocationExceptionWrapper and add logic to IINvocation.Ret…

2a6a5f4

…urnValue and IInvocation.Exception.

Add inheritdoc doc comments.

01b9329

Add tests for IInvocation.ReturnValue

dd5c654

Add test MockInvocationsIncludeException_Setup

1936182

Add exception handling to Interceptor.Intercept

67793f3

Add test for "CallBase = true" and method returning an exception type.

e49273f

Add code comment why mock.Invocations is empty.

a66310e

MaStr11 commented Oct 14, 2020

View reviewed changes

stakx suggested changes Oct 14, 2020

View reviewed changes

stakx added this to the 4.15.0 milestone Oct 14, 2020

MaStr11 added 12 commits October 14, 2020 20:37

Add header to InvocationExceptionWrapper

1e18aff

Change doc comments on IInvocation.

988feb0

Remove "inhertitdoc" from internal type.

a18d167

Harden test by replacing hard coded value by calculated random.Next

31f5ec7

Add MockInvocationsIncludeException_BaseCall_Virtual test

09816d0

Add test for behavior of MockException and remove exclusion of catch(…

a064c98

…MockException)

Update changelog

22f3575

Revert whitespace change

f2cfc4c

Revert whitespace change

0ae9189

Use Assert.Throws instead of try/catch

6055015

Formatting

e1fa793

Whitespace

b2c07f3

Fix failing test

b9a4d63

stakx approved these changes Oct 14, 2020

View reviewed changes

MaStr11 added 2 commits October 15, 2020 13:27

Remove misleading test

3f2a43b

Assert thrown.Reasons instead of thrown.Message

3b5be64

stakx merged commit 5323136 into devlooped:master Oct 15, 2020

MaStr11 deleted the IInvocationException branch October 15, 2020 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exception property to IInvocation #1077

Add exception property to IInvocation #1077

MaStr11 commented Oct 14, 2020

MaStr11 left a comment

MaStr11 Oct 14, 2020

stakx Oct 14, 2020

stakx left a comment

stakx Oct 14, 2020

stakx Oct 14, 2020

MaStr11 Oct 14, 2020

MaStr11 Oct 14, 2020

stakx Oct 14, 2020 •

edited

MaStr11 commented Oct 14, 2020

stakx left a comment

stakx Oct 14, 2020 •

edited

MaStr11 commented Oct 15, 2020

stakx commented Oct 15, 2020

		@@ -69,16 +69,25 @@ public MethodInfo MethodImplementation

		public Type ProxyType => this.proxyType;

		/// <inheritdoc cref="IInvocation.ReturnValue"/>

		// Base-call exceptions are not recorded
		Assert.Empty(mock.Invocations);

Add exception property to IInvocation #1077

Add exception property to IInvocation #1077

Conversation

MaStr11 commented Oct 14, 2020

MaStr11 left a comment

Choose a reason for hiding this comment

MaStr11 Oct 14, 2020

Choose a reason for hiding this comment

stakx Oct 14, 2020

Choose a reason for hiding this comment

stakx left a comment

Choose a reason for hiding this comment

stakx Oct 14, 2020

Choose a reason for hiding this comment

stakx Oct 14, 2020

Choose a reason for hiding this comment

MaStr11 Oct 14, 2020

Choose a reason for hiding this comment

MaStr11 Oct 14, 2020

Choose a reason for hiding this comment

stakx Oct 14, 2020 • edited

Choose a reason for hiding this comment

MaStr11 commented Oct 14, 2020

stakx left a comment

Choose a reason for hiding this comment

stakx Oct 14, 2020 • edited

Choose a reason for hiding this comment

MaStr11 commented Oct 15, 2020

stakx commented Oct 15, 2020

stakx Oct 14, 2020 •

edited

stakx Oct 14, 2020 •

edited