assert: Add HTTP builder to combine request x response using builder.

Matovidlo commented 11 months ago

Summary

Adds new method into assert http_assertions.go. Should be used as generic solution for testing request parameters to response possibilities using 1 method with options. Previous solutions did not have response header check method. However implementing that it would be necessary to create single test with 3 assert conditions. Now it is possible to use single function to test typical HTTP response values(status, body, headers).

Changes

Add new function HTTP(...) that can be customized based on user needs. Use go generate ./... to update generated code. Add new test TestHTTPBuilder without example usage.

Motivation

Impossible to test HTTP response headers.

assert.True(mockAssert.HTTP(httpHelloName, "GET", "/", url.Values{"name": []string{"World"}}, WithCode(200), WithResponseHeader(http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}), WithExpectedBody(*bytes.NewBufferString("Hello, World!"))))

Related issues

Closes #598

arjunmahishi commented 7 months ago

Thanks for putting up this PR! 🎉

Honestly, I am not a fan of the assert package calling the handler function as a part of an assert function/method. IMO, this package should only match values. I would write HTTP assertions like this:

resp, err := makeSomeHTTPCall()
require.NoError(t, err)
defer resp.Body.Close()

assert.HTTPStatus(t, resp, http.StatusOK)
assert.HTTPRespBodyContains(t, resp, "foo")
assert.HTTPRespHeaderContains(t, resp, ...)

Simple assertions on a given value.

But it looks like there already are some functions (like HTTPBodyContains, HTTPStatusCode, etc) that take the handler as an argument and execute the HTTP request as a part of the assertion function.

Would love to hear the thoughts of other maintainers on this. @dolmen @brackendawson

Matovidlo commented 7 months ago

@arjunmahishi Hello, I just added new function to already created module in simillar way. I am not sure how to refactor it so the code base looks fine. Either it would need first of all refactor of the module and then make this PR as feature to new code base.

Also there would be a major change if we refactor these function to not work with handler. So when projects with already created tests would need to refactor it. Probablly this is for further discussion in some Issue and create for example v2 branch?

Just to mention the motivation, due to not accepting this PR I need 2 testing frameworks to test the HTTP request, response. The issue I found is that it is not possible to test the response header/headers. However, to simplify the tests I thought it would be better to create 1 assert function. The developer will scope the HTTP by what is the developer testing.

I would like to receive at least some comment whether I have to refactor it to have just HTTPResponseHeaderContains Instead of HTTP.

https://github.com/stretchr/testify/pull/601 here is simillar PR with header tests that could be compared with this PR.

Have a nice day

arjunmahishi commented 7 months ago

Yes. I understand the intent of this PR. Maybe I did not phrase it correctly. Don't let my comment be a blocker for this PR. The testing strategy i mentioned would require a lot of refactoring. And its probably not worth doing that. I was just putting it out there.

I think the changes made in this PR are useful. Other maintainers who've been around for longer can give more contextual comments on this.

Matovidlo commented 1 month ago

@arjunmahishi Is there any update regarding this PR? I would like to push it forward if possible. :slightly_smiling_face:

brackendawson commented 1 day ago

Honestly, I am not a fan of the assert package calling the handler function as a part of an assert function/method. IMO, this package should only match values.

This is my thinking too. I note this that proposed function also only works with http.HandlerFunc and not http.Handler, would we add another function to support http.Handler? Better to make assertions against a httptest.ResponseRecorder because this would support both standard library handlers, as well as those from other libraries that differ from stdlib such as gin and restful.

stretchr / testify