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

VerifyAll does not print request body #73

Open
ivanjx opened this issue Oct 2, 2023 · 3 comments
Open

VerifyAll does not print request body #73

ivanjx opened this issue Oct 2, 2023 · 3 comments

Comments

@ivanjx
Copy link

ivanjx commented Oct 2, 2023

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
}
@skwasjer
Copy link
Owner

skwasjer commented Oct 3, 2023

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
Copy link
Owner

skwasjer commented Dec 22, 2023

@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.

@ivanjx
Copy link
Author

ivanjx commented Dec 25, 2023

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

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