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

Matovidlo · 2023-10-23T12:31:15Z

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 · 2024-02-15T18:18:15Z

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 · 2024-02-16T12:34:53Z

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

#601 here is simillar PR with header tests that could be compared with this PR.

Have a nice day

arjunmahishi · 2024-02-16T14:44:50Z

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.

arjunmahishi · 2024-02-16T15:01:59Z

assert/http_assertions.go

+		h.Helper()
+	}
+
+	b := &builder{}


Since the options is optional, should the builder have some defaults?

Consider this:

func handler(w http.ResponseWriter, r *http.Request) { w.Write([]byte("hello")) } func TestHttp(t *testing.T) { assert.HTTP(t, handler, http.MethodGet, "/", nil) }

Fails with:

=== RUN TestHttp main_test.go:15: Error Trace: /Users/armahishi/dev/personal/forks/testify/example/main_test.go:15 Error: Expected HTTP success status code for "/?" but received 200 Test: TestHttp --- FAIL: TestHttp (0.00s) FAIL FAIL github.com/stretchr/testify/example 0.173s FAIL

Good point I have added the default code for builder.

I have made a change, based on what you said in next review step.
There are no defaults as the option would override something that is default for the builder. You mentioned that you expected that the status.code is not tested as You did not ask for it.
As the options should be optional, I made a change when you select no option, there is nothing to be tested so the test fails. It is possible to to remove this check, however it is incorrectly tested handler as there is not assertion.

arjunmahishi · 2024-02-16T15:12:46Z

assert/http_assertions.go

+	for _, option := range options {
+		err := option(b)
+		if err != nil {
+			Fail(t, fmt.Sprintf("Failed to build http options, got error: %s", err))
+		}
+	}


If only a subset of options are provided, it should only assert that.

Example:

func handler(w http.ResponseWriter, r *http.Request) { w.Write([]byte("hello")) } func TestHttp(t *testing.T) { buf := bytes.NewBufferString("hello") assert.HTTP(t, handler, http.MethodGet, "/", nil, assert.WithExpectedBody(*buf)) }

=== RUN TestHttp main_test.go:18: Error Trace: /Users/armahishi/dev/personal/forks/testify/example/main_test.go:18 Error: Expected HTTP success status code for "/?" but received 200 Test: TestHttp --- FAIL: TestHttp (0.00s) FAIL FAIL github.com/stretchr/testify/example 0.162s FAIL

In general, if the all those options are not optional, they should not be variadic.

To preserve the variadic options, then the code cannot be set as default because there is option that would be set up always and therefore it is not an option, it would be a required parameter of function.
Therefore I decided to make a status.code to be option however to make sure the usage of tests is correct, I throw a fail when no options are set as there is nothing to assert using result of handler call.
Basically it is a prevention of missuse of test that it does nothing except the handler execution.

arjunmahishi · 2024-02-16T15:21:49Z

assert/http_assertions.go

+	if w.Code != b.code {
+		Fail(t, fmt.Sprintf("Expected HTTP success status code for %q but received %d", url+"?"+values.Encode(), w.Code))
+		return false
+	}


We can reuse the existing assertions. It already has a lot of diff logic.

Suggested change

if w.Code != b.code {

Fail(t, fmt.Sprintf("Expected HTTP success status code for %q but received %d", url+"?"+values.Encode(), w.Code))

return false

}

Equal(t, b.code, w.Code)

=== RUN TestHttp main_test.go:15: Error Trace: /Users/armahishi/dev/personal/forks/testify/example/main_test.go:15 Error: Expected HTTP success status code for "/?" but received 200 Test: TestHttp --- FAIL: TestHttp (0.00s) FAIL FAIL github.com/stretchr/testify/example 0.161s FAIL

VS

=== RUN TestHttp main_test.go:15: Error Trace: /Users/armahishi/dev/personal/forks/testify/example/main_test.go:15 Error: Not equal: expected: 404 actual : 200 Test: TestHttp --- FAIL: TestHttp (0.00s) FAIL FAIL github.com/stretchr/testify/example 0.163s FAIL

I see your point, however I wanted to be concise with the code style in the module, and there is not usage of Equal even though it is possible.
As I am more sceptical to use module itself I think it is correct to not use Equal function as if it will be changed in future, you have to rework whole http_assertions.go module as it is dependend on that.
It is probably bad assumption but I think we can have a discussion about this and possibly make another PR with appropriate refactor of http_assertions.go module. 😅

When no option provided there is nothing to test. Throw a Fail when no options are selected to prevent missuse of new function.

Matovidlo · 2024-08-08T18:49:15Z

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

brackendawson · 2024-10-02T13:19:42Z

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.

Matovidlo · 2024-10-07T11:18:09Z

@brackendawson Will be there any progress or just close this PR as wont do? I don't see a point what should I do differently or how to send PR with enhancement of this.

brackendawson · 2024-10-07T11:43:32Z

I honestly don't see us adding more assertions that call handlers. Sorry about that.

dolmen added the pkg-assert Change related to package testify/assert label Oct 30, 2023

Matovidlo force-pushed the http-builder-assert branch from dfbe58e to 8cec282 Compare February 16, 2024 12:27

arjunmahishi reviewed Feb 16, 2024

View reviewed changes

brackendawson self-requested a review February 16, 2024 15:24

brackendawson added this to the v1.9.0 milestone Feb 19, 2024

brackendawson mentioned this pull request Feb 24, 2024

Http Assertions does not allow the creation of a body #925

Closed

brackendawson modified the milestones: v1.9.0, v1.10 Feb 28, 2024

Matovidlo requested a review from arjunmahishi March 6, 2024 09:46

Martin Vaško added 2 commits April 17, 2024 13:39

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

21869a3

fix(http): Make option optional.

09e4190

When no option provided there is nothing to test. Throw a Fail when no options are selected to prevent missuse of new function.

Matovidlo force-pushed the http-builder-assert branch from 47afa36 to 09e4190 Compare April 17, 2024 11:39

brackendawson removed their request for review October 2, 2024 13:21

brackendawson closed this Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Matovidlo commented Oct 23, 2023 •

edited

Loading

arjunmahishi commented Feb 15, 2024 •

edited

Loading

Matovidlo commented Feb 16, 2024 •

edited

Loading

arjunmahishi commented Feb 16, 2024 •

edited

Loading

arjunmahishi Feb 16, 2024

Matovidlo Mar 6, 2024

Matovidlo Mar 6, 2024

arjunmahishi Feb 16, 2024

Matovidlo Mar 6, 2024

arjunmahishi Feb 16, 2024 •

edited

Loading

Matovidlo Mar 6, 2024

Matovidlo commented Aug 8, 2024

brackendawson commented Oct 2, 2024

Matovidlo commented Oct 7, 2024

brackendawson commented Oct 7, 2024

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

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

Conversation

Matovidlo commented Oct 23, 2023 • edited Loading

Summary

Changes

Motivation

Related issues

arjunmahishi commented Feb 15, 2024 • edited Loading

Matovidlo commented Feb 16, 2024 • edited Loading

arjunmahishi commented Feb 16, 2024 • edited Loading

arjunmahishi Feb 16, 2024

Choose a reason for hiding this comment

Matovidlo Mar 6, 2024

Choose a reason for hiding this comment

Matovidlo Mar 6, 2024

Choose a reason for hiding this comment

arjunmahishi Feb 16, 2024

Choose a reason for hiding this comment

Matovidlo Mar 6, 2024

Choose a reason for hiding this comment

arjunmahishi Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Matovidlo Mar 6, 2024

Choose a reason for hiding this comment

Matovidlo commented Aug 8, 2024

brackendawson commented Oct 2, 2024

Matovidlo commented Oct 7, 2024

brackendawson commented Oct 7, 2024

Matovidlo commented Oct 23, 2023 •

edited

Loading

arjunmahishi commented Feb 15, 2024 •

edited

Loading

Matovidlo commented Feb 16, 2024 •

edited

Loading

arjunmahishi commented Feb 16, 2024 •

edited

Loading

arjunmahishi Feb 16, 2024 •

edited

Loading