Feature: call counts and Times.Add #17

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@quetzalcoatl

This patch bases on some internal changes introduced in 'Mocking the delegates' feature.
I know the "single feature per branch" rule, but I perceive those two additions as one, because they address the same problem.


The .Verify + Times.AtLeast is handy, but sometimes you just need to have some more insight on what was going on. Especially, the Times class is lacking support for checking 'incremental' calls. In some cases, it is desirable to not specify how many calls there should be, but rather, how many new calls should occur.

This patch adds a few methods that allow you to get the number of method calls, in a way similar to how the fluent Verify() is normally used.

Just as you could write:

mock.Verify( obj => obj.Foo(), Times.Exactly(4) );

now you can also:

int n = mock.Count( obj => obj.Foo() );
Assert.True( n % 3 == 0 );

Additionally, this patch also includes some minor additions to the Times class

It is now possible to "offset" a 'Times' instance by a number, In but in no way this is intended to provide complex math. It is just a minor flavor that helps a little with some incremental corner cases.

var mock = new Mock<...>(....);
// do some setup that causes the Foo to be called unknown number of times
int n = mock.Count( obj => obj.Foo() );

mock.Bar(); // do the actual test that should call Foo >=3 times

mock.Verify( obj => obj.Foo(), Times.AtMost(3).Add(n) );

Offsetting is only possible in 'forward' manner. No subtracting is possible. Cases like Never/Once/Any are handled properly and intuitively, I hope:)
eg.:
(any) + 5 = (any)
(never) + 3 = (exactly:3)
(atmost:3) + 5 = (atmost:8)
(between:3..5) + 2 = (between:5..7)

Two new 'Times' members were also added, to make the Times/Add a little more flexible in certain cases:

  • Times.AcceptAny - always succeeds

    (acceptAny) = (atleast:0)
    (acceptAny) + 3 = (atleast:3)
    (acceptAny) + 5 = (atleast:5)

  • Times.AcceptNone - always fails

    (acceptNone) = (acceptNone)
    (acceptNone) + 3 = (acceptNone)
    (acceptNone) + 5 = (acceptNone)

@quetzalcoatl

Plase ignore comments earlier than -this- comment. They came from the 'mocking the delegates' feature, and are visible here too, but are mostly irrelevant for this feature (except for some common code base).

@kzu
Member
kzu commented Jun 21, 2012

How about calling the Count() method CallsTo() instead? Or maybe CountCalls(... ) ?

Also, I'm not sure about the usefullness of the last two:

Times.AcceptAny - always succeeds
isn't it just like specifying no Times?

I'm just thinking whether it makes more sense to just have a ResetCalls(...) method instead, that just sets the callcount to zero again, so that all existing Times work just like before? Seems like having to keep that count around just for adding it to Times is of little value?

Thoughts?

@quetzalcoatl
  1. Please focus on the example that allows you to write arbitrary checks:
int n = mock.Count( obj => obj.Foo() );
Assert.True( n % 3 == 0 );

there is no way to do that with just Verify() and Times class. Also, there is little sense in extending the Times structure to have gazillions of operators/helpers that will provide ability to construct robust conditions, because one can write a simple Assert with well-readable integer math.

  1. I am completely aware that existence of Times.Add stands against of what I just wrote. I added that method not to provide 'math', but to provide some support for Verify()s in a loop, and handling simple call-count increments. For all more complex cases like that %3==0, the .Count() is provided.

The Times.AcceptAny exists just to visually complement the Times.AcceptNone.
The Times.AcceptAny is equal to Times.AtLeast(0), which indeed checks nothing - but makes sense as a neutral starting point (zero-like value) for subsequent calls to Times.Add; like zero for "int i=0" at beginning of FOR loop.

The method Times.Add is only a syntax sugar, piggybacked here. It can be safely removed/separated from the rest of Call-Counts. I considered posting is as separate feature, but I found it useful in creating a few abstract algorithms that were reused in data-driven tests that tested a generator of a tree-like data structure, so I thought that maybe someone will find that handy too. It does not break any previous features of Times, so it seemed to me as safe enhancement.

  1. I agree that ResetCallCounts seems also handy, I'm actually surprised I did not thought about it earlier. But what would it actually cause to be zeroed? All counters? That would probably quite easy to implement, and surely it would make some tests more readable, as you would not have to constantly remember to update the dozens of subsequent Times.XYZ after you removed a line at the beginning of the test. The lack of fine-grained control smells a bit, but the method seems very easy to grasp, and this is a big plus.

On the other hand, trying to provide some fluent control like ResetCall(obj => obj.Foo(It.IsAny(), It.Is(...))) seems a bit of overkill, and I think it would be very hard to use properly. From "the test-writer's point of view", the .Count(filter) used like .Verify(filter) seems more natural to me than ResetCalls(filter), which forces me to thinking about invocation log history entries instead of what new has recently happened.

  1. At first, I named the new public methods "CountCalls", but then, it occurred to me that all other similar methods are Setup() and Verify() not SetupCall and VerifyCall, so I renamed it to Count() for consistency. Actually, there's Count(filter) in LINQ, so I think it it fits even better.

To sum up:

  • I think that Call-Counts are handy (maybe not for daily use, but still), and a additional ResetCallCounts would be a wise addition, but cannot substitute them
  • Having the Reset, the all changes to Times class fall into 'deletium' zone. If you wish, I'll remove them. I'm not sure however if I will manage to implement Reset in a reasonable time
  • I'd personally stick with .Count() to match current naming, but CountCalls is OK too. (on CallsTo, I think it could be mistaken for Setup by novices)
@quetzalcoatl quetzalcoatl Peeking mock call counts and simple Times math
The .Verify + Times.AtLeast is handy, but sometimes you just need to have some more insight on what was going on. Especially, the Times class is lacking support for checking 'incremental' calls. In some cases, it is desirable to not specify how many calls there should be, but rather, how many new calls should occur.

This patch adds a few methods that allow you to get the number of method calls, in a way similar to how the fluent Verify() is normally used.
b995102
@kzu
Member
kzu commented Jun 25, 2012
  1. Seems like a contrived example to me :). Focusing on contrived examples doesn't help, hehe. So if you can come up with something that looks like a real one, it would be awesome ;).

  2. ResetCalls (or whatever we call it) would receive the lambda that you want to reset for.

"From "the test-writer's point of view", the .Count(filter) used like .Verify(filter) seems more natural to me than ResetCalls(filter), which forces me to thinking about invocation log history entries instead of what new has recently happened.".
I fail to see the difference. The precise call (filter) you care about, is what will get reset. Just like you know you need to count before verifying, you know you need to reset before verifying, no?

  1. Ok, I buy Count() as a name. Still not sure about the feature though...
@quetzalcoatl

I couln't came up with a real code, as it was already refactored and currently coes not use .Count(). I managed to glue up some artifical case that is similar to what I had been testing back then.

I've posted it along with some explanations and thoughts at http://quetzalcoatl-pl.blogspot.com/2012/06/random-thoughts-on-moq-and-incremental.html

Excuse me but, well, ekhem, it grew quite lengthy, yet again ;)

At first, I tried to show the possible pains of .Reset without .Count, and what is the core difference.
Then, how Count may help to solve that.
Then, I remembered another idea that I had (but have no time to implement, yet) - "checkpoints" or "marks".
Do not frown, I do not mean chekpointing and versioning the whole mock, etc :)

If you really don't like adding the .Count method, or if you don't have time to read as whole, just ignore the Count() for a while and read the text from "Actually, what I'd like to see is:" (bolded line near the 2/3 of the post) and please let me know what you think about packaging those features into Marks.

@kzu
Member
kzu commented Sep 7, 2012

Will take a look at that. Thanks for looking deeper into this :)

@kzu
Member
kzu commented Apr 3, 2013

I like the Marks proposal returning from Verify :).

Not a fan of Count though ;).

Mark could just have a .Verify(), it's implicit (I think) that it's for the subsequent calls after the original Verify. And since it has different parameters (no expression), it's also reinforcing of that "next-ness".

Did you have a chance to work on that?

Cheers!

@LeonAdeoye

Daniel, I am using Moq for the first time this week after switching from RhinoMocks. Amongst a number of reasons, I find Moq's syntax more intuitive. However, I am wondering where this thread has gone as I also faced this issue with keeping track of incremental calls to the same methods (across many test functions). To get around this, I am using the Callback method but would prefer a more intuitive Times.Reset(). Any advice would be much appreciated.

    [Test]
    public void SaveSearch_SelectedBookAndCriteriaDescriptionKeyPropertiesBothSet_CallsSaveSearchMethodOfSearchManager()
    {
        searchManagerMock.Setup(m => m.SaveSearch(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<bool>(),
            It.IsAny<bool>(), It.IsAny<IDictionary<String, String>>())).Callback(() => wasCalled = true);

        functionsViewModel.SelectedBook = testBook;
        functionsViewModel.CriteriaDescriptionKey = "Test Criteria";
        functionsViewModel.SaveSearch();

        Assert.IsTrue(wasCalled, "Saving a newly added book critiria does NOT call SaveSearch method of ISearchManager!");
    }
@kzu
Member
kzu commented Aug 29, 2013

across many test functions

You're reusing the mocks across tests? That's kind of a red herring...

kzu @ mobile
On Aug 28, 2013 9:07 PM, "Leon Adeoye" notifications@github.com wrote:

Daniel, I am using Moq for the first time this week after switching from
RhinoMocks. Amongst a number of reasons, I find Moq's syntax more
intuitive. However, I am wondering where this thread has gone as I also
faced this issue with keeping track of incremental calls to the same
methods (across many test functions). To get around this, I am using the
Callback method but would prefer a more intuitive Times.Reset(). Any advice
would be much appreciated.

[Test]
public void SaveSearch_SelectedBookAndCriteriaDescriptionKeyPropertiesBothSet_CallsSaveSearchMethodOfSearchManager()
{
    searchManagerMock.Setup(m => m.SaveSearch(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<bool>(),
        It.IsAny<bool>(), It.IsAny<IDictionary<String, String>>())).Callback(() => wasCalled = true);

    functionsViewModel.SelectedBook = testBook;
    functionsViewModel.CriteriaDescriptionKey = "Test Criteria";
    functionsViewModel.SaveSearch();

    Assert.IsTrue(wasCalled, "Saving a newly added book critiria does NOT call SaveSearch method of ISearchManager!");
}


Reply to this email directly or view it on GitHubhttps://github.com/Moq/moq4/pull/17#issuecomment-23458458
.

@LeonAdeoye

I was thinking that it would be better to initialize these 'utility' mocks in the testfixture setup method just the once to speed up the testing and not have to do it for each test.

@kzu
Member
kzu commented Sep 17, 2013

This should be an instructive read on the topic: http://jamesnewkirk.typepad.com/posts/2007/09/why-you-should-.html

I'd strongly advice against such reuse. At most, you should initialize those mocks in the class constructor (in xUnit) and therefore you don't need to track incremental calls or anything, since they are brand new instances for every test that runs.

@LeonAdeoye

Thanks Dan.
While I understand the reasoning behind the link you sent me, I am not sure I agree with it for all situations.
In my case, I have several stub and mocks to create, and several helper objects to use for testing, and I don't think duplicating initialization code for 100s tests is worth it. But thanks for your suggestion - onboarded.

@kzu
Member
kzu commented Sep 18, 2013

I don't think duplicating initialization code for 100s tests is worth it

You don't have to. You can reuse your factories that create them ;)

@kzu kzu closed this Sep 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment