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

Verify can create setups that causes a subsequent VerifyAll to fail #699

Closed
stakx opened this issue Sep 22, 2018 · 6 comments
Closed

Verify can create setups that causes a subsequent VerifyAll to fail #699

stakx opened this issue Sep 22, 2018 · 6 comments
Assignees
Labels
Milestone

Comments

@stakx
Copy link
Contributor

stakx commented Sep 22, 2018

Reproduction code:

public interface IX
{
    IX X { get; }
}

var mock = new Mock<IX>();
mock.Verify(m => m.X.X, Times.Never);
mock.VerifyAll();

Actual behavior:

VerifyAll throws a MockException with message:

The following setups on mock 'Mock<IX:00000001>' were not matched:
IX mock => mock.X

Expected behavior:

All verification steps should pass because there are no invocations nor any setups.

@stakx stakx added the bug label Sep 22, 2018
@lepijohnny
Copy link
Contributor

lepijohnny commented Feb 19, 2019

There are more cases affected by this one as well. I would like to share them as they are, IMO, all related with the way how FluentMockVisitor unfold fluent expression into inner mocks with setup even though Verify has been called, not Setup actually. It seems that Verify using fluent expression add (unexpected) side effects.

1. MochBehaviour.Strict has been avoided

Reproduction code:

public interface IX
{
    IX X { get; }
}

var mock = new Mock<IX>(MockBehavior.Strict);
mock.Verify(m => m.X.X, Times.Never);
var _ = mock.Object.X;

Actual behaviour

No exception

Expected behavior

MockException has been thrown as there is no setup for m.X

2. It is possible to invoke method\property without proper setup

Reproduction code:

public interface IX
{
    IX X { get; }
}

var mock = new Mock<IX>();
mock.Verify(m => m.X.X, Times.Never);
var _ = mock.Object.X.X;

Actual behaviour

No exception

Expected behavior

NullReferenceException has been thrown because we didn't setup return value for IX and it should be null.

3. Override the existing setup(IMO the most dangerous)

Reproduction code:

public interface IX
{
    IX X { get; }
   int Count { get; }
}

var mock = new Mock<IX>();
var nested = new Mock<IX>();
nested.Setup(m => m.Count).Returns(5);
mock.Setup(m => m.X).Returns(nested.Object);

mock.Verify(m => m.X.X, Times.Never);
int c = mock.Object.X.Count;

Actual behaviour

c is 0

Expected behavior

c is 5 as there is only one setup for it.

@lepijohnny
Copy link
Contributor

@stakx - FluentMockVisitor translate the fluent expression the same way, it doesn't matter if it is Setup or Verify. It will always, if inner mock is not there, create one, SetupAllProperties and add the Setup. This could lead to pollution of the mock and this vague behavior. What would be drawback of introducing additional info and passing it to FluentMockVisitor in order to avoid setup at all if verify is invoked?

@stakx
Copy link
Contributor Author

stakx commented Feb 21, 2019

@lepijohnny - I am already busy re-architecting the way how recursive / "multi-dot" setup and verification expressions are processed internally. Once I'm done with that, FluentMockVisitor will likely no longer be needed at all. ;-)

(I should probably mark this and some other issues as "assigned to myself" so others know that a bugfix is already being prepared.)

@stakx stakx self-assigned this Feb 21, 2019
@lepijohnny
Copy link
Contributor

Sure, makes sense 👍

@stakx
Copy link
Contributor Author

stakx commented Mar 3, 2019

@lepijohnny - Those bugs are gone now. I mentioned you in the changelog entry because you kindly provided a lot more test cases than I originally thought of; they've made it into the code base (here). Thanks for contributing those! 👍

@lepijohnny
Copy link
Contributor

lepijohnny commented Mar 4, 2019

@stakx - I appreciate that! Great job btw! 👍

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

No branches or pull requests

2 participants