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

Callback not raised in mocked subclass #1067

Closed
InteXX opened this issue Oct 10, 2020 · 15 comments · Fixed by #1068
Closed

Callback not raised in mocked subclass #1067

InteXX opened this issue Oct 10, 2020 · 15 comments · Fixed by #1068
Assignees
Labels
Milestone

Comments

@InteXX
Copy link

InteXX commented Oct 10, 2020

I'm running into the problem described in painful detail here. It seems Moq won't raise a callback when we mock a subclass.

The odd thing with all of this is that UserManager(Of TUser) is itself a subclass of UserManager(Of TUser, String). It has no members of its own. According to this scenario, mocking UserManager(Of TUser) should cause a callback failure as well—but it doesn't. The callback is raised as expected.

I don't know whether this is a bug in Moq or a bug in my code. Perhaps I'm not setting things up just right?

Suggestions appreciated.

@stakx
Copy link
Contributor

stakx commented Oct 10, 2020

Hi @InteXX, since you posted here, I'm going to ignore your longer Stack Overflow post (I am no longer participating there).

This is the output I'm getting for your code, using the current version of Moq (4.14.6 at this time of writing):

Callback for [SomeClass.SomeMethodAsync]
---------------------------------------------------------
Callback Param1: Called from SomeClass
---------------------------------------------------------


Callback for [SomeSubclass.SomeMethodAsync]
---------------------------------------------------------
Callback Param1: Called #1 from SomeSubclass
Callback Param2: Called #2 from SomeSubclass
---------------------------------------------------------

Unfortunately, you did not state exactly what output you expected, but judging from this issue's title, I would say everything works fine? The callback is triggered for the mocked subclass, too.

@InteXX
Copy link
Author

InteXX commented Oct 10, 2020

Life without SO? Oh my... how do you do it? ;-)

Unfortunately, my PoC code (on Fiddler) was incorrect at the time you tested it. Incorrect, that is, in terms of reproducing the problem. Here's the version that skips the callback:

Public Sub MockSomeSubclass()
  ...

  lResult = oMock.Object.SomeMethodAsync("Called from SomeSubclass").Result
End Sub

See the difference? Here we're calling the superclass's method, the inherited one with only a single parameter.

I did catch that oversight, but not before you'd tested it using this:

  lResult = oMock.Object.SomeMethodAsync("Called #1 from SomeSubclass", "Called #2 from SomeSubclass").Result

So it was a matter of timing. Yes, when fixed the PoC successfully raises both callbacks—so it's actually not a reproduction of the problem. But my original code, the one mocking Identity's UserManager, is still failing. The subclass's overload is never raised, even when I use the correct signature:

Dim oResult = Await Me.UserManager.CreateAsync(oUser, Model.Password, Model.City)

I've tried everything I can think of to get that callback to run. I'm still at a loss.

I've removed the Fiddle and all links pointing to it.

@stakx
Copy link
Contributor

stakx commented Oct 10, 2020

Yes, SomeMethodAsync(String) and SomeMethodAsync(String, String) are two distinct methods, so if you set up a callback for the latter but call the former, the callback won't be triggered.

Can you please post a complete but short repro code? The Stack Overflow post looks rather long and involved, I'd prefer not to have to piece together a repro myself from several long blocks of code and subsequent edits.

@InteXX
Copy link
Author

InteXX commented Oct 10, 2020

OK, will do. Give me a bit.

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

Now isn't that interesting!

The callback is raised as expected in C#, but not in VB.

Here's the repro solution:

https://1drv.ms/u/s!AodXF_j3BiWkheUQBVvdC3LHzizjAw

The offending line is Await Me.UserManager.CreateAsync(oUser, Model.Password, Model.City) in HomeController.CreateAsync().

Set a breakpoint there, and also in the callback: BaseTests.GetUserManagerMock(), and then debug HomeControllerTests.CreateUserSucceeds(). The C# project will break at the callback, but the VB project won't.

I'm not spotting any functional difference between the two. Can you?

@paul1956
Copy link

Where is the VB Event Handler created?

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

@paul1956

Event handler? No event handler...

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

@paul1956

What is it you're looking for?

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

@paul1956

Oh, you must be looking for the callback. It's in BaseTests.GetUserManagerMock().

@stakx stakx removed the needs-repro label Oct 11, 2020
@stakx
Copy link
Contributor

stakx commented Oct 11, 2020

@InteXX, thanks for the repro project.

I haven't verified this yet, but I am almost certain that this case will boil down to the fact that the VB.NET compiler is in the habit of adding type conversion expressions everywhere in LINQ expression trees (unlike the C# compiler).

When using C#:

You have this expression in your code:

Manager => Manager.CreateAsync(It.IsAny<TUser>(), It.IsAny<string>(), It.IsAny<TCity>())

Which the C# compiler turns into this (with generic type arguments substituted, and formatted with Moq's expression.ToStringFixed() extension method):

Manager => Manager.CreateAsync(It.IsAny<User>(), It.IsAny<string>(), It.IsAny<City>())

When using VB.NET:

You have this expression in your code:

Function(Manager) Manager.CreateAsync(It.IsAny(Of TUser), It.IsAny(Of String), It.IsAny(Of TCity))

Which the VB.NET compiler turns into this (with generic type arguments substituted, and formatted with Moq's expression.ToStringFixed() extension method):

Manager => Manager.CreateAsync((User)(object)It.IsAny<User>(), It.IsAny<string>(), (City)(object)It.IsAny<City>())

Can you see the difference?

So I suspect what might be going wrong here is that Moq doesn't trim away the superfluous (T)(object) conversions in the right places; either right away (at setup time), or when matching invocations (at call time).

@stakx
Copy link
Contributor

stakx commented Oct 11, 2020

Bingo!

So Moq already has some logic to deal with superfluous conversion expressions:

https://github.com/moq/moq4/blob/7b2d74e2d453f93b7a1e03a5bc0bd194c67ea14b/src/Moq/MatcherFactory.cs#L116-L127

However, this logic will not work if there are two conversions (as in your case). After removing one, another one remains, covering up the It.IsAny<T>(). Because of that, Moq will not instantiate the proper matcher type, leading to your setup never triggering later on.

If this were changed as follows:

-if (expression.NodeType == ExpressionType.Convert) 
+while (expression.NodeType == ExpressionType.Convert) 
 { 
 	expression = ((UnaryExpression)expression).Operand; 
 }

Your VB.NET test would pass like it should.

Note however that blindly removing conversion expressions might be incorrect in some instances. So we need to determine which (combinations of) conversion expressions are safe to remove, then rewrite the above code segment accordingly.

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

@stakx

Based on your initial discovery, I tried this:

Function(Manager) Manager.CreateAsync(It.IsAny(Of Db.User), It.IsAny(Of String), It.IsAny(Of Db.City))

It worked.

The change in casting is no skin off my nose, so I'll use it. (That other looks hard to get right and easy to get wrong.)

All of this reminds one of the Programmer's Poem:

I hate programming
I hate programming
I hate programming
It works!
I love programming

Too bad you're no longer on SO. I could send you the bounty ;-)

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

@stakx

Thank you for that.

@InteXX
Copy link
Author

InteXX commented Oct 11, 2020

@stakx

I marked this as the answer to the SO question.

@stakx
Copy link
Contributor

stakx commented Oct 11, 2020

@InteXX, you're welcome. Thank you for providing the input needed to make Moq a better (more robust) library! Regarding Stack Overflow, I'm glad you posted your own answer to give the question (and future visitors to it) some kind of closure. 👍

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