Merge pull request #90 from Antonboom/feat/require-http-handlers

go-require: require-error: support HTTP handlers

Author: Antonboom

README.md

@@ -68,6 +68,7 @@ $ testifylint --enable-all --disable=empty,error-is-as ./...
 # Checkers configuration.
 $ testifylint --bool-compare.ignore-custom-types ./...
 $ testifylint --expected-actual.pattern=^wanted$ ./...
+$ testifylint --go-require.ignore-http-handlers ./...
 $ testifylint --require-error.fn-pattern="^(Errorf?|NoErrorf?)$" ./...
 $ testifylint --suite-extra-assert-call.mode=require ./...
 ```
@@ -336,7 +337,7 @@ go func() {
     conn, err = lis.Accept()
     require.NoError(t, err) ❌
 
-    if assert.Error(err) {
+    if assert.Error(err) {     ✅
         assert.FailNow(t, msg) ❌
     }
 }()
@@ -367,7 +368,13 @@ Also a bad solution would be to simply replace all `require` in goroutines with
 
 The checker is enabled by default, because `testinggoroutine` is enabled by default in `go vet`.
 
-P.S. Related `testify`'s [thread](https://github.com/stretchr/testify/issues/772).
+In addition, the checker warns about `require` in HTTP handlers (functions and methods whose signature matches 
+[http.HandlerFunc](https://pkg.go.dev/net/http#HandlerFunc)), because handlers run in a separate 
+[service goroutine](https://cs.opensource.google/go/go/+/refs/tags/go1.22.3:src/net/http/server.go;l=2782;drc=1d45a7ef560a76318ed59dfdb178cecd58caf948) that 
+services the HTTP connection. Terminating these goroutines can lead to undefined behaviour and difficulty debugging tests.
+You can turn off the check using the `--go-require.ignore-http-handlers` flag.
+
+P.S. Look at [testify's issue](https://github.com/stretchr/testify/issues/772), related to assertions in the goroutines.
 
 ---
 
@@ -481,11 +488,11 @@ You can set `--require-error.fn-pattern` flag to limit the checking to certain c
 For example, `--require-error.fn-pattern="^(Errorf?|NoErrorf?)$"` will only check `Error`, `Errorf`, `NoError`, and `NoErrorf`.
 
 Also, to minimize false positives, `require-error` ignores:
-- assertion in the `if` condition;
-- assertion in the bool expression;
+- assertions in the `if` condition;
+- assertions in the bool expression;
 - the entire `if-else[-if]` block, if there is an assertion in any `if` condition;
 - the last assertion in the block, if there are no methods/functions calls after it;
-- assertions in an explicit goroutine;
+- assertions in an explicit goroutine (including `http.Handler`);
 - assertions in an explicit testing cleanup function or suite teardown methods;
 - sequence of `NoError` assertions.
 

analyzer/analyzer.go

@@ -19,7 +19,7 @@ const (
 	url  = "https://github.com/antonboom/" + name
 )
 
-// New returns new instance of testifylint analyzer.
+// New returns a new instance of testifylint analyzer.
 func New() *analysis.Analyzer {
 	cfg := config.NewDefault()
 

analyzer/analyzer_test.go

@@ -49,10 +49,23 @@ func TestTestifyLint(t *testing.T) {
 				"expected-actual.pattern": "goldenValue",
 			},
 		},
-		{dir: "ginkgo"},
 		{
-			dir:   "go-require-issue66",
-			flags: map[string]string{"enable-all": "true"},
+			dir: "ginkgo",
+		},
+		{
+			dir: "go-require-http-handlers",
+			flags: map[string]string{
+				"enable": checkers.NewGoRequire().Name() + "," + // https://github.com/Antonboom/testifylint/issues/66
+					checkers.NewRequireError().Name(), // https://github.com/Antonboom/testifylint/issues/73
+			},
+		},
+		{
+			dir: "go-require-ignore-http-handlers",
+			flags: map[string]string{
+				"disable-all":                     "true",
+				"enable":                          checkers.NewGoRequire().Name(),
+				"go-require.ignore-http-handlers": "true",
+			},
 		},
 		{
 			dir:   "go-require-issue67",

analyzer/checkers_factory.go

@@ -55,6 +55,9 @@ func newCheckers(cfg config.Config) ([]checkers.RegularChecker, []checkers.Advan
 		case *checkers.ExpectedActual:
 			c.SetExpVarPattern(cfg.ExpectedActual.ExpVarPattern.Regexp)
 
+		case *checkers.GoRequire:
+			c.SetIgnoreHTTPHandlers(cfg.GoRequire.IgnoreHTTPHandlers)
+
 		case *checkers.RequireError:
 			c.SetFnPattern(cfg.RequireError.FnPattern.Regexp)
 

analyzer/checkers_factory_test.go

@@ -166,6 +166,20 @@ func Test_newCheckers(t *testing.T) {
 			},
 			expAdvanced: []checkers.AdvancedChecker{},
 		},
+		{
+			name: "go-require ignore http handlers",
+			cfg: config.Config{
+				DisableAll:      true,
+				EnabledCheckers: config.KnownCheckersValue{checkers.NewGoRequire().Name()},
+				GoRequire: config.GoRequireConfig{
+					IgnoreHTTPHandlers: true,
+				},
+			},
+			expRegular: []checkers.RegularChecker{},
+			expAdvanced: []checkers.AdvancedChecker{
+				checkers.NewGoRequire().SetIgnoreHTTPHandlers(true),
+			},
+		},
 		{
 			name: "require-equal fn pattern defined",
 			cfg: config.Config{

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

@@ -0,0 +1,130 @@
+package gorequireissue66issue73_test
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
+)
+
+// NOTE(a.telyshev): Neither `assert` nor `require` is the best way to test an HTTP handler:
+// it leads to redundant stack traces (up to runtime assembler), as well as undefined behaviour (in `require` case).
+// Use HTTP mechanisms (status code, headers, response data) and place assertions in the main test function.
+
+func TestServer_Assert(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		file, err := os.Open("some file.json")
+		if !assert.NoError(t, err) {
+			return
+		}
+
+		data, err := io.ReadAll(file)
+		if !assert.NoError(t, err) {
+			return
+		}
+
+		w.Header().Set("Content-Type", "application/json")
+		_, err = w.Write(data)
+		assert.NoError(t, err)
+	}))
+	defer ts.Close()
+
+	client := ts.Client()
+
+	req, err := http.NewRequest("GET", ts.URL+"/assert", nil)
+	assert.NoError(t, err) // want "require-error: for error assertions use require"
+
+	resp, err := client.Do(req)
+	require.NoError(t, err)
+	defer func() {
+		assert.NoError(t, resp.Body.Close())
+	}()
+
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+}
+
+func TestServer_Require(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		file, err := os.Open("some file.json")
+		require.NoError(t, err) // want "go-require: do not use require in http handlers"
+
+		data, err := io.ReadAll(file)
+		require.NoError(t, err) // want "go-require: do not use require in http handlers"
+
+		w.Header().Set("Content-Type", "application/json")
+		_, err = w.Write(data)
+		if !assert.NoError(t, err) {
+			assert.FailNow(t, err.Error()) // want "go-require: do not use assert\\.FailNow in http handlers"
+		}
+	}))
+	defer ts.Close()
+
+	client := ts.Client()
+	client.Timeout = 10 * time.Second
+
+	req, err := http.NewRequest("GET", ts.URL+"/require", nil)
+	require.NoError(t, err)
+
+	resp, err := client.Do(req)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, resp.Body.Close())
+	}()
+
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+}
+
+type ServerSuite struct {
+	suite.Suite
+}
+
+func TestServerSuite(t *testing.T) {
+	suite.Run(t, &ServerSuite{})
+}
+
+func (s *ServerSuite) TestServer() {
+	httptest.NewServer(http.HandlerFunc(s.handler))
+	httptest.NewServer(s)
+}
+
+func (s *ServerSuite) ServeHTTP(w http.ResponseWriter, _ *http.Request) {
+	s.T().Helper()
+
+	file, err := os.Open("some file.json")
+	s.Require().NoError(err) // want "go-require: do not use require in http handlers"
+
+	data, err := io.ReadAll(file)
+	if !s.NoError(err) {
+		return
+	}
+
+	w.Header().Set("Content-Type", "application/json")
+	_, err = w.Write(data)
+	if !s.NoError(err) {
+		s.FailNow(err.Error()) // want "go-require: do not use s\\.FailNow in http handlers"
+	}
+}
+
+func (s *ServerSuite) handler(w http.ResponseWriter, _ *http.Request) {
+	s.T().Helper()
+
+	file, err := os.Open("some file.json")
+	s.Require().NoError(err) // want "go-require: do not use require in http handlers"
+
+	data, err := io.ReadAll(file)
+	if !s.NoError(err) {
+		return
+	}
+
+	w.Header().Set("Content-Type", "application/json")
+	_, err = w.Write(data)
+	if !s.NoError(err) {
+		s.FailNow(err.Error()) // want "go-require: do not use s\\.FailNow in http handlers"
+	}
+}

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

@@ -0,0 +1,81 @@
+package gorequireignorehttphandlers_test
+
+import (
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
+)
+
+func TestServer_Require(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		file, err := os.Open("some file.json")
+		require.NoError(t, err)
+
+		data, err := io.ReadAll(file)
+		require.NoError(t, err)
+
+		w.Header().Set("Content-Type", "application/json")
+		_, err = w.Write(data)
+		if !assert.NoError(t, err) {
+			assert.FailNow(t, err.Error())
+		}
+	}))
+	defer ts.Close()
+
+	client := ts.Client()
+	client.Timeout = 10 * time.Second
+
+	req, err := http.NewRequest("GET", ts.URL+"/require", nil)
+	require.NoError(t, err)
+
+	statusCode := make(chan int)
+	go func() {
+		resp, err := client.Do(req)
+		require.NoError(t, err) // want "go-require: require must only be used in the goroutine running the test function"
+		defer func() {
+			require.NoError(t, resp.Body.Close()) // want "go-require: require must only be used in the goroutine running the test function"
+		}()
+		statusCode <- resp.StatusCode
+	}()
+
+	require.Equal(t, http.StatusOK, <-statusCode)
+}
+
+type SomeServerSuite struct {
+	suite.Suite
+}
+
+func TestSomeServerSuite(t *testing.T) {
+	suite.Run(t, &SomeServerSuite{})
+}
+
+func (s *SomeServerSuite) TestServer() {
+	httptest.NewServer(http.HandlerFunc(s.handler))
+	httptest.NewServer(s)
+}
+
+func (s *SomeServerSuite) ServeHTTP(hres http.ResponseWriter, hreq *http.Request) {
+	var req MyRequest
+	err := json.NewDecoder(hreq.Body).Decode(&req)
+	s.Require().NoError(err)
+	s.Equal("42", req.ID)
+}
+
+func (s *SomeServerSuite) handler(hres http.ResponseWriter, hreq *http.Request) {
+	var req MyRequest
+	err := json.NewDecoder(hreq.Body).Decode(&req)
+	s.Require().NoError(err)
+	s.Equal("42", req.ID)
+}
+
+type MyRequest struct {
+	ID string
+}