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

Ability to mock requests from Protected methods in sequences #800

Open
bensmith009988 opened this issue Apr 29, 2024 · 8 comments · May be fixed by #845
Open

Ability to mock requests from Protected methods in sequences #800

bensmith009988 opened this issue Apr 29, 2024 · 8 comments · May be fixed by #845
Labels
feature-request Request for a new NSubstitute feature

Comments

@bensmith009988
Copy link

I fall into a category of people that has transitioned to using NSubstitute after discovering security concerns in Moq, which are detailed thoroughly in this article: https://medium.com/@michalsitek/critical-security-vulnerability-in-moq-4-20-0-ffd24739cc49.

So far, there has been only 1 feature of NSubstitute that was present in Moq, that is no longer possible: Mocking protected methods easily. Our company often writes unit tests for classes that make outbound http requests. In these classes that make the requests, we typically access the System.Net.Http.HttpClient via constructor injection. Mocking Http Requests is not exactly trivial with any mocking library, and usually involves some form of mocking the actual HttpMessageHandler, and building a new client with the mocked handler.

In Moq, you could skip all of this and simply mock the response of the protected method SendAsync in the unit test setup, by passing in a string value of the name of the protected method, as well as the request parameters to the method the mocked response is expecting. (Mocking the actual REST methods on the HttpClient doesn't actually work, because they all eventually call "SendAsync", which is the protected method we need to override). Here is an example of how simple it was to mock an Http request (regardless if it was Post, Put, Delete, or Get) looked like using Moq, because of the feature described:

var handlerMock = new Mock<HttpMessageHandler>();
            handlerMock.Protected()
                       .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
                       .ReturnsAsync(new HttpResponseMessage());

This was great because it offered a way to get granular with exactly what each Request message looked like, (by identifying requests from their outbound target address), and provide specific responses for each request. You could call "SetupSequence," and string together all of your specific requests and each specific response.

Because NSubsitute doesn't have a way to override protected methods like this, we are forced to mock the HttpClientFactory
CreateClient() method. To do this, we have a FakeHttpMessageHandler class that is used on top of the DelegatingHandler to override the protected method "SendAsync." While technically this is a workaround, and does work for a single outbound http request, it only works for 1 outbound http request. We can't mock multiple requests/responses to the SendAsync method. This matters because on methods where we have 3 and 4 outbound requests with specific logic that changes the cyclical complexity and logic routing, we lose the ability to write a whole suite of thorough test cases. We also can't specify WHICH specific incoming request should have WHICH specific response. We just know that eventually SendAsync will be called, and when it is, toss it this response.

You can see our FakeHttpMessageHanlder workaround below, and you will understand what I mean. We have attempted to come up with ways to inject multiple response, and sequencing, but have been unsuccessful at doing so. Additionally, we are having to include this workaround in every project we write tests for. We have over 130 repositories with LOTS of testing projects within. It is becoming cumbersome to maintain a workaround in tons of places. Please introduce this feature. It would save a lot of headache from your users. I know we are not alone, as the web is riddled with workarounds to this same problem. I have personally not seen a single implementation of a workaround that allows for getting granular on mocking sequences of requests/responses.

The FakeHttpMessageHandler that overrides the SendAsync method on the original DelegatingHandler:

public class FakeHttpMessageHandler : DelegatingHandler
{
    private HttpResponseMessage fakeResponse;

    public FakeHttpMessageHandler(HttpResponseMessage responseMessage)
    {
        fakeResponse = responseMessage;
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        return await Task.FromResult(fakeResponse);
    }
}

An example of how we use the FakeHttpMessageHandler, referenced in each of our tests that needs to mock a response to an outbound HttpRequest.

[Fact]
public async Task SomeMethod_ReturnsAPIResponse()
{
    //Arrange
    var expectedResponse = fixture.Create<SomeObject>();

    clientFactory.CreateClient(Arg.Any<string>())
                         .Returns(new HttpClient(SetupMessageHandler(HttpStatusCode.OK, expectedResponse)));

    UnitTestBuilder builder = new UnitTestBuilder()
        .Test<WebApiService>()
        .With(clientFactory)
        .Build();

    var service = builder.Services.GetRequiredService<WebApiService>();

    //Act
    var result = await service.SomeMethodWithArgs(fixture.Create<string>());

    //Assert
    result.Should().BeEquivalentTo(expectedResponse );
}

private static FakeHttpMessageHandler SetupMessageHandler(HttpStatusCode returnStatusCode, object returnObject)
{
    return new FakeHttpMessageHandler(new HttpResponseMessage
    {
        StatusCode = returnStatusCode,
        Content = new StringContent(JsonConvert.SerializeObject(returnObject), Encoding.UTF8, "application/json")
    });
}

Hopefully you are able to see the downsides in test cases without the ability to do what Moq provided. Hopefully the verbiage on this post made sense, but I am happy to clarify.

@304NotModified 304NotModified added the feature-request Request for a new NSubstitute feature label Apr 29, 2024
@304NotModified
Copy link
Contributor

Related #222 (with a working extension method? )

@Jason31569
Copy link

I have created an extension method to allow me to mock up protected virtual method for this exact same thing to test methods that involve rest api calls. It would be great if we could incorporate this into NSubstitute and not have to pull down yet another nuget pkg just to get the extension method

@dtchepak
Copy link
Member

Hi @Jason31569 ,

Would you be able to send a PR with this? Maybe add to NSubstitute.Extensions so people can opt-in?

@Jason31569
Copy link

Sure, but I am stump trying to make this generic enough that it can be used in all (or most) use cases

For example:
public static TResult Protected<T, TResult>(this T obj, string methodName, params object[] args) where T : class

When:
sub.Protected<AnotherClass, string>("ProtectedMethod", Arg.Any<string>(), Arg.Any<int>(), Arg.Any<char>()).Returns(expectedMsg);

It seems for all ref types NSubstitute sets the arg to null, e.g.args[0] == null

Is there a clean way I can detect the correct type (in this case string)?

@dtchepak
Copy link
Member

@Jason31569 I think you should just be able to call methodName and pass args. The underlying substitute will handle the null values and attempt to inject the queued arg matchers.

I'm not sure there is much point in making the return type T. 🤔 I think we can just make it object and let people attempt to return any type. I don't think it's providing much type safety either way; people can mistakenly specify T or mistakenly pass the wrong type to Returns.

(If I've missed the point of the question here please let me know!)

@Jason31569
Copy link

Jason31569 commented Nov 11, 2024

The problem is the extension method cannot figure out the Type[] for args being passed

Given this class with 3 overloads:

protected virtual string ProtectedMethod()

protected virtual string ProtectedMethod(int i)

protected virtual string ProtectedMethod(string msg, int i, char j)

I am doing this to find the right method (maybe there is a better way?):
MethodInfo mthdInfo = obj.GetType().GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Instance, Type.DefaultBinder, args.Select(x => x.GetType()).ToArray(), null);

Unit test with var sub = Substitute.For<AnotherClass>():
sub.Protected("ProtectedMethod", Arg.Any<string>(), Arg.Any<int>(), Arg.Any<char>()).Returns(expectedMsg); throws null ref exception because Arg.Any<string>() throws null ref exception at args.Select( x => x.GetType())

Not to distract from the original issue I ran into, but I also found out that sub.Protected("ProtectedMethod", "worker", Arg.Any<int>(), Arg.Any<char>()).Returns(expectedMsg); doesn't seem to setup the return correctly. I am getting empty string result instead of expectedMsg. Maybe it's a non-issue because I am mixing actual value with Arg.Any here?

This works fine (with exact values):
sub.Protected("ProtectedMethod", "worker", 3, 'x').Returns(expectedMsg);

re the return type T - nice, I thought I would have to deal with casting return time in my tests if I didn't define the generic return type. Returning an object makes the unit tests cleaner

@dtchepak
Copy link
Member

For working out arg matcher types it is probably necessary to check queued argument matchers (some discussion in this thread.

Alternatively maybe it is possible to use InvokeMember to skip the lookup step? (untested, just an idea)

@Jason31569
Copy link

Jason31569 commented Nov 20, 2024

@dtchepak let me know your thoughts on the changes?

Didn't go the route of InvokeMember because it would fail without a useful error msg if the user accidentally tried to mock a non-virtual method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for a new NSubstitute feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants