MockHttp icon indicating copy to clipboard operation
MockHttp copied to clipboard

VerifyAll does not print request body

Open ivanjx opened this issue 2 years ago • 3 comments

i think it would be helpful if the request body is also printed when calling VerifyAll() instead of just the header.

MockHttp.HttpMockException
There are 1 unfulfilled expectations:
	Method: POST, RequestUri: 'https://x.com:9999/rpc', Headers: Authorization: Basic dXNlcm5hbWU6cGFzc3dvcmQ=, MockHttp.Json.JsonContentMatcher
Seen requests: Method: POST, RequestUri: 'https://x.com:9999/rpc', Version: 1.1, Content: System.Net.Http.Json.JsonContent, Headers:
{
  Authorization: Basic dXNlcm5hbWU6cGFzc3dvcmQ=
  Content-Type: application/json; charset=utf-8
  Content-Length: 194
}

ivanjx avatar Oct 02 '23 05:10 ivanjx

It would seem helpful and trivial to change, but there are issues/difficulties:

  • in scenarios where multiple requests have been issued, the 'seen requests' output message will have all of them. F.ex. for 10 requests, this exception message becomes quite large and you will have a hard time to figure out which one is relevant. It then often becomes easier to just debug the test and inspect MockHttpHandler.InvokedRequests.
  • request bodies can potentially be very long, so it needs to be truncated to a max length.
  • request bodies can be in a non-text/readable content-type, or even mixed like multipart/form-data.
  • and last but not least, reading the content requires async method (because we have to call Content.ReadAsStringAsync() or similar), but VerifyAll() is currently synchronous. So it would require an async overload. I am not against this, but retrofitting this into VerifyAll() is just not possible and would require doing sync over async.

All of this is doable, sure, but I am not sure the added complexity is really worth it. That said, I am open to suggestions and am interested to hear other thoughts on this.

skwasjer avatar Oct 03 '23 13:10 skwasjer

@ivanjx Perhaps using a verbosity flag to opt-in would make sense? This ensures we don't flood test output logs in case of a test failure where a mock has many seen requests and/or with large response payloads, unless explicitly opted-in? I am somewhat hesitant about this, due to many CI environments having strict limits on log file size.

Body truncation would still be necessary to eg. 5000 chars (considering someone could mock a large file upload) as would media type handling (to catch binary content), but this is doable.

That still leaves the problem of VerifyAll not being asynchronous, but perhaps this is the time to change it to async.

skwasjer avatar Dec 22 '23 22:12 skwasjer

how about a diff of the body if the type is text (plain/text, json, xml, html, etc) ?

ivanjx avatar Dec 25 '23 10:12 ivanjx