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

[Feature request] Loose mode for commands and strict for queries. #1056

Closed
voroninp opened this issue Sep 14, 2020 · 11 comments
Closed

[Feature request] Loose mode for commands and strict for queries. #1056

voroninp opened this issue Sep 14, 2020 · 11 comments

Comments

@voroninp
Copy link

Is it possible to add one more mocking mode, so void methods do not require setup and are just verified, yet methods which return value require setup?

Also for the methods which break CQS principle like Task<int> SaveChangesAsync() it would be nice to explicitly tell, that returned value does not matter:

contextMock.Setup(_ => _.SaveChangesAsync()).ReturnsWhatever();

So setup behaves as if it were a lose mock, but developer clearly sees that returned value does not matter.

@stakx
Copy link
Contributor

stakx commented Sep 14, 2020

Is it possible to add one more mocking mode, so void methods do not require setup and are just verified, yet methods which return value require setup?

You should be able to do that by implementimg a custom DefaultValueProvider that throws unconditionally, then setting your mock.DefaultValueProvider to an instance of that.

Also for the methods which break CQS principle like Task SaveChangesAsync() it would be nice to explicitly tell, that returned value does not matter

Same. You should be able to produce some default return value for Task<>.

@voroninp
Copy link
Author

voroninp commented Sep 14, 2020

@stakx

Same. You should be able to produce some default return value for Task<>

Doesn't it contradict to the unconditional throwing for any returned value?

@voroninp
Copy link
Author

voroninp commented Sep 15, 2020

@stakx It looks like DefaultValueProvider does not know for which method it is called. I'd be nice to tell in the exception message which call needs a setup. Yet I can extract it from stack frame, but it's not so performant.

@stakx
Copy link
Contributor

stakx commented Sep 15, 2020

looks like DefaultValueProvider does not know for which method it is called

@voroninp, that's right, while DefaultValueProvider has two methods GetDefaultParameterValue and GetDefaultReturnValue which would in theory allow you to find out where the default value will be used, those aren't currently called; Moq currently calls straight into the fallback GetDefaultValue method. This may change in a future version.

In your case, you might fare well enough by producing some sensible Task<> values and throw for everything else. This just might be good enough since Task objects appear in method signatures most frequently as return parameters, and much less often as formal in parameters.

(I didn't mean to provide you with the perfect solution, just take my above post as a starting point for your own experiments.)

@voroninp
Copy link
Author

voroninp commented Sep 17, 2020

Code, if anyone needs it later

sealed class AlwaysThrowingDefaultValueProvider : DefaultValueProvider
{
    protected override object GetDefaultValue(Type type, Mock mock)
    {
        // these two are equivalents of voids for async operations.
        // Formally we need to check the type is awaitable and result of operation is void.
        // But it's to much effort.
        if (type == typeof(Task))
        {
            return Task.CompletedTask;
        }
        if (type == typeof(ValueTask))
        {
            return new ValueTask();
        }

        var mockedType = mock.Object.GetType();
        var callLocation = new StackTrace()
        .GetFrames()
        .Select(_ => new { Method = _.GetMethod(), File = _.GetFileName(), Line = _.GetFileLineNumber() })
        .First(_ => _.Method.DeclaringType.IsAssignableFrom(mockedType));

        throw new QueryNotSetupException($"Call to {callLocation.Method.Name} must have setup.");
    }
}

sealed class QueryNotSetupException : Exception
{
    public QueryNotSetupException(string message) : base(message) { }
}

/// <summary>
/// Queries are methods without side effects but returning values. Commands are void methods with side effects.
/// Thus Queries should be strictly mocked, whereas Commands should be loosely mocked, that is be just verifiable without
/// requiring explicit setup.<br/><br/>
/// 
/// See: https://blog.ploeh.dk/2013/10/23/mocks-for-commands-stubs-for-queries/
/// </summary>
public static class StrictQueries
{
      /// <summary>
      /// Creates mock with loosely mocked Commands and strictly mocked Queries</c>.
      /// </summary>
      public static void Mock<T>(out Mock<T> mock) where T : class =>
          mock = new Mock<T>(MockBehavior.Loose)
          {
              DefaultValueProvider = new AlwaysThrowingDefaultValueProvider()
          };

      /// <summary>
      /// Creates mock with loosely mocked Commands and strictly mocked Queries</c>.
      /// </summary>
      public static void Mock<T>(out Mock<T> mock, Fixture fixture) where T : class
      {
          Mock(out mock);
          var obj = mock.Object;
          fixture.Register(() => obj);
      }
}

@voroninp
Copy link
Author

@stakx TBH, I'd like native Moq support for this mode. Though it works, as you suggested, failure messages are not that friendly compared to the ones generated by Moq itself.

@stakx
Copy link
Contributor

stakx commented Nov 17, 2020

I understand. However, this requirement is too specific to fit well into the core library.

Two things:

  • Turns out I lied to you above. Moq does call into DefaultValueProvider.GetDefaultReturnValue (which by default defers to GetDefaultValue) so you can find out quite easily which method got called.

  • If you want standard exceptions, then perhaps forget about a custom default value provider. What you could try instead is using strict mocks paired with a helper method that reflects over the mocked type and adds standard setups for all methods returning a [Value]Task. (If strict mocks are too strict, you could instead also add setups for [Value]Task<> that throw an exception.)

@voroninp
Copy link
Author

However, this requirement is too specific to fit well into the core library.

Do you mean loose commands, but strict queries? Or the part with Task and ValueTask?

@stakx
Copy link
Contributor

stakx commented Nov 17, 2020

Both. Concepts like queries and commands belong in a layer above Moq, they're too specific for a general mocking library. And while Moq has special support for tasks, your requirements are again too specific when combined to warrant a solution in the library.

@voroninp
Copy link
Author

Concepts like queries and commands belong in a layer above Moq, they're too specific for a general mocking library.

I agree it's a bit more abstract level, yet the tool could promote better practices so to say. ;-)

I see people using loose mocks not to bother with mocking voids - that is commands. But then it makes tests be a hell when one gets NullReferenceException after introducing yet another call which is expected to return some result - query, and Moq silently returns default.

@stakx
Copy link
Contributor

stakx commented Nov 18, 2020

That's what DefaultValue.Mock (or a custom default value provider) is for. But I think we're starting to go in circles.

the tool could promote better practices so to say. ;-)

No. Moq isn't anyone's nanny. It has a specific purpose: providing you with mock objects, mostly for use in tests. That's it. Promoting or even enforcing specific architectural patterns simply is out of scope.

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

No branches or pull requests

2 participants