Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go-require: require-error: support HTTP handlers #90

Merged
merged 3 commits into from
May 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 ./...
```
Expand Down Expand Up @@ -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) ❌
}
}()
Expand Down Expand Up @@ -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.

---

Expand Down Expand Up @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
19 changes: 16 additions & 3 deletions analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions analyzer/checkers_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
14 changes: 14 additions & 0 deletions analyzer/checkers_factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
}
}
Original file line number Diff line number Diff line change
@@ -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
}

This file was deleted.

Loading
Loading