-
-
Notifications
You must be signed in to change notification settings - Fork 802
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
Regression in 4.7.46: overwritten Setup still runs match conditions. #433
Comments
Hi @evildour, thanks for reporting this. The regression appears to have been introduced in version 4.7.46. I'm looking into it now. |
OK, I've tracked this change in behaviour down to the following change mentioned in the changelog entry for 4.7.46:
Before 4.7.46, there was some faulty logic in an internal class ( Only now, we are having the reverse problem: your two setups are for the same method invocation (" So in the end I think there are two or three problems here:
Not quite sure what to do about this. Your scenario (sequentially setting up the same method call twice with actualy proxy invocations in-between) is probably an edge case, and fixing this might potentially break a lot of more common scenarios. Any thoughts? |
Hi @stakx, thanks for the quick response. I tested my sample above with 4.7.58 and 4.7.63 from nuget and it worked in these two. The first version where it was broken was 4.7.99, so perhaps there is something else going on here? I believe what you're saying is that this is expected because the 2nd setup shouldn't override the 1st. If that is indeed the case, then I believe there is a different bug here: the verification being done in the 1st setup when the value 2 is passed is being ignored. That condition is returning false in that case but the I've actually worked around the issue by verifying, then resetting, then using Setup again, so if you're telling me this is working as intended, that's fine. As far as not having side effects, that isn't really an option for the case where I was using this behavior: I needed to capture a randomly generated id for an instance that was passed to a method on the mocked object so I could use that id for verification purposes elsewhere (see here). There was no other way to get the id. |
@evildour - sorry for the late reply.
Strange that we are getting differing results for your repro code with past Moq versions. I'll check again, just to make sure.
Sorry for not having expressed myself very clearly, I was in a bit of a hurry pressing the Comment button before hopping off a train. I meant to say that in this particular scenario, the 2nd setup should override the 1st and therefore your code should work. But the 2nd setup does not override the 1st anymore... at least not in 4.7.99. The fact that it did in a previous Moq version was purely incidental, due to a bug in We need to solve this problem a different way, namely by deciding whether your 2nd setup should override the 1st. This comes down to whether two setups for the same method call that only differ in the matcher used are equal, or to narrow it down even more: whether any two matchers are considered equal or not. I'm not absolutely certain what the general answer to that last question should be.
Glad you found a workaround!
I am only voicing my own opinion here, and I believe that in your particular scenario, things aren't working as intended. It would make more sense for the 2nd setup to override the 1st. I'm just not sure what the correct answer would be for the more general question raised here (matcher equality). This is up for debate, ideally we'd have a counterexample for consideration. |
I see, that makes sense. I'm not sure what the right thing to do here is. I can see arguments for it going either way and honestly I think either one would be fine. But regardless, I think there is some bug here. If you feel the 2nd setup should override the 1st, then the 1st matcher condition shouldn't be evaluated. If you feel the 2nd setup shouldn't override the 1st, then the false result of the 1st matcher condition shouldn't be ignored. So before things were being totally overridden. Now they're being partially overridden. Unless I'm just misunderstanding how things are supposed to work (I'm new to Moq so it's quite possible). But I assume if the matcher returns false for a particular argument passed in, the verification should fail. If I change the first |
Ok, so I must apologize. It just dawned on me I am pretty sure I was misunderstanding things due to: 1) being new to Moq, 2) never using a mocking framework before, and 3) making some poor assumptions based on some other testing code I saw in the repo I was working in. I assumed what I was doing was setting up a verification on arguments to a method call where the predicates for each argument must evaluate to true and if they don't, a later On a side note, would you be able to point me to any docs that would have cleared up my mistake here, such as a really basic introduction into how to use a library like this? I didn't see anything explicit in the quick start that explained what a setup is. The API docs have a summary of "Specifies a setup on the mocked type for a call to to a void method" and remarks of "If more than one setup is specified for the same method or property, the latest one wins and is the one that will be executed", both of which could seemingly apply to my bad assumption of things. And for |
Yes, this sounds roughly correct. Setting up a method lets the mock know to "expect" a specific method call. (In fact, the various Non-strict mocks (those initialized with mockFoo.Verify(foo => foo.SetValue(1)); Argument matchers only come into the picture when you don't know, or don't care about, specific argument values. Matchers come in various forms, e.g. as placeholders for any value of a given type ( In summary, both
At current, Moq only has the (somewhat outdated) API reference documentation and the quickstart guide that you've already seen. There are also some posts on kzu's blog that may help. But there is no conceptual overview, unfortunately. (Not sure why, perhaps it was thought that Moq was more or less self-explanatory, given some knowledge of how mocking libraries work in general.) I'd say anyone's welcome to improve or expand the documentation in the wiki. |
Thanks for such a detailed explanation. I think exactly what you wrote here would make a great intro to someone unfamiliar with these concepts. So based on all this, it sounds like my "workaround" of verify all and reset is not quite the right way to do things. It looks like a better fix is to use setups with non-capturing predicates and then callbacks to capture the values I want, which when applied to the sample above, looks like this: var mockFoo = new Mock<Foo>();
int oneValue = 0;
mockFoo.Setup(foo => foo.SetValue(1)).Callback<int>(x => oneValue = x);
mockFoo.Object.SetValue(1);
int twoValue = 0;
mockFoo.Setup(foo => foo.SetValue(2)).Callback<int>(x => twoValue = x);
mockFoo.Object.SetValue(2);
mockFoo.VerifyAll();
Debug.Assert(oneValue == 1);
Debug.Assert(twoValue == 2); Thanks again for your help and sorry for the confusion. |
With a class like this defined:
Something like this used to work:
But as of 4.7.99, the first assert fails because
oneValue
is 2, not 1. It appears the overwritten Setup's match condition is still being evaluated, but its result is simply ignored.The text was updated successfully, but these errors were encountered: