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

MockSequence #21

Closed
michaelhidalgo opened this issue Jul 12, 2012 · 27 comments
Closed

MockSequence #21

michaelhidalgo opened this issue Jul 12, 2012 · 27 comments

Comments

@michaelhidalgo
Copy link

Reported by project member bcardiff, Mar 23, 2008

Implement MockSequence idea (as explained at
http://groups.google.com/group/moqdisc/browse_thread/thread/80404b259824812d
by Eugene) on top of Moq public interface.

@michaelhidalgo
Copy link
Author

Comment 17 by project member dan...@cazzulino.com, Jun 13, 2012

An implementation of this exists in Moq v4. There are apparently some issues, though.

It would be great if someone could work on that feature by forking the current repo at https://github.com/Moq/moq4

@grzesiek-galezowski
Copy link

Hi, I did a little work recently on bringing more proper sequences into Moq. The current approach works only with strict behavior, which is not what I liked. Also, implementing a solution that fits moq API nicely is hard when working only with public API (the current approach is based on .When() API, which requires usage of Setup().Verifiable() instead of mock.Verify(), and the latter is, AFAIK, the preferred and intuitive way for verifying mocks. Thus, some work inside the Moq code is required in my opinion to make this feature work fine.

The little prototype I made is on http://ubuntuone.com/44stjlAYK9kQzmx8gM5Jzh. It would be nice if someone could look at it and pick it up. I do not dare submitting it as a patch, since it is a little hacked, because I did my changes in Mono on Linux, so just for the sake of easier experimenting, I added some acceptance tests in Nunit (because they run in monodevelop - these can be easily converted to xUnit through) and I deleted all existing unit tests (because most of them would not pass). Anyway, it allows playing with the idea and gives a good clue on what changes would be needed in mock core (surprisingly, the changes required there are very small!). Also, this prototype does not support Times.XYZ(), however, this combination is not expected to be used extensively and can be added later without breaking any public API. Recursive mock verification is supported by the way.

Some examples to give a feel of what the API is like (the sequence is called LooseSequence because it allows skipping calls between the matched ones. Another sequence called StrictSequence can be developed that does not allow such skipping) are included below. As I said, the current solution is lacking in my opinion, so I implemented another one, that in my opinion gives more "native" feel:

[Test]
public void ShouldProperlyRecognizeNonMatchingArgumentsInSequentialVerification()
{
  var sequence = new LooseSequence();
  var mock1 = new Mock<RoleWithArgumentAndReturnValue> { CallSequence = sequence };
  var mock2 = new Mock<RoleWithArgumentAndReturnValue>  { CallSequence = sequence };

  mock2.Object.Do(1);
  mock1.Object.Do(2);
  mock2.Object.Do(3);

  mock1.VerifyInSequence(m => m.Do(2));
  Assert.Throws<MockException>(() => mock2.VerifyInSequence(m => m.Do(1)));

}

[Test]
public void ShouldRecognizeSequenceOfCallsMadeOnTheSameMock()
{
  var sequence = new LooseSequence();
  var mock1 = new Mock<RoleWithArgumentAndReturnValue> { CallSequence = sequence };

  mock1.Object.Do(1);
  mock1.Object.Do(2);
  mock1.Object.Do(3);

  mock1.VerifyInSequence(m => m.Do(2));
  mock1.VerifyInSequence(m => m.Do(3));
  Assert.Throws<MockException>(() => mock1.VerifyInSequence(m => m.Do(1)));

}

[Test]
public void ShouldSupportMockChainingWithMatchingCalls()
{
  var sequence = new LooseSequence();
  var mock1 = new Mock<RoleForRecursiveMocking>() 
  { 
    CallSequence = sequence, DefaultValue = DefaultValue.Mock
  };

  mock1.Object.Do().Do();

  mock1.VerifyInSequence(m => m.Do());
  mock1.VerifyInSequence(m => m.Do().Do());
}

[Test]
public void ShouldSupportMockChainingWithUnmatchingCalls()
{
  var sequence = new LooseSequence();
  var mock1 = new Mock<RoleForRecursiveMocking>() 
  { 
    CallSequence = sequence, DefaultValue = DefaultValue.Mock
  };

  mock1.Object.Do().Do();

  mock1.VerifyInSequence(m => m.Do().Do());
  Assert.Throws<MockException>(() => 
    mock1.VerifyInSequence(m => m.Do())
  );
}

@grzesiek-galezowski
Copy link

I put my changes together with some new implementation and a suite of acceptance tests on:
https://github.com/grzesiek-galezowski/moq4

I don't know the current process, should someone review this, or should I create a pull request straight away?

Also, I put my changes on dev branch. Is this OK?

@kzu
Copy link
Member

kzu commented Dec 26, 2012

I'd like to hear others' opinions on this too. Up front, it looks
interesting.

El miércoles, 26 de diciembre de 2012, Grzegorz Gałęzowski escribió:

I put my changes together with some new implementation and a suite of
acceptance tests on:
https://github.com/grzesiek-galezowski/moq4

I don't know the current process, should someone review this, or should I
create a pull request straight away?

Also, I put my changes on dev branch. Is this OK?


Reply to this email directly or view it on GitHubhttps://github.com//issues/21#issuecomment-11695801.

/from mobile

@grzesiek-galezowski
Copy link

Thanks, in the meantime, I added another small feature, which is VerifyNotInSequence(). Both of these combined allow verifications such as:

var mock = new Mock<RoleWithArgumentAndReturnValue>()
{
    CallSequence = new LooseSequence()
};

mock.Object.Do(3);
mock.Object.Do(2);

mock.VerifyInSequence(m => m.Do(3));
mock.VerifyNotInSequence(m => m.Do(3));
mock.VerifyNotInSequence(m => m.Do(33));
mock.VerifyNotInSequence(m => m.Do(987));
mock.VerifyInSequence(m => m.Do(2));

Can't wait for the feedback!

@kzu
Copy link
Member

kzu commented Jan 29, 2013

I find this quite confusing:

mock.VerifyInSequence(m => m.Do(3));
mock.VerifyNotInSequence(m => m.Do(3));

The fact that calling Verify causes the mock to change its internal state (track what should be the next Verify) is a bit puzzling. Not sure what the alternatives are though...

mock.Verify(m => m.Do(3), m => m.Do(2));

?

Not sure how the NotInSequence would look like, although I'm wondering where would I use that... wouldn't it just be a case of:

mock.Verify(m => m.Do(3), m => m.Do(It.Is(i => i != 3)));

?

@grzesiek-galezowski
Copy link

Daniel, thank you for the comments.

I expect the VerifyNotInSequence() to be used very sparingly, to say things like: after you commit a transaction, do not modify the record:

transaction.VerifyInSequence(t => t.Commit());
record.VerifyNotInSequence(r => r.SetKeyValue(It.IsAny<string>(), It.IsAny<string>()));

After I made changes on this fork, I made a trial by deploying the compiled assembly where I work and changing all the tests that need ordered verifications to use my solution. None of them needed this feature, so now I wonder whether it will be ever used. I can remove it from the code if there's no need for it.

As for the first question - that's a really good question and I thought about it for a long time. I think we both agree that if we want to use Verify() syntax for verifying calls order, we have to record the sequences somewhere. So, if I understand right, your doubts are about providing some kind of cleanup after each verification so that the next verification, whatever it might be, will work on the same "state" as the previous one. I can think of four approaches (in the end, I'll suggest an interesting alternative to what I put in the repository):

  1. As you suggested, approach like:

    mock.Verify(m => m.Do(3), m => m.Do(It.Is(i => i != 3)));

    which I think is hard to achieve, since it's common to verify call order for few mocks, so this kind of syntax would be difficult to use in such case.

  2. "verification scope" - this is the approach that an extension called Moq.Sequences uses (also, FakeItEasy uses this approach as well, look at: https://github.com/FakeItEasy/FakeItEasy/wiki/Ordered-assertions). It's basically about wrapping the ordered verifications in using(x) {} block. Personally, I tried to run away from it, however, it's still an option. Anyway, if we did this, the state change after each verification would not vanish completely, but rather it would be limited to inside of the scope.

  3. Implicit sequences with external verification feature provider - this is a little similar to what you suggested and it's what NSubstitute did in their experimental support for ordered verifications (http://nsubstitute.github.com/help/received-in-order/) and this is more or less like the previous approach. The only bigger difference I see is that if we succeeded at passing each verified call as an expression, we could hide the "internal state changes" completely.

  4. The approach I chose was "Registering explicit sequences". In other words, the mocks does not actually change its "internal state", but rather notifies a sequence that is registered using the "mock.CallSequence = x;" syntax as an observer. The confusion you're talking about might come from this that the communication with this "observer sequence" is hidden underneath Mock methods so it looks like the internal state of the mock is changing.

Thus, I'm thinking that an alternative might be to move the VerifyInSequence() methods to the sequence objects. This would require sharing some code between Mock and sequence objects, but would give us an opportunity to use syntax such as this:

var sequence = new LooseSequence();
var mock1 = new Mock<IRole1>() { CallSequence = sequence };
var mock2 = new Mock<IRole2>() { CallSequence = mock1.CallSequence };

mock1.Object.DoX();
mock2.Object.DoY();

sequence.On(mock1).Verify(m => m.DoX());
sequence.On(mock2).Verify(m => m.DoY());

The state of the sequence would still be changed after each Verify() (I think the only way to prevent it is to use some kind of scope as I mentioned in point 2 or providing a Rewind() method on the sequence that would reset the verification state).

These are the alternatives that I see. Looking forward to hear your thoughts!

@grzesiek-galezowski
Copy link

I tried to investigate the scope of verification a little and the state-modification issue. I think the only viable solution I can think for now is an API like this:

var sequence = new LooseSequence()
var mock = new Mock<IRole>() { CallSequence = sequence };
var mock2 = new Mock<IRole>() { CallSequence = mock.CallSequence };

mock.Object.Method1();
mock2.Object.Method2(234);
mock.Object.Method1();

sequence.VerifyInOrder(
  Call.To(mock, m => m.Method1()),
  Call.To(mock2, m => m.Method2(234)),
  Call.To(mock, m => m.Method1())
);

This can even be spiked on top of what I already implemented. In such case, the VerifyInOrder method would go something like this:

public void VerifyInOrder(params IVerificationStep[] steps)
{
  foreach(var step in steps)
  {
    step.Verify();
  }
  this.Rewind(); //go back to initial state
}

And Call.To() method would go like this:

public static IVerificationStep To<T>(Mock<T> mock, Expression<Action<T>> action) where T : class
{
  return new VerificationStep<T>(mock, action);
}

And the verification step would be:

class VerificationStep<T> : IVerificationStep where T : class
{
  Mock<T> _mock;
  Expression<Action<T>> _action;

  public VerificationStep (Mock<T> mock, Expression<Action<T>> action)
  {
    _mock = mock;
    _action = action;
  }

  public void Verify()
  {
    _mock.VerifyInSequence(_action);
  }
}

Of source, a better option than putting it on top of VerifyInSequence could be to do a little clean up before.

The only drawback when comparing this solution to what I currently have in the repository is that with what I presented below, we'd loose the exact line numbers where the verifications were requested in case of exception, but maybe we could somehow work around it using reflection and storing the relevant stack frame inside the VerificationStep instance...?

@grzesiek-galezowski
Copy link

Hi again,
I Actually found that even nicer syntax is possible using the above approach:

var sequence = new LooseSequence();
var mock = new Mock<IDependency> { CallSequence = sequence };
var mock2 = new Mock<IDependency> { CallSequence = mock.CallSequence };

mock.Object.Method1();
mock2.Object.Method2(234);
mock.Object.Method1();

sequence.VerifyReceivedInOrder(
  mock.CallTo(m => m.Method1()),
  mock2.CallTo(m => m.Method2(234)),
  mock.CallTo(m => m.Method1())
);

What do you think? When I have a little more spare time, I'll try to investigate how NSUbstitute achieved their single-lambda verification.

By the way, what do you think on the implicit vs explicit sequence? If we hid the sequence and made it implicit, the code could be even simpler:

var mock = new Mock<IDependency>();
var mock2 = new Mock<IDependency>();

mock.Object.Method1();
mock2.Object.Method2(234);
mock.Object.Method1();

Mock.ObjectsReceivedInOrder(
  mock.CallTo(m => m.Method1()),
  mock2.CallTo(m => m.Method2(234)),
  mock.CallTo(m => m.Method1())
);

or something like that. It would require us to decide which sequence to use by default and provide a way for mocks to share a sequence implicitly (are there any multi-threading constraints that make it harder?), but would further simplify the API. And multiple sequences per test is not something you need often.

@kzu
Copy link
Member

kzu commented Feb 23, 2013

that looks quite promissing :)

/kzu

Daniel Cazzulino

On Fri, Feb 22, 2013 at 5:33 PM, Grzegorz Gałęzowski <
notifications@github.com> wrote:

Hi again,
I Actually found that even nicer syntax is possible using the above
approach:

var sequence = new LooseSequence();
var mock = new Mock { CallSequence = sequence };
var mock2 = new Mock { CallSequence = mock.CallSequence };

mock.Object.Method1();
mock2.Object.Method2(234);
mock.Object.Method1();

sequence.VerifyReceivedInOrder(
mock.CallTo(m => m.Method1()),
mock2.CallTo(m => m.Method2(234)),
mock.CallTo(m => m.Method1())
);

What do you think? When I have a little more spare time, I'll try to
investigate how NSUbstitute achieved their single-lambda verification.


Reply to this email directly or view it on GitHubhttps://github.com//issues/21#issuecomment-13969842.

@grzesiek-galezowski
Copy link

I pushed a set of changes to the new API together with implementation of verifying Gets and Sets for properties (which was missing before). The changes can be observed at:
https://github.com/grzesiek-galezowski/moq4/blob/dev/UnitTests/VerifyInSequenceFixture.cs

Before I go further, I'd need to know your views on two topics:

  1. explicit vs implicit sequence (the current solution uses explicit sequence, however, we can change that easily)
  2. Leave vs throw out the verification that call was not made in sequence (for now I left it but didn't wrap it with the new API, because the fate of this feature is pending).

Best regards,
grzesiek

@Ireney
Copy link

Ireney commented Apr 3, 2013

I like the work Grzesiek has done on the sequences. I had no problem (ideological or otherwise) using strict behaviour to get my sequence tests working in the past, but support for loose behaviour would be welcome. That, and the ability to verify/assert the sequence/order, which falls more in line with the AAA testing pattern I subscribe to.

What are the chances this will be rolled into the project proper?

ib.

@grzesiek-galezowski
Copy link

Hi, Ireney, it all depends on Daniel. As you can see here, my latest questions remain unanswered for over three months...

@Robelind
Copy link

Why is there no further responses from maintainers on this issue?
The current implementation is terrible, requiring strict mocks to work and giving a totally misleading message when the expectation fails.

@kzu
Copy link
Member

kzu commented Aug 12, 2014

@Robelind I understand this might be important to you. In general, sequence-based testing is rare and might even be considered a smell as you might be testing too much of the internals of implementations.

In the many years since I created Moq, I haven't found a single project where using sequence verification was required. The current implementation was also contributed, and it seems to work for most users who need sequence-based testing (not that I have used it at all ever, as I said).

So, priority-wise it's pretty low.

Now, on to actual useful comment :)

@grzesiek-galezowski can you verify the senquence for anything but "in order"? If so, then sequence.VerifyReceivedInOrder might be unnecessarily verbose, and just Verify would be enough?

Also, how about flipping this around and making the sequence verification an overload of the Verify on the mock itself? Like:

moq.Verify(sequence, m => m.Method1());
moq2.Verify(sequence, m => m.Method2());

This could even support Times too like the other overload. The sequence would track where it's at in the verification steps and throw if order is missing.

Also, how about having a single MockSequence with the same MockBehavior.Strict|Loose we have now? This would be a more intuitive API than having two distinct sequence objects. This could internally set up whatever strategy is needed to throw or allow non-configured calls. This both a mock itself, and a mock sequence, are related to "mocks", MockBehavior would make sense still? Or should we have a SequenceBehavior instead?

@grzesiek-galezowski
Copy link

@kzu Thanks for the suggestions, will take a look at this. Looks like a lot has happened on the trunk, so I will have to swallow the changes as well...

While the sequencing behavior looks like a simple idea, it is more complicated than mock behavior. However, since the initial spike, I have come to conclusion that not all possible combinations of options are equally useful. We might get away with defining loose/strict sequence and maybe define an extension point for others to define theirs for less usual cases.

@grzesiek-galezowski
Copy link

By the way, which branch is the official one? a year ago, the dev was ahead, but it seems like master is ahead now.

@kzu
Copy link
Member

kzu commented Aug 12, 2014

Master, yes
On Aug 12, 2014 6:23 PM, "Grzegorz Gałęzowski" notifications@github.com
wrote:

By the way, which branch is the official one? a year ago, the dev was
ahead, but it seems like master is ahead now.


Reply to this email directly or view it on GitHub
#21 (comment).

@grzesiek-galezowski
Copy link

@kzu I looked into the proposed syntax and there are some issues.

The trick it that for the sequence to contain the information about the call order at the point of verification, the mocks must know which sequence object to report to when the calls happen.

Thus, to have a syntax like this:

moq.Verify(sequence, m => m.Method1());
moq2.Verify(sequence, m => m.Method2());

this sequence must be some kind of global (because at this point it has to contain the calls order from all the mocks) or registered with all the mocks anyway before all the calls happen (in which case the sequence passed to Verify would not matter anyway)... This is the reason why I have chosen the following syntax of registering a sequence in the first place:

var sequence = new LooseSequence();
var mock = new Mock<IDependency> { CallSequence = sequence };
var mock2 = new Mock<IDependency> { CallSequence = mock.CallSequence };

BUT, I think we can get away with specifying a default implicit sequence as I proposed earlier, with the ability of overwriting (with loose/strict sequence) - e.g. in NSubstitute the sequence is implicit and it fits most cases.

I also thought that if we had an implicit sequence, we could just say "switch the implicit sequence into strict/loose mode", passing the MockBehavior, but I have no idea which object it could be attached to (as the sequence is a cross-mock feature).

@kzu
Copy link
Member

kzu commented Aug 13, 2014

Alternatively, how about something like this:

sequence.Setup(mock, m => m.Method1())....

So the setups are done through the sequence to the mock. The first
parameter allows the mock to know what sequence is the setup being
configured with/for, and the second would be just the regular Moq API.

This still means you will only record as part of the sequence the things
you have explicitly set up, which is not ideal.

Passing a CallSequence property isn't a bad idea at all. Maybe that's
simpler. Also, maybe this is something that should also be exposed in the
MockRepository?

I think a single mock sequence (the implicit one) doesn't really make
sense. Isn't it typically the ordering of calls across mocks that matter
most?

/kzu

On Wed, Aug 13, 2014 at 3:12 PM, Grzegorz Gałęzowski <
notifications@github.com> wrote:

@kzu https://github.com/kzu I looked into the proposed syntax and there
are some issues.

The trick it that for the sequence to contain the information about the
call order at the point of verification, the mocks must know which sequence
object to report to when the calls happen.

Thus, to have a syntax like this:

moq.Verify(sequence, m => m.Method1());
moq2.Verify(sequence, m => m.Method2());

this sequence must be some kind of global (because at this point it has
to contain the calls order from all the mocks) or registered with all the
mocks anyway before all the calls happen (in which case the sequence
passed to Verify would not matter anyway)... This is the reason why I
have chosen the following syntax of registering a sequence in the first
place:

var sequence = new LooseSequence();
var mock = new Mock { CallSequence = sequence };
var mock2 = new Mock { CallSequence = mock.CallSequence };

BUT, I think we can get away with specifying a default implicit sequence
as I proposed earlier, with the ability of overwriting (with loose/strict
sequence) - e.g. in NSubstitute the sequence is implicit and it fits most
cases.

I also thought that if we had an implicit sequence, we could just say
"switch the implicit sequence into strict/loose mode", passing the
MockBehavior, but I have no idea which object it could be attached to (as
the sequence is a cross-mock feature).


Reply to this email directly or view it on GitHub
#21 (comment).

@regisbsb
Copy link

regisbsb commented Jun 9, 2015

@kzu should we close this now?

@akazemis
Copy link

I'd like to bring this issue back to the life cuz I reckon it would be a cool feature for Moq.
Call sequence would be handy in some scenarios. Say you want to test a domain service that calls a Repository class and we want to make sure that the SaveChanges() method runs after the other methods in the repository.

@grzesiek-galezowski
Copy link

@akazemis Hi, I worked a bit on this API, but got discouraged by a long lack of reaction from the maintainer and lost my motivation. Then, there has been some new development on the main branch and there are some conflicts between my branch and the main one. I don't know what is the size of this conflict. I can help however I can if somebody wants to take over the development or I can even resume the development if I know it will not be in vain this time.

@regisbsb
Copy link

regisbsb commented Dec 28, 2016 via email

@Ray-B
Copy link

Ray-B commented Sep 27, 2017

@akazemis I'm on the fence with this. How badly do people need this when there are suitable work-arounds like callbacks?

@Robelind You've never needed sequence verification? This is anecdotal evidence at best. And I don't say that to offend (I'm with you on it being a smell), but I think we need a better body of evidence than that.

I've seen instances where a Cache.Clear() might be called after something like Repo.SaveChanges(). You may want to verify your SUT clears the cache after and only after.

I will grant that a case can be made that sort of code might be indicative of a smell, but if it is, then the smell is the result a behavior on the part of the devs, and not implicity as a result of the capabilities of the mocking framework ;)

Sometimes as a developer, I just want to add some tests to some crappy legacy code someone else wrote in order to increase my confidence or understanding, and I don't have the time to do a sufficient job refactoring.

My two cents.

@firelizzard18
Copy link

Is this going to happen? Because I need to find another solution if it's not.

@stakx
Copy link
Contributor

stakx commented Jul 13, 2018

@michaelhidalgo, @grzesiek-galezowski, @Ireney, @Robelind, @regisbsb, @akazemis, @Ray-B, @firelizzard18:

I am finally closing this issue, and the pull request that goes along with it. Please see #642 for a brief explanation why I'm doing this.

I'd like to apologize in the name of the Moq team to @grzesiek-galezowski. I wasn't around 3 1/2 years ago, but back then you made a pretty big effort to drive this issue ahead, and just didn't get a response after a while. 😞 I for one appreciate your effort, call sequence verification was what originally brought me to Moq in the first place, and I'd have loved to see this enhancement fulfilled.

If anyone would like to pick up work on this issue, please post here and we'll see what we can do.

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

No branches or pull requests

10 participants