Skip to content

Commit ee2bbf8

Browse files
AntonboomAntonboom
authored
Merge pull request #90 from Antonboom/feat/require-http-handlers
go-require: require-error: support HTTP handlers
2 parents e36c534 + 613bd95 commit ee2bbf8

17 files changed

+492
-100
lines changed

‎README.md

+12-5
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ $ testifylint --enable-all --disable=empty,error-is-as ./...
6868
# Checkers configuration.
6969
$ testifylint --bool-compare.ignore-custom-types ./...
7070
$ testifylint --expected-actual.pattern=^wanted$ ./...
71+
$ testifylint --go-require.ignore-http-handlers ./...
7172
$ testifylint --require-error.fn-pattern="^(Errorf?|NoErrorf?)$" ./...
7273
$ testifylint --suite-extra-assert-call.mode=require ./...
7374
```
@@ -336,7 +337,7 @@ go func() {
336337
conn, err = lis.Accept()
337338
require.NoError(t, err) ❌
338339

339-
if assert.Error(err) {
340+
if assert.Error(err) { ✅
340341
assert.FailNow(t, msg) ❌
341342
}
342343
}()
@@ -367,7 +368,13 @@ Also a bad solution would be to simply replace all `require` in goroutines with
367368

368369
The checker is enabled by default, because `testinggoroutine` is enabled by default in `go vet`.
369370

370-
P.S. Related `testify`'s [thread](https://github.com/stretchr/testify/issues/772).
371+
In addition, the checker warns about `require` in HTTP handlers (functions and methods whose signature matches
372+
[http.HandlerFunc](https://pkg.go.dev/net/http#HandlerFunc)), because handlers run in a separate
373+
[service goroutine](https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/net/http/server.go;l=2782;drc=1d45a7ef560a76318ed59dfdb178cecd58caf948) that
374+
services the HTTP connection. Terminating these goroutines can lead to undefined behaviour and difficulty debugging tests.
375+
You can turn off the check using the `--go-require.ignore-http-handlers` flag.
376+
377+
P.S. Look at [testify's issue](https://github.com/stretchr/testify/issues/772), related to assertions in the goroutines.
371378

372379
---
373380

@@ -481,11 +488,11 @@ You can set `--require-error.fn-pattern` flag to limit the checking to certain c
481488
For example, `--require-error.fn-pattern="^(Errorf?|NoErrorf?)$"` will only check `Error`, `Errorf`, `NoError`, and `NoErrorf`.
482489

483490
Also, to minimize false positives, `require-error` ignores:
484-
- assertion in the `if` condition;
485-
- assertion in the bool expression;
491+
- assertions in the `if` condition;
492+
- assertions in the bool expression;
486493
- the entire `if-else[-if]` block, if there is an assertion in any `if` condition;
487494
- the last assertion in the block, if there are no methods/functions calls after it;
488-
- assertions in an explicit goroutine;
495+
- assertions in an explicit goroutine (including `http.Handler`);
489496
- assertions in an explicit testing cleanup function or suite teardown methods;
490497
- sequence of `NoError` assertions.
491498

‎analyzer/analyzer.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ const (
1919
url = "https://github.com/antonboom/" + name
2020
)
2121

22-
// New returns new instance of testifylint analyzer.
22+
// New returns a new instance of testifylint analyzer.
2323
func New() *analysis.Analyzer {
2424
cfg := config.NewDefault()
2525

‎analyzer/analyzer_test.go

+16-3
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,23 @@ func TestTestifyLint(t *testing.T) {
4949
"expected-actual.pattern": "goldenValue",
5050
},
5151
},
52-
{dir: "ginkgo"},
5352
{
54-
dir: "go-require-issue66",
55-
flags: map[string]string{"enable-all": "true"},
53+
dir: "ginkgo",
54+
},
55+
{
56+
dir: "go-require-http-handlers",
57+
flags: map[string]string{
58+
"enable": checkers.NewGoRequire().Name() + "," + // https://github.com/Antonboom/testifylint/issues/66
59+
checkers.NewRequireError().Name(), // https://github.com/Antonboom/testifylint/issues/73
60+
},
61+
},
62+
{
63+
dir: "go-require-ignore-http-handlers",
64+
flags: map[string]string{
65+
"disable-all": "true",
66+
"enable": checkers.NewGoRequire().Name(),
67+
"go-require.ignore-http-handlers": "true",
68+
},
5669
},
5770
{
5871
dir: "go-require-issue67",

‎analyzer/checkers_factory.go

+3
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,9 @@ func newCheckers(cfg config.Config) ([]checkers.RegularChecker, []checkers.Advan
5555
case *checkers.ExpectedActual:
5656
c.SetExpVarPattern(cfg.ExpectedActual.ExpVarPattern.Regexp)
5757

58+
case *checkers.GoRequire:
59+
c.SetIgnoreHTTPHandlers(cfg.GoRequire.IgnoreHTTPHandlers)
60+
5861
case *checkers.RequireError:
5962
c.SetFnPattern(cfg.RequireError.FnPattern.Regexp)
6063

‎analyzer/checkers_factory_test.go

+14
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,20 @@ func Test_newCheckers(t *testing.T) {
166166
},
167167
expAdvanced: []checkers.AdvancedChecker{},
168168
},
169+
{
170+
name: "go-require ignore http handlers",
171+
cfg: config.Config{
172+
DisableAll: true,
173+
EnabledCheckers: config.KnownCheckersValue{checkers.NewGoRequire().Name()},
174+
GoRequire: config.GoRequireConfig{
175+
IgnoreHTTPHandlers: true,
176+
},
177+
},
178+
expRegular: []checkers.RegularChecker{},
179+
expAdvanced: []checkers.AdvancedChecker{
180+
checkers.NewGoRequire().SetIgnoreHTTPHandlers(true),
181+
},
182+
},
169183
{
170184
name: "require-equal fn pattern defined",
171185
cfg: config.Config{

‎analyzer/testdata/src/go-require-http-handlers/go_require_http_handlers_test.go

+130
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package gorequireissue66issue73_test
2+
3+
import (
4+
"io"
5+
"net/http"
6+
"net/http/httptest"
7+
"os"
8+
"testing"
9+
"time"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
"github.com/stretchr/testify/suite"
14+
)
15+
16+
// NOTE(a.telyshev): Neither `assert` nor `require` is the best way to test an HTTP handler:
17+
// it leads to redundant stack traces (up to runtime assembler), as well as undefined behaviour (in `require` case).
18+
// Use HTTP mechanisms (status code, headers, response data) and place assertions in the main test function.
19+
20+
func TestServer_Assert(t *testing.T) {
21+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
22+
file, err := os.Open("some file.json")
23+
if !assert.NoError(t, err) {
24+
return
25+
}
26+
27+
data, err := io.ReadAll(file)
28+
if !assert.NoError(t, err) {
29+
return
30+
}
31+
32+
w.Header().Set("Content-Type", "application/json")
33+
_, err = w.Write(data)
34+
assert.NoError(t, err)
35+
}))
36+
defer ts.Close()
37+
38+
client := ts.Client()
39+
40+
req, err := http.NewRequest("GET", ts.URL+"/assert", nil)
41+
assert.NoError(t, err) // want "require-error: for error assertions use require"
42+
43+
resp, err := client.Do(req)
44+
require.NoError(t, err)
45+
defer func() {
46+
assert.NoError(t, resp.Body.Close())
47+
}()
48+
49+
assert.Equal(t, http.StatusOK, resp.StatusCode)
50+
}
51+
52+
func TestServer_Require(t *testing.T) {
53+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
54+
file, err := os.Open("some file.json")
55+
require.NoError(t, err) // want "go-require: do not use require in http handlers"
56+
57+
data, err := io.ReadAll(file)
58+
require.NoError(t, err) // want "go-require: do not use require in http handlers"
59+
60+
w.Header().Set("Content-Type", "application/json")
61+
_, err = w.Write(data)
62+
if !assert.NoError(t, err) {
63+
assert.FailNow(t, err.Error()) // want "go-require: do not use assert\\.FailNow in http handlers"
64+
}
65+
}))
66+
defer ts.Close()
67+
68+
client := ts.Client()
69+
client.Timeout = 10 * time.Second
70+
71+
req, err := http.NewRequest("GET", ts.URL+"/require", nil)
72+
require.NoError(t, err)
73+
74+
resp, err := client.Do(req)
75+
require.NoError(t, err)
76+
defer func() {
77+
require.NoError(t, resp.Body.Close())
78+
}()
79+
80+
require.Equal(t, http.StatusOK, resp.StatusCode)
81+
}
82+
83+
type ServerSuite struct {
84+
suite.Suite
85+
}
86+
87+
func TestServerSuite(t *testing.T) {
88+
suite.Run(t, &ServerSuite{})
89+
}
90+
91+
func (s *ServerSuite) TestServer() {
92+
httptest.NewServer(http.HandlerFunc(s.handler))
93+
httptest.NewServer(s)
94+
}
95+
96+
func (s *ServerSuite) ServeHTTP(w http.ResponseWriter, _ *http.Request) {
97+
s.T().Helper()
98+
99+
file, err := os.Open("some file.json")
100+
s.Require().NoError(err) // want "go-require: do not use require in http handlers"
101+
102+
data, err := io.ReadAll(file)
103+
if !s.NoError(err) {
104+
return
105+
}
106+
107+
w.Header().Set("Content-Type", "application/json")
108+
_, err = w.Write(data)
109+
if !s.NoError(err) {
110+
s.FailNow(err.Error()) // want "go-require: do not use s\\.FailNow in http handlers"
111+
}
112+
}
113+
114+
func (s *ServerSuite) handler(w http.ResponseWriter, _ *http.Request) {
115+
s.T().Helper()
116+
117+
file, err := os.Open("some file.json")
118+
s.Require().NoError(err) // want "go-require: do not use require in http handlers"
119+
120+
data, err := io.ReadAll(file)
121+
if !s.NoError(err) {
122+
return
123+
}
124+
125+
w.Header().Set("Content-Type", "application/json")
126+
_, err = w.Write(data)
127+
if !s.NoError(err) {
128+
s.FailNow(err.Error()) // want "go-require: do not use s\\.FailNow in http handlers"
129+
}
130+
}

‎analyzer/testdata/src/go-require-ignore-http-handlers/go_require_ignore_http_handlers_test.go

+81
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
package gorequireignorehttphandlers_test
2+
3+
import (
4+
"encoding/json"
5+
"io"
6+
"net/http"
7+
"net/http/httptest"
8+
"os"
9+
"testing"
10+
"time"
11+
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
14+
"github.com/stretchr/testify/suite"
15+
)
16+
17+
func TestServer_Require(t *testing.T) {
18+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
19+
file, err := os.Open("some file.json")
20+
require.NoError(t, err)
21+
22+
data, err := io.ReadAll(file)
23+
require.NoError(t, err)
24+
25+
w.Header().Set("Content-Type", "application/json")
26+
_, err = w.Write(data)
27+
if !assert.NoError(t, err) {
28+
assert.FailNow(t, err.Error())
29+
}
30+
}))
31+
defer ts.Close()
32+
33+
client := ts.Client()
34+
client.Timeout = 10 * time.Second
35+
36+
req, err := http.NewRequest("GET", ts.URL+"/require", nil)
37+
require.NoError(t, err)
38+
39+
statusCode := make(chan int)
40+
go func() {
41+
resp, err := client.Do(req)
42+
require.NoError(t, err) // want "go-require: require must only be used in the goroutine running the test function"
43+
defer func() {
44+
require.NoError(t, resp.Body.Close()) // want "go-require: require must only be used in the goroutine running the test function"
45+
}()
46+
statusCode <- resp.StatusCode
47+
}()
48+
49+
require.Equal(t, http.StatusOK, <-statusCode)
50+
}
51+
52+
type SomeServerSuite struct {
53+
suite.Suite
54+
}
55+
56+
func TestSomeServerSuite(t *testing.T) {
57+
suite.Run(t, &SomeServerSuite{})
58+
}
59+
60+
func (s *SomeServerSuite) TestServer() {
61+
httptest.NewServer(http.HandlerFunc(s.handler))
62+
httptest.NewServer(s)
63+
}
64+
65+
func (s *SomeServerSuite) ServeHTTP(hres http.ResponseWriter, hreq *http.Request) {
66+
var req MyRequest
67+
err := json.NewDecoder(hreq.Body).Decode(&req)
68+
s.Require().NoError(err)
69+
s.Equal("42", req.ID)
70+
}
71+
72+
func (s *SomeServerSuite) handler(hres http.ResponseWriter, hreq *http.Request) {
73+
var req MyRequest
74+
err := json.NewDecoder(hreq.Body).Decode(&req)
75+
s.Require().NoError(err)
76+
s.Equal("42", req.ID)
77+
}
78+
79+
type MyRequest struct {
80+
ID string
81+
}

‎analyzer/testdata/src/go-require-issue66/gorequire_requireerror_conflict_test.go

-50
This file was deleted.

0 commit comments

Comments
 (0)