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

Proposal Verifiable return an IVerifier #534

Closed
BlythMeister opened this issue Nov 26, 2017 · 11 comments
Closed

Proposal Verifiable return an IVerifier #534

BlythMeister opened this issue Nov 26, 2017 · 11 comments

Comments

@BlythMeister
Copy link
Contributor

Could the verifiable method return an object (IVerifier) which has already been setup with the function call details.

You could then state (after execution of code) verifier.Verify(Times.Once)

This means you don't need to specify the expression and arguments twice, but keeps sortation of setup and verify.

@stakx
Copy link
Contributor

stakx commented Nov 26, 2017

Having a per-setup Verify method group isn't a bad idea per se. But I'm not sure, it could easily lead to rather ugly test code that's longer than necessary:

var mock = new Mock<T>(MockBehavior.Strict);
var setup1 = mock.Setup(m  =>)..Verifiable();
var setup2 = mock.Setup(m =>)..Verifiable();
…
setup1.Verify(Times.);
setup2.Verify(Times.);

If something like this is really required, IMHO we'd do better with either #373/#401 (setup.Verifiable(Times) followed by mock.Verify()) or #527 (mock.VerifyNoOtherCalls()).

Do you see any scenario(s) where your proposition would be preferable to the alternatives just mentioned?

@BlythMeister
Copy link
Contributor Author

#373/#401 would be my preferred, but they were rejected....so thinking of something different.

#527 i think is a must, but doesn't solve my stub and then verify times requirements.

Seeing the examples, your right, it's not nice.
Bring able to state the times as a verify without a repeat of full expression in the setup/return is what I would like, trying to think of a nice language construct.

#527 is more about not needing a strict mock and not needing to setup a void, but still verify that no extra methods called.

@stakx
Copy link
Contributor

stakx commented Nov 26, 2017

trying to think of a nice language construct

There's no need to reinvent the wheel, #401 probably comes very close to an ideal solution for specifying Times upfront.

my stub and then verify times requirements

Could you post an example that shows what you're unhappy with?

@BlythMeister
Copy link
Contributor Author

It's the bits I put into #530 (after I realised sequence existed)

If you have a complex function your setting up, the expression in the setup might be long.
To then assert it was called the correct number of times you need to repeat that setup.

So far my only workarounds are to add an extension/superclass to a mock which creates a Func to do a verify when calling setup or to put the expression into a variable. Neither are elegant.

The only reason I was thinking of other language constructs was because I thought that proposal was rejected (as the issue and or were closed)

@stakx
Copy link
Contributor

stakx commented Nov 27, 2017

If you have a complex function your setting up, the expression in the setup might be long.

Just one thought as an aside: Perhaps it is a good thing to feel that pain. Perhaps it means that you should make that function less complex.

I happen to think that while Moq shouldn't be too opinionated, it's OK to nudge people to consider better / cleaner designs.

In the past, Moq was changed in a way to help steer people towards a better separation of the Arrange and Assert phases of a test. (For example, .AtMostOnce() and .AtMost(n) were marked obsolete.)

That's why I'm reluctant to merge anything that takes Moq back in the other direction. It's perhaps the main reason why I closed #401, and it's the reason why I was asking for your specific use case—I want to be sure this is really needed before we go on.

@BlythMeister
Copy link
Contributor Author

The case I am working on right now isn't complex per say.
But it's a chat bot, so I'm having to stub the generated message and then assert I only got that message once.

The message is a piece of text.
I guess I could just put that expected message as a variable and use it twice rather than try to work around the MOQ framework.

I'm coming from a different framework (Rhino) and I'm thinking in that mindset, which as you say may not be correct, and maybe the functions/tests are too complex.

When you mark something as verifiable, does that mean 1 invocation or any number when you call verify?

@stakx
Copy link
Contributor

stakx commented Nov 27, 2017

When you mark something as verifiable, does that mean 1 invocation or any number when you call verify?

TL;DR: at least one invocation.

You're looking at a part of Moq that perhaps isn't very easy to understand.

You can perform verification against setups. This is what mock.Verify() and mock.VerifyAll() do. This type of verification checks whether each (non-conditional) setup received at least one invocation. The methods differ only in which setups they check: .VerifyAll() checks all setups, while .Verify() only checks those marked as .Verifiable().

You can perform verification against invocations. This is what the other Verify overloads (where you need to provide an invocation expression) do. This type of verification allows you to specify a Times, by default it checks whether there was at least one invocation matching the given expression.

@BlythMeister
Copy link
Contributor Author

@stakx Ok, thanks.

Maybe i am trying to compare and contrast to what i am used to (which may not be the best way to work)

I'm going to try and embrace the difference and see how i get on.

@BlythMeister
Copy link
Contributor Author

BlythMeister commented Nov 27, 2017

@stakx how would you feel about a .VerifyAll which takes a times?

So you could go .VerifyAll(Times.Once)
the default behaviour without passing would be Times.AtLeastOnce (as it is today)?

NB: leaving closed as i am curious and want to a place to discuss as i'm getting some good ideas from you :) (thanks!!)
Also, i hope you don't mind me picking your brain as i learn more about Moq :)

@stakx
Copy link
Contributor

stakx commented Nov 27, 2017

how would you feel about a .VerifyAll which takes a times?

This doesn't work. .Verify() and .VerifyAll() check several setups, I'd say the case where you want the same Times for every setup is pretty rare.

The only scenario where this might make good sense is the distinction between Times.AtLeastOnce (the default behavior of those verification methods) vs. Times.Once vs. Times.AtMostOnce. But it would be wrong, IMHO, to add a new .Verify[All](Times) overload for these particular cases because you could pass any Times, not just those three.

@BlythMeister
Copy link
Contributor Author

@stakx yeah very true...

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

No branches or pull requests

2 participants