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

VerifySet fails on non-trivial property setter #430

Closed
TimothyHayes opened this issue Jul 28, 2017 · 10 comments · Fixed by #782
Closed

VerifySet fails on non-trivial property setter #430

TimothyHayes opened this issue Jul 28, 2017 · 10 comments · Fixed by #782
Labels

Comments

@TimothyHayes
Copy link

TimothyHayes commented Jul 28, 2017

When I attempt to use a VerifySet on a property that refers to another property within the setter, Moq throws Expression is not a property setter invocation.

Here is the sample class. Notice the use of the IsMale property within the Antlers setter:

public class Vixen
{

    public Vixen(bool pIsMale)
    {
        IsMale = pIsMale;
    }

    private bool _IsMale;
    public virtual bool IsMale
    {
        get { return this._IsMale; }
        private set { this._IsMale = value; }
    }

    private bool _Antlers;
    public virtual bool Antlers
    {
        get { return this._Antlers; }
        set
        {
            // females cannot have antlers
            if (IsMale)
                this._Antlers = value;
            else
                this._Antlers = false;
        }
    }
}

Here are the unit tests. Each one throws the same Exception noted above:

public class VixenTests
{
    [Fact]
    public void Antlers_NoSetup()
    {
        // Arrange

        // create mock of class under test
        var sut = new Mock<Vixen>(true) { CallBase = true };

        // Act
        sut.Object.Antlers = true;

        // Assert
        sut.VerifySet(x => x.Antlers = true);
    }

    [Fact]
    public void Antlers_SetupProperty()
    {
        // Arrange

        // create mock of class under test
        var sut = new Mock<Vixen>(true) { CallBase = true };
        sut.SetupProperty(x => x.Antlers, false);

        // Act
        sut.Object.Antlers = true;

        // Assert
        sut.VerifySet(x => x.Antlers = true);
    }

    [Fact]
    public void Antlers_SetupSet()
    {
        // Arrange

        // create mock of class under test
        var sut = new Mock<Vixen>(true) { CallBase = true };
        sut.SetupSet(x => x.Antlers = true);

        // Act
        sut.Object.Antlers = true;

        // Assert
        sut.VerifySet(x => x.Antlers = true);
    }
}

I posted this problem on StackOverflow and received an explanation from user Jason Boyd. Check here for that link.

Can the VerifySet function be improved to allow for non-trivial property setters?

@stakx
Copy link
Contributor

stakx commented Jul 28, 2017

@TimothyHayes, thanks for reporting this bug. I've added this issue to a fairly long list (see #414) of problems caused by FluentMockContext, a component that Moq uses internally to "dry-run" verify or setup delegates to analyse them.

To understand what this component does, take a look at the signature of the sut.VerifySet method you're calling: it accepts an Action<T>, not an Expression<Action<T>>. That is, Moq cannot simply inspect the property set you've passed to VerifySet to see what it should verify. What Moq does is to activate a recorder-like mode (FluentMockContext), then it invokes your verify action. All calls on the proxy will be recorded, Moq will then inspect those. Unfortunately, in your case, FluentMockContext records too much due to CallBase being set.

A quick hacky fix would be possible in your case. Instead of checking the last invocation, Moq could be changed to check the last property setter invocation. I cannot say whether such a fix would break less tests than it fixes, which is why I am reluctant to apply that fix. A better, but still somewhat hacky fix might be to change the recorder to only record calls made directly from the verify lambda. This might be worth following up.

To really solve your issue, FluentMockContext probably needs to be replaced with something else altogether. I am currently investigating the second option mentioned in #218 (comment), i.e. delegate decompilation, which would be the most powerful and preserve Moq's current usage patterns. This would allow Moq to rebuild an Expression<Action<T>> from an Action<T> (as passed to VerifySet); then it could inspect the rebuilt expression tree. (A proof of concept for this idea can be found at stakx/DelegateDecompiler, which already works quite well, but it isn't production-ready yet.)

@TimothyHayes
Copy link
Author

Thank you for pursuing. I have another potential work-around. The FluentMockContext appears to execute the actual property setter even if a SetupProperty is present on the property. If FluentMockContext could execute against the mocked version due to the SetupProperty, then, at least in many cases, the developer could prove that the property setter was called with a specific value without actually executing base-implemented code, even in the VerifySet. Good Luck!

@TimothyHayes
Copy link
Author

Sorry, did not understand close and comment.

@TimothyHayes TimothyHayes reopened this Jul 31, 2017
@Alezis
Copy link

Alezis commented Aug 7, 2017

I'm not sure is it the same issue or not (or event is it Moq issue), but decided to post here.
Have dll with 41 tests. Using xUnit. I got error on build machine for only one test. The same test is working on my local PC, but test agent that runs tests via "dotnet test ..." fails.
This test was added recently and is used VerifySet. Spent a lot of time investigating what's going on and found that time to time I got this test failed. For example I run tests 3 times (with the same "dotnet test ..." command): 2 times - all tests passed, 1 time - one test failed. It occurs sporadically.
Error message is: Expected invocation on the mock at least once, but was never performed: resp => resp.StatusCode = 401.
I checked few times, all dlls produced by compiled are the same - identical, I checked with byte to byte comparison.
And this is the test method (I renamed/skipped a bit):

[Fact]
public async Task Invoke_ArgumentIsEmpty_401ErrorReturned()
{
	//arrange
	var aad = "599782ED-4A2C-4A59-9E29-A84560FEEF29";
	var tenantC = new Mock<ITenantC>();
	tenantC .Setup(c => c.GetIdAsync(aad)).Returns(Task.FromResult(String.Empty));

	var fakeMiddlewareMock = new Mock<OwinMiddleware>(null);
	var owinContextMock = new Mock<IOwinContext>();
	var owinRequestMock = new Mock<IOwinRequest>();
	var owinResponseMock = new Mock<IOwinResponse>();
	var userPrincipalMock = new Mock<IPrincipal>();

	userPrincipalMock.Setup(u => u.Identity).Returns(new ClaimsIdentity(new[] { new Claim("iss", $"https://sts.windows.net/{aad}/") }));
	owinRequestMock.Setup(r => r.User).Returns(userPrincipalMock.Object);
	owinContextMock.Setup(c => c.Request).Returns(owinRequestMock.Object);
	owinContextMock.Setup(c => c.Response).Returns(owinResponseMock.Object);
	owinContextMock.Setup(c => c.Environment).Returns(new Dictionary<string, object>());
	owinResponseMock.SetupProperty(resp => resp.StatusCode);

	var options = new MyMiddlewareOptions()
	{
		CacheTimeout = new TimeSpan(0, 0, 0, 0, 1)
	};
	var middleware = new MyMiddleware(fakeMiddlewareMock.Object, tenantC.Object, options);

	//act
	await middleware.Invoke(owinContextMock.Object);

	//assert
	owinResponseMock.VerifySet(resp => resp.StatusCode = 401);
}

public class MyMiddleware : AbstractMiddeware
{
	// [Skipped code]
	
	public override async  Task Invoke(IOwinContext context)
	{
		// [Skipped code]
		
		var tenantId = await _tenantC.GetIdAsync(_aad);
		if (string.IsNullOrWhiteSpace(tenantId))
		{
			await ProcessError(context, "Tenant is empty.");
			return;
		}

		// [Skipped code]
	}
	
	// [Skipped code]
}

public abstract class AbstractMiddeware : OwinMiddleware
{
	// [Skipped code]


	internal async Task ProcessError(IOwinContext owinContext, string errorMessage)
	{
		var failedContext = new MiddlewareErrorContext()
		{
			OwinContext = owinContext,
			Options = _options,
			ErrorMessage = errorMessage
		};

		await ProcessMiddlewareErrorContext(failedContext);

		if (failedContext.IsSkipped)
		{
			await Next.Invoke(owinContext);
			return;
		}

		if (failedContext.IsHandled)
		{
			return;
		}

		owinContext.Response.StatusCode = 401;  /// HERE  I SET PROPERTY. ALL OBJECTS HERE ARE MOCKED
		await owinContext.Response.WriteAsync(errorMessage);
	}
}

@Alezis
Copy link

Alezis commented Aug 8, 2017

Well, I don't know whats wrong with my code. I cleared out my test project and remained with 8 tests. Got the same error. Run with "dotnet test ... --no-build" fails almost always, but sometimes all tests passed. I tried run dll with "xunit.console.exe ..." and all tests passed always. I tried 20 times - nothing. It brings me to thought that may be bug in this link dotnet -> xunit.

@stakx
Copy link
Contributor

stakx commented Aug 8, 2017

@Alezis: It would've been easier if you had posted that as a separate issue so there wouldn't be the risk of hijacking the OP's issue. I've just quickly glanced over your code, you have a very low timeout (1 millisecond) in there. Given that your tests sometimes pass and sometimes fail, I would suspect that the timeout might have something to do with it. If this timeout is honored at all, perhaps try increasing it to a higher value (such as 1 second).

@Alezis
Copy link

Alezis commented Aug 8, 2017

@stakx. I should apologize. I found that cache affects my tests. You are right, if I set this to 1 second or even more I could find bug more easily. From other view I would not rewrite my caching logic and improve code )). Anyway, I fixed everything. Tests pass. Should I delete my previous comments ?

@stakx
Copy link
Contributor

stakx commented Aug 8, 2017

No, leave your posts as they are, that's OK. :) I would only ask that if you do need more assistance with the use of Moq in your code, then open a new issue. Glad you could fix the immediate problem!

@stakx
Copy link
Contributor

stakx commented Jul 13, 2018

Closing this, this problem will be tracked in #414 (together with a couple other related issues).

@stakx stakx closed this as completed Jul 13, 2018
@stakx
Copy link
Contributor

stakx commented Mar 10, 2019

The test cases at the beginning of this issue reveal a regresion in current HEAD, more specifically SetupSet, VerifySet et al. (delegate-based methods) will no longer work for mock types that don't have a parameterless ctor. This should be fixed.

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