Proposal: make_request helpers shouldn't assert status code

[david-crespo]

It confuses me every time that a helper called make_request_with_body takes a status code and asserts the response has it. I would much rather see explicit asserts in the calling test code. The main problem I see with making this change is that if you take out an assert, callers are required to add in the assert in their test code, otherwise any existing tests will continue to silently pass even if they're not supposed to.

[davepacheco]

Ah, sorry for the confusion. When I wrote these, I was trying to minimize the boilerplate. It seemed like the alternative added several lines of code to every caller. It's not just the assertion about the status code. Today, we give you back either Ok(response) (which has the status code) or Err(error), where error is a deserialized structure with information about the error. We'd have to return something more complicated for the error case, and the caller would have to either unwrap what they got back or pattern-match and check the response code in a different way for each case. Maybe try prototyping to see what it looks like?

All existing callers today are relying on this behavior, and it's a public interface so this would be a breaking change. These interfaces are getting pretty unwieldy anyway. I'd suggest we develop a new client testing interface (it need not even be part of dropshot, though it could be if that's more convenient). There might be something out there already that does this better. If not, I'd probably start with a builder pattern. Maybe something like:

TestRequest::build()
    .method(Method::GET)
    .url(...)
    [.body(...)]
    [.header(...)]
    .execute()
    .await
    .assert_status_code(http::StatusCode::OK)
    .parse_response_as::<Result<SuccessType>, HttpErrorResponseBody>()

The existing thing also checks that a 204 has no response body. It'd be nice to preserve that.

[david-crespo]

Totally agree about a new interface, that sounds great. We have a lot more examples to abstract from now. I've tried to do that a bit with wrappers around stuff in test_utils but that approach is necessarily limited.

[david-crespo]

I don't mind the response-parsing stuff in the helpers, I think that's appropriate. I don't even mind the bit where we assert that a 204 actually has an empty body. All that stuff is about the internal consistency of a response. I just don't think asserting about a particular response code fits in with that.