Repository navigation
Add VerifyNoOtherCalls to MockFactory - #682
Conversation
There was a problem hiding this comment.
Thanks for your PR. Could you please take another look at the following few things? Also, could you please add an entry to CHANGELOG.md (under the Unpublished → Added heading) that briefly summarises this enhancement?
| /// <exception cref="MockException">One or more mocks had setups that were not satisfied.</exception> | ||
| public virtual void VerifyNoOtherCalls() | ||
| { | ||
| VerifyMocks(Mock.VerifyNoOtherCalls); |
There was a problem hiding this comment.
I suspect that this call won't do proper error aggregation. Let's say I'm adding another test for a repository with more than one mock:
[Fact]
public void VerifyNoOtherCalls_should_correctly_aggregate_unmatched_calls_from_more_than_one_mock()
{
var repository = new MockRepository(MockBehavior.Default);
repository.Create<IFoo>().Object.Do();
repository.Create<IBar>().Object.Redo();
var ex = Record.Exception(() => repository.VerifyNoOtherCalls());
Assert.Contains("The following invocations were not verified:", ex.Message);
Assert.Contains("IFoo.Do()", ex.Message);
Assert.Contains("IBar.Redo()", ex.Message);
}This test will fail because the error message is this:
The following invocations were not verified:
IFoo.Do()
No mention of the mock2.Redo() call.
Could you please add a test such as the above and look into error aggregation for VerifyNoOtherCalls? Hint: It might be enough to change the following line:
to ... when (error.Reason == MockExceptionReason.UnmatchedSetups || MockExceptionReason.UnverifiedInvocations).
As an inspiration for what properly aggregated errors might look like, here's how errors are aggregated for e.g. VerifyAll. Take this code:
var repo = new MockRepository(MockBehavior.Default);
repo.Create<IFoo>().Setup(m => m.Do());
repo.Create<IBar>().Setup(m => m.Redo());
repo.VerifyAll();This will produce an error message such as the following:
The following setups on mock 'Mock<IFoo:00000001>' were not matched:
IFoo m => m.Do()
The following setups on mock 'Mock<IBar:00000001>' were not matched:
IBar m => m.Redo()
(This is basically a concatenation of the each "failed" mock's error message, with a blank line in-between.)
There was a problem hiding this comment.
This threw up an interesting issue of multiple types of exception needing to be grouped, so i added a new exception reason for this.
There was a problem hiding this comment.
Thanks for the changes made so far. I have a few more. 🦄 The main issue here is with the way error aggregation is done. I think it'd be better not to make aggregation in VerifyMocks more general using a new MockExceptionReason.MultipleVerificationErrors. Instead, just perform specific aggregation right in MockFactory.VerifyNoOtherCalls.
That being said, greatly I appreciate the effort you made! Keep up the good work! 👍
| string.Format( | ||
| CultureInfo.CurrentCulture, | ||
| Resources.UnverifiedInvocations, | ||
| string.Join(Environment.NewLine, errors.Select(error => error.Message)))); |
There was a problem hiding this comment.
There's two problems here:
-
Using
string.FormatonResources.UnverifiedInvocationshere will lead to duplicated text in the error message, since the same resource string will already be included at the beginning of eacherror.Message.It would be better to follow the pattern that can be seen in the
MockException.UnmatchedSetups(IEnumerable<MockException> errors)method:return new MockException( MockExceptionReason.UnverifiedInvocations, string.Join( Environment.NewLine, errors.Select(error => error.Message)));
-
It would be important to include the failed mock's name in each original
error.Messageso that when multiple error messages are concatenated, the user can still tell which reported failures were caused by which mocks. This could be done as follows:-
First, change the value of
Resources.UnverifiedInvocationsso it includes a placeholder for the mock's name:The following invocations on mock {0} were not verified:\n{1}. -
In the
MockException.UnverifiedInvocations(IEnumerable<Invocation> invocations)method, include the mock's name whenstring.Format-ting (which means that method needs an additional parameter for the mock:string.Format( CultureInfo.CurrentCulture, Resources.UnverifiedInvocations, mock.ToString(), string.Join(Environment.NewLine, invocations)));
-
| <value>The following invocations were not verified: | ||
| {0}</value> | ||
| </data> | ||
| <data name="MultipleVerificationErrors" xml:space="preserve"> |
There was a problem hiding this comment.
Please remove this, along with the change in Resources.Designer.cs.
Instead, as mentioned in another review comment, change the value of UnverifiedInvocations (right above these lines) to include a placeholder for the mock's name. (The following invocations on mock {0} were not...; see other review comment for details.)
|
@stakx I see where your coming from and I too was dubious over the way the usage was going, so I'll revert a bit and implement the aggregate exception nicer. |
|
@stakx all updated (i rebased with a squash to clean the history, hope that's OK) |
|
@BlythMeister - Rebasing, squashing, fixing up, etc. your PR to keep it clean and tidy is very much appreciated! 👍 Will review again in a bit. |
There was a problem hiding this comment.
Thanks again for making (most of) the requested changes. There's just one bit missing now.
Let's say I do the following:
public interface IFoo
{
void Do();
}
public interface IBar
{
void Redo();
}
var repo = new MockRepository(MockBehavior.Default);
repo.Create<IFoo>().Object.Do();
repo.Create<IBar>().Object.Redo();
repo.VerifyNoOtherCalls();I will get the following verification error message:
The following invocations were not verified:
IFoo.Do()
The following invocations were not verified:
IBar.Redo()
It should read as follows instead:
The following invocations on mock 'Mock<IFoo:00000001>' were not verified:
IFoo.Do()
The following invocations on mock 'Mock<IBar:00000001>' were not verified:
IBar.Redo()
(I didn't leave a review comment about it but of course this will require a small change in the last test you're adding.)
Please see the two unresolved comments in my previous review for some hints how to achieve this.
Thanks for the great work so far!
|
Thank you for keeping bearing with me, I really appreciate it, and have already learnt a lot about moq! |
|
@stakx updates made, i did also update the test for the verify to make it match the new test which ensures the full error message is correct. |
|
@BlythMeister - Thanks! Did you see the two test failures? (They appear to be failing due to different line endings (CRLF vs LF). Instead of using (Btw. when you look at the AppVeyor log, you might see some � glyphs; never mind those, that's likely just a display / encoding problem on AppVeyor.) |
There was a problem hiding this comment.
Almost there! Just the two tests failing on AppVeyor. Should be easily fixed, however.
|
@stakx hopefully there now! |
There was a problem hiding this comment.
AppVeyor is still complaining, but I'm going to merge this anyway. The thing is, this isn't the first time that this problem has shown up, and I'd like to finally create a proper assertion helper for comparing multi-line strings instead of off-loading this work on you (it's really a problem unrelated to this single PR). I'll fix this remaining issue in a separate step, hope that's OK with you.
Thanks a lot for the great work! 👍 It's been a joy working on this PR with you. You've responded very quickly to reviews and even taken the time to tidy up the PR intermittently. Couldn't ask for more!
|
@BlythMeister - I've just pushed Moq 4.10.0 (which includes your addition) to NuGet. It should be available shortly. |
Enables single call to verify no other calls on all mocks generated by the factory
Based on the discussion with @stakx in #675 it appeared that i was not using the framework correctly.
The reason i chose to do it this was way the inability to perform a
VerifyNoOtherCallson all mocks generated from a single factory.This new method means you can create many mocks from a single factory, verify each expectation in turn and then at the end call
Factory.VerifyNoOtherCallsto ensure that only the items you have verified occurred for every mock generated in a given test.Current rather nasty workaround