[RFC] IJsonSerializer parameter in Connection.cs wasn't being used

[devkhan]

Passing on serialize argument to Connection's ctor, wasn't passing on before. Breaks tests Updated tests. Cannot merge Merged.

Refer #1119

This breaks six tests in total, one of them being this, I guess that's because, the IJsonSerializer in the test is a substitute, it doesn't actually provide a serialization strategy. I don't know much about unit testing and mocking frameworks.

@shiftkey @M-Zuber thoughts?

[M-Zuber]

The failing tests seem to be due to the fact that they pass in Substitute.For<IJsonSerializer>().
I am attempting to debug further to try and understand what the fix should be.

[devkhan]

Yes, in fact it is because of it. I tried passing an actual object of SimpleJsonSerializer, and the test passed.

Can we pass an actual object, instead of a substitute?

[M-Zuber]

That would have to be the call of @haacked or @shiftkey as they should be more familiar with what exactly is going on in the test.

And the truth is, it does not make sense for that change to be breaking the test anyways so I would vote for some more investigation first.

[devkhan]

I'm ok with whatever you want to do. Can you tell me where should I look into to learn further more about this?

[M-Zuber]

I'm honestly not sure myself...
Do you know if there is a way to set a breakpoint within the lamda on line 322?

[devkhan]

I don't think we can break at one condition, but if we break down the compound && condition into several lines, we can surely put breakpoints on each one of them.

[M-Zuber]

That gave me an idea, I'll get back to you in a bit.

[M-Zuber]

TL;DR use the substitute also for setting up the data.
Instead of this I wrote this:

 var serializer = Substitute.For<IJsonSerializer>();
string data = serializer.Serialize(new object());
var httpClient = Substitute.For<IHttpClient>();
IResponse response = new Response();
 httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response));
var connection = new Connection(new ProductHeaderValue("OctokitTests"),
      _exampleUri,
      Substitute.For<ICredentialStore>(),
      httpClient,
      serializer);

[devkhan]

It is a clever trick, but due to lack of experience, I can't say whether we should use this or not. @shiftkey, feedback?

[ryangribble]

Hey @devkhan thanks for contributing the fix and this is a great opportunity for you to gain experience in the area of open source contributions and familiarity with mock based unit testing! :)

The purpose of the Octokit.net unit tests is to verify expected behaviour but run totally self contained locally (ie not calling out to any github.com api calls and the like). So the NSubstitute mocks are used to mock out any of these expensive/external API calls so that the tests run quickly and against controlled data.

Mocks help us achieve the Unit Testing "Arrange, Act, Assert" pattern.

• Arrange is where we setup our input data and configure our mocks
  (ie, "when you receive a call that looks like this, reply with this data")
• Act is where we make the call we are actually testing
• Assert is where we then use our mocks or the returned data to make various assertions about what we expected to happen (which throw an exception and fail the test if the conditions we test for didnt occur)
  (ie, "assert that when I called the Users.GetAll() client call, the underlying httpConnection received a Get<User>(...) call with parameters that contain specific values we wanted (like the expectedUrl, input parameters etc)"

So anyway, the reason those tests are now failing is that the substituted IJsonSerializer is now correctly passed through thanks to your fix, meaning that down in the guts of constructing the http call to the API, the "data" of the response body is null (because the substitute hasnt been configured to return any data when Serialize() is called on it). This means that the Assert on httpClient.Send() is failing, since it did not receive a call that matched what we specified (specifically, the body data is now null rather than it being "" as expected)

@M-Zuber 's example is setting the expected body data to be the (null) return of the mocked serializer... which does make the test pass, however I would probably recommend instead to configure the mock to return the data we want since that is more in keeping with the unit testing approach and would also be applicable in other situations where we had a "real" body instead of a new object() as in this case.

We can also add an assert to ensure that the IJsonSerializer Received() 1 call to Serialize() thus proving it was correctly passed through.

Does that give you enough information @devkhan to tackle correcting the failing unit tests yourself?

To give an example of how I would probably fix these unit tests (explanatory comments like this aren't required, im just adding them to walk you through what's happening):

[Fact]
public async Task RunsConfiguredAppWithAppropriateEnv()
{
    // Arrange

    // - Setup request body and expected data
    var body = new object();
    string expectedData = SimpleJson.SerializeObject(body);

    // - Setup IJsonSerializer mock to return expectedData when Serialize() is called
    var serializer = Substitute.For<IJsonSerializer>();
    serializer.Serialize(body)
        .Returns(expectedData);

    // - Setup IHttpClient mock to return a response object when Send() is called
    IResponse response = new Response();
    var httpClient = Substitute.For<IHttpClient>();
    httpClient.Send(Args.Request, Args.CancellationToken)
        .Returns(Task.FromResult(response));

    // Act

    // - Create our connection object
    var connection = new Connection(new ProductHeaderValue("OctokitTests"),
        _exampleUri,
        Substitute.For<ICredentialStore>(),
        httpClient,
        serializer);

    // - Make the Patch() call
    await connection.Patch<string>(new Uri("endpoint", UriKind.Relative), body);

    // Assert

    // - Verify JsonSerializer.serialize() was called with body object
    serializer.Received(1).Serialize(body);

    // - Verify HttpClient.Send() was called with all the correct arguments
    httpClient.Received(1).Send(Arg.Is<IRequest>(req =>
        req.BaseAddress == _exampleUri &&
        (string)req.Body == expectedData &&
        req.Method == HttpVerb.Patch &&
        req.ContentType == "application/x-www-form-urlencoded" &&
        req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken);
}

[shiftkey]

To follow up on @ryangribble's summary, if they're not testing the actual serialized result (which I'm pretty sure is most of them) we can just simplify things to indicate that we don't care. For example:

[Fact]
public async Task RunsConfiguredAppWithAppropriateEnv()
{
    var httpClient = Substitute.For<IHttpClient>();
    IResponse response = new Response();
    httpClient.Send(Args.Request, Args.CancellationToken).Returns(Task.FromResult(response));
    var connection = new Connection(new ProductHeaderValue("OctokitTests"),
        _exampleUri,
        Substitute.For<ICredentialStore>(),
        httpClient,
        Substitute.For<IJsonSerializer>());

    await connection.Patch<string>(new Uri("endpoint", UriKind.Relative), new object());

    httpClient.Received(1).Send(Arg.Is<IRequest>(req =>
        req.BaseAddress == _exampleUri &&
        req.Body != null &&
        req.Method == HttpVerb.Patch &&
        req.ContentType == "application/x-www-form-urlencoded" &&
        req.Endpoint == new Uri("endpoint", UriKind.Relative)), Args.CancellationToken);
}

Or you could drop the req.Body verification - but I favour some correctness here if you think it's relevant to the test...

[devkhan]

@ryangribble thanks for explaining 🙇 It did filled some gaps in my knowledge. And, now that I understand the project better, I can tackle the problem at hand.

@shiftkey thanks. I will surely first try to keep the correctness intact as much as possible. Let me get back to you.

Just to be sure: I should try to configure the object's substitute in a way that httpClient.Send's IRequest param expects it to be, so that it correctly mocks the current situation?

[devkhan]

@ryangribble I tried to change the substitute, but the point is IReuqest doesn't enforce any restrictions on the Body param except being an object, and I think that's ok because the failing tests are the one which check for general HTTP requests. So, @M-Zuber solution seems enough.

Anything else should I add?

[ryangribble]

Personally I do prefer to either do like I did and declare the body and the expected serialised data (even if blank) or do like @shiftkey and indicate you don't care to check the body at all

Anyway, just give it a go however you think and push up your changes and we can have a look

[M-Zuber]

I was very tired last night and not paying attention to the point raised by @ryangribble , namely that no mock was set for the Serialize call.

Personally I would mock that call and test that body == data in order to not leave holes in the test. You can not always know what edge case will be protected against by having that in the test.

[devkhan]

I have applied @M-Zuber 's fix to the failing tests. Now the question is, how to better mock the request body?

[M-Zuber]

Follow @ryangribble's recommendation

[devkhan]

... IJsonSerializer is now correctly passed through thanks to your fix, meaning that down in the guts of constructing the http call to the API, the "data" of the response body is null (because the substitute hasnt been configured to return any data when Serialize() is called on it). This means that the Assert on httpClient.Send() is failing, since it did not receive a call that matched what we specified (specifically, the body data is now null rather than it being "" as expected)

@M-Zuber 's example is setting the expected body data to be the (null) return of the mocked serializer... which does make the test pass, however I would probably recommend instead to configure the mock to return the data we want since that is more in keeping with the unit testing approach and would also be applicable in other situations where we had a "real" body instead of a new object() as in this case...

I tried substituting the new object() with something like:

Substitute.For<IObject>().ToString().Returns(""); // I defined a dummy IObject. :P

which does passes the test, but as @ryangribble said, I don't how it will provide checking mechanism for other types of request (as this is a simple Patch request, we are simply passing in a plain object, but what about other cases where body will be different)?

How should I mock the object being passed to the connection.Patch() or to serializer.Serialize()?

[M-Zuber]

You shouldn't need to mock the object being passed in.
You want to do something like:

var serializer = Substitute.For<IJsonSerializer>();
serializer.Serialize(body)
   .Returns(SimpleJson.SerializeObject(body));

which is setting up the mock IJsonSerializer to use the SimpleJson impl.

Warning I do not know if the above works, but I believe that is at least the correct concept.

[devkhan]

But then again won't it be the same as before, when directly SimpleJson.SerializeObject was being called instead of the interface's Serialize()?
I mean, we won't be "mocking" the body.

P.S.: When I tried it, the tests were failing. 😢

[ryangribble]

does the test I posted earlier work for you? what about the variant @shiftkey posted?

[M-Zuber]

@shiftkey's variant works , but not yours

[ryangribble]

@shiftkey's variant works , but not yours

@shiftkey's is fine to go with. But just for completeness sake... mine should work too! Did you replace the entire RunsConfiguredAppWithAppropriateEnv() test with my test from above? It passes for me!

[devkhan]

I currently don't have access to my system, so I'll make the changes later, but it should work just fine.

[M-Zuber]

I ran your test exactly as above and get the following :(

[ryangribble]

@M-Zuber that sounds like you dont actually have the "fix" from this PR in place?
https://github.com/octokit/octokit.net/pull/1133/files#diff-af38d3b1f8ffea99f4957f21d0d6271eR136

It also shows that my test asserts the Serializer passed in is actually used, whilst the other tests didnt actually check this. Whether this is something these particular tests SHOULD test (or whether it should be covered off by another test) is up for debate though :)

[haacked]

Martin Fowler has a great post about using mocks and stubs. http://www.martinfowler.com/articles/mocksArentStubs.html

In general, I prefer state based testing over interaction testing. With Octokit.net, sometimes that's really difficult by the nature of what we're doing.

But in this case, I'd be fine with passing the actual seralizer because it's return value is deterministic. However, I'm also fine with the format that @ryangribble and @shiftkey proposed.

[devkhan]

It is deterministic because we are passing SimpleJson as serailizer and we know its result, but what if the user passes a different serializer (maybe with a different serialization strategy). I'm not sure, but s it possible?

[M-Zuber]

@ryangribble - hanging my head. I am going to sleep for a month or so and then I'll come back :)

[devkhan]

@M-Zuber Did I made a mistake?

[M-Zuber]

No, I wasn't paying enough attention and reporting false findings, thats all.

[ryangribble]

No, I wasn't paying enough attention and reporting false findings, thats all.

Hehe, we've all been there. Pretty funny you did all the right stuff on the tests, but forgot to actually implement the fix that started the entire PR! 😆

[ryangribble]

Thanks for your work @devkhan and good discussion by all :)
Despite using the SimpleJsonSerializer for the tests, I feel like now you've added the mock to check that the passed in ISerializer is actually being used, that gives us some good coverage if a consumer ever does want to use their own ISerializer

[ryangribble]

[devkhan]

Thanks everyone. @M-Zuber @ryangribble @haacked @shiftkey