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

System.InvalidOperationException because of call to Verifiable in MockSequence after 4.14.0 #1114

Closed
Lyra2108 opened this issue Dec 8, 2020 · 2 comments · Fixed by #1121
Closed
Assignees
Labels
Milestone

Comments

@Lyra2108
Copy link

Lyra2108 commented Dec 8, 2020

Failure description

I updated Moq from 4.13.1 to 4.15.2. This causes some of our tests to fail with the following exception:
System.InvalidOperationException : This call to 'Verifiable' will have no effect because conditional setups are ignored by both 'Verify' and 'VerifyAll'. This might indicate an error in your code.

This exception seems to be added with 4.14.0 (or to be more precise #997 ). To me it seems to be an error, because the code worked fine before the update here an example of a sequence test (with NUnit):

[Test]
public void TestMockSequence()
{
	var myMock = new Mock<MyClass>();
	var myTestObject = new MyTestObject(myMock.Object);
	var sequence = new MockSequence {Cyclic = false};
	myMock.InSequence(sequence).Setup(x => x.FirstCall()).Verifiable();
	myMock.InSequence(sequence).Setup(x => x.SecondCall()).Verifiable();

	myTestObject.TestMe();

	myMock.Verify();
	myMock.VerifyNoOtherCalls();
}

public class MyTestObject
{
	private readonly MyClass _myMockObject;

	public MyTestObject(MyClass myMockObject)
	{
		_myMockObject = myMockObject;
	}

	public void TestMe()
	{
		_myMockObject.FirstCall();
		_myMockObject.SecondCall();
	}
}

public class MyClass
{
	public virtual void FirstCall()
	{
	}

	public virtual void SecondCall()
	{
	}
}

With 4.13.1 it works as expected, if the order is changed the test fails otherwise it passes.
The line which does the validation is:

myMock.VerifyNoOtherCalls();

Which might be missleading, because on the first glace it looks like it is the verify line. But when I have a look at the Quickstart on MockSequence it seems to be the way this works because there it is added to a "Strict"-Mock.

Failed workaround

If I remove the .Verifiable() like suggested in the error message and the update notes. VerifyNoOtherCalls failes because it does not consider the sequence set up as a "real" set up.

Workaround

Changing the mock to a strict-mock and removing the verifiable from the calles does work.

This was not done in the test before because the mock is used for other test cases where no strict setup was requiered.

Expected behavoir

Either VerifyNoOtherCalls() should not fail if a set up is done in a sequence w/o .Verifiable() or .Verifiable() should be allowed for sequence tests.

@stakx stakx added the bug label Dec 8, 2020
@stakx
Copy link
Contributor

stakx commented Dec 8, 2020

Thanks for reporting. This appears to be a regression introduced by db0f208. I'll look into it soon.

@stakx stakx self-assigned this Dec 28, 2020
@stakx stakx added this to the 4.15.3 milestone Dec 28, 2020
@stakx
Copy link
Contributor

stakx commented Dec 28, 2020

It appears we need to undo #997 in order to fix this regression.

@stakx stakx modified the milestones: 4.15.3, 4.16.0 Jan 1, 2021
mburumaxwell pushed a commit to faluapp/falu-dotnet that referenced this issue Jun 12, 2021
Bumps [Moq](https://github.com/moq/moq4) from 4.15.2 to 4.16.0.

#Changelog

*Sourced from [Moq's changelog](https://github.com/moq/moq4/blob/main/CHANGELOG.md).*

> ## 4.16.0 (2021-01-16)
>
> #### Added
>
> * Ability to directly set up the `.Result` of tasks and value tasks, which makes setup expressions more uniform by rendering dedicated async verbs like `.ReturnsAsync`, `.ThrowsAsync`, etc. unnecessary:
>
>    ```diff
>    -mock.Setup(x => x.GetFooAsync()).ReturnsAsync(foo)
>    +mock.Setup(x => x.GetFooAsync().Result).Returns(foo)
>    ```
>
>    This is useful in places where there currently aren't any such async verbs at all:
>
>    ```diff
>    -Mock.Of<X>(x => x.GetFooAsync() == Task.FromResult(foo))
>    +Mock.Of<X>(x => x.GetFooAsync().Result == foo)
>    ```
>
>    This also allows recursive setups / method chaining across async calls inside a single setup expression:
>
>    ```diff
>    -mock.Setup(x => x.GetFooAsync()).ReturnsAsync(Mock.Of<IFoo>(f => f.Bar == bar))
>    +mock.Setup(x => x.GetFooAsync().Result.Bar).Returns(bar)
>    ```
>
>    or, with only `Mock.Of`:
>
>    ```diff
>    -Mock.Of<X>(x => x.GetFooAsync() == Task.FromResult(Mock.Of<IFoo>(f => f.Bar == bar)))
>    +Mock.Of<X>(x => x.GetFooAsync().Result.Bar == bar)
>    ```
>
>    This should work in all principal setup methods (`Mock.Of`, `mock.Setup…`, `mock.Verify…`). Support in `mock.Protected()` and for custom awaitable types may be added in the future. (@stakx, [#1126](devlooped/moq#1126))
>
> #### Changed
>
> * Attempts to mark conditionals setup as verifiable are once again allowed; it turns out that forbidding it (as was done in [#997](devlooped/moq#997) for version 4.14.0) is in fact a regression. (@stakx, [#1121](devlooped/moq#1121))
>
> #### Fixed
>
> * Performance regression: Adding setups to a mock becomes slower with each setup (@CeesKaas, [#1110](devlooped/moq#1110))
>
> * Regression: `mock.Verify[All]` no longer marks invocations as verified if they were matched by conditional setups. (@Lyra2108, [#1114](devlooped/moq#1114))

#Commits

- [`74d5863`](devlooped/moq@74d5863) Update version to 4.16.0
- [`424fe31`](devlooped/moq@424fe31) Fix typo in changelog
- [`f48c0f4`](devlooped/moq@f48c0f4) Merge pull request [#1126](devlooped/moq#1126) from stakx/setup-task-result
- [`6f6a89d`](devlooped/moq@6f6a89d) Update the changelog
- [`66bcb21`](devlooped/moq@66bcb21) Enable `task.Result` in delegate-based setup methods
- [`42521c4`](devlooped/moq@42521c4) Add ability in `IAwaitableFactory` to create result...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants