search

hibri / HttpMock

A library for creating Http servers on the fly in tests and stubbing responses
MIT License
128 stars 44 forks source link

Assertion for query parameters #39

Closed mike-tomaras closed 8 years ago

mike-tomaras commented 8 years ago

Hi,

We have been using your library here at Audionetwork for our integration tests and we found it hard to write tests that asserted the query parameters. If we added them to the string in the form of "?a=b&c=d" in the AssertWasCalled(), we got an error that the path was never stubbed because the RequestProcessor.FindHandler() did not parse the path to strip it from the query params.

So, instead of doing this parsing, we decided to treat the query params like the body and headers of the request. We added another RequestHandlerExpectExtensions method, the WithParams() to be called after the AssertWasCalled().

Please review the changes and let me know if you have any questions or remarks. Especially about where I decided to put everything and if the test coverage is right.

Thank you, Michael Tomaras

knocte commented 8 years ago

Can you squash the commits into one please?

Also, add that good long description in this pull request, to the commit message itself.

hibri commented 8 years ago

Hi @miket969 Good stuff, thanks for this. I'll review it. A few more things to make reviewing easier, because I can't see the exact changes.

Could you please add a new test/s for your scenario, without modifying existing tests ? It's a good idea to keep existing tests green while adding new features without changing them, as it could hide behaviour that might break someone else's code.

For example https://github.com/hibri/HttpMock/pull/39/files#diff-821ec996d27d59e0c0b019769aa425d9L57

Please, also undo the formatting changes, such as in here https://github.com/hibri/HttpMock/pull/39/files#diff-37e8cad20cb5c7cc50ab8f31a9ef2d75L118

and here https://github.com/hibri/HttpMock/pull/39/files#diff-359c724420e75dfd9ed0d3aafc7a336dL18

mike-tomaras commented 8 years ago

Thanks for looking into it. The reason I changed the existing tests is that I purposefully removed the checking of query parameters from the EndpointMatchingRule.IsEndpointMatch() because that assertion happens in the new RequestHandlerExpectExtensions.WithParams().

Maybe I did not get your initial design purpose. The question I want to ask you then is: In the EndpointMatchingRule.IsEndpointMatch() you assert that the incoming request matches on the verb, the path and the query parameters on your stubbed endpoint. The headers and body are not included in this match as they are asserted separately with the RequestHandlerExpectExtensions.WithBody() and RequestHandlerExpectExtensions.WithHeader() methods. Is there a reason why I can't treat the query params in the same way? The problem started from a bug in RequestProcessor.FindHandler(). I want to verify the query params of the request so the path passed to this method actually includes them and no handlers are ever matched. We could sanitize the path in this case but this would ignore the query params. That is where the whole adding of RequestHandlerExpectExtensions.WithQueryParams() started and we had to modify how IsEndpointMatch() worked.

Please let me know if I am on the right path. As for the formatting changes, I'll try and remove them all.

Thank you

hibri commented 8 years ago

@miket969 The purpose of the EndPoint matching rule is not to Assert an incoming request. It's to find a stub for an incoming request and find a handler (if it exists) to return a canned response. The .WithParams method exists to do an even more specific match with query parameters. The HTTP method, path and query parameters are enough to do this.

The RequestHandlerExpectExtensions are to verify if the last call to a handler, that was stubbed previously. This is where the headers and body are verified.

AssertWasCalled shouldn't contain any query string parameters, it should only have the path.

For your scenario, you'll need to add only the WithParams extension to RequestHandlerExpectExtensions to verify that the a path was called with the expected query params. I've tested this change with the tests you've added.

Also please try to pass in an IResolveConstraint to WithParams (See usage of .WithBody ), so that you lean on NUnit's Collection assertion methods.

The reason for this is not to fix the query param match verification logic, inside HttpMock, to give the user more flexibility in the match logic.

Hope this makes sense.

mike-tomaras commented 8 years ago

@hibri Thank you for the clarification. I will give it another go and do another pull request when I'm done.

hibri commented 8 years ago

@miket969 great. Let me know if you need help. Happy to pair with you on Skype. Feel free to email me at hibri at hibri dot net

mike-tomaras commented 8 years ago

Hi Hibri, I've sent you an email and it hasn't bounced, so I guess I spelled the email address correctly :) I'm just letting you know because of your previous swift responses.