From 7b8d8297600d53af3b28b6cecf804a316a9dc4f2 Mon Sep 17 00:00:00 2001 From: Xabier Larrakoetxea Date: Mon, 1 Jun 2020 13:16:29 +0200 Subject: [PATCH 1/3] Improve unit tests on middlewares Signed-off-by: Xabier Larrakoetxea --- go.mod | 2 +- go.sum | 7 +- middleware/echo/echo.go | 3 +- middleware/echo/echo_test.go | 92 +++++++++------- middleware/gin/gin.go | 3 +- middleware/gin/gin_test.go | 115 +++++++++++++------- middleware/gorestful/gorestful.go | 3 +- middleware/gorestful/gorestful_test.go | 84 ++++++++------- middleware/httprouter/httprouter.go | 5 +- middleware/httprouter/httprouter_test.go | 84 ++++++++------- middleware/negroni/negroni.go | 3 +- middleware/negroni/negroni_test.go | 83 ++++++++------- middleware/std/std.go | 1 + middleware/std/std_test.go | 127 ++++++++++------------- 14 files changed, 335 insertions(+), 277 deletions(-) diff --git a/go.mod b/go.mod index 8121f1a..ff8e37f 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/julienschmidt/httprouter v1.3.0 github.com/labstack/echo/v4 v4.1.16 github.com/prometheus/client_golang v1.6.0 - github.com/stretchr/testify v1.5.1 + github.com/stretchr/testify v1.6.0 github.com/urfave/negroni v1.0.0 go.opencensus.io v0.22.3 ) diff --git a/go.sum b/go.sum index ebdae8f..de0b2fc 100644 --- a/go.sum +++ b/go.sum @@ -106,6 +106,7 @@ github.com/modern-go/reflect2 v1.0.1 h1:9f412s+6RmYXLWZSEzVVgPGK7C2PphHj5RJrvfx9 github.com/modern-go/reflect2 v1.0.1/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= @@ -143,8 +144,8 @@ github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= -github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4= -github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= +github.com/stretchr/testify v1.6.0 h1:jlIyCplCJFULU/01vCkhKuTyc3OorI3bJFuw6obfgho= +github.com/stretchr/testify v1.6.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/ugorji/go v1.1.7 h1:/68gy2h+1mWMrwZFeD1kQialdSzAb432dtpeJ42ovdo= github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw= github.com/ugorji/go/codec v1.1.7 h1:2SvQaVZ1ouYrrKKwoSk2pzd4A9evlKJb9oTL+OaLUSs= @@ -241,4 +242,6 @@ gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.5/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10= gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= diff --git a/middleware/echo/echo.go b/middleware/echo/echo.go index 8fcc1c3..62d75bd 100644 --- a/middleware/echo/echo.go +++ b/middleware/echo/echo.go @@ -1,5 +1,4 @@ -// Package echo is a helper package to get an echo compatible -// handler/middleware from the standard net/http Middleware factory. +// Package echo is a helper package to get an echo compatible middleware package echo import ( diff --git a/middleware/echo/echo_test.go b/middleware/echo/echo_test.go index cccc00e..be9a8ee 100644 --- a/middleware/echo/echo_test.go +++ b/middleware/echo/echo_test.go @@ -1,79 +1,91 @@ package echo_test import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + mmetrics "github.com/slok/go-http-metrics/internal/mocks/metrics" "github.com/slok/go-http-metrics/metrics" "github.com/slok/go-http-metrics/middleware" echoMiddleware "github.com/slok/go-http-metrics/middleware/echo" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" ) -func getTestHandler(statusCode int) echo.HandlerFunc { - return func(context echo.Context) error { - context.Response().WriteHeader(statusCode) - return nil - } -} - func TestMiddleware(t *testing.T) { tests := map[string]struct { - handlerID string - statusCode int - req *http.Request - config middleware.Config - expHandlerID string - expService string - expMethod string - expStatusCode string + handlerID string + config middleware.Config + req func() *http.Request + mock func(m *mmetrics.Recorder) + handler func() echo.HandlerFunc + expRespCode int + expRespBody string }{ "A default HTTP middleware should call the recorder to measure.": { - statusCode: http.StatusAccepted, - req: httptest.NewRequest(http.MethodPost, "/test", nil), - expHandlerID: "/test", - expMethod: http.MethodPost, - expStatusCode: "202", + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(5)).Once() + + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() echo.HandlerFunc { + return func(context echo.Context) error { + resp := context.Response() + resp.WriteHeader(202) + _, err := resp.Write([]byte("test1")) + return err + } + }, + expRespCode: 202, + expRespBody: "test1", }, } for name, test := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) + require := require.New(t) // Mocks. mr := &mmetrics.Recorder{} - expHTTPReqProps := metrics.HTTPReqProperties{ - ID: test.expHandlerID, - Service: test.expService, - Method: test.expMethod, - Code: test.expStatusCode, - } - expHTTPProps := metrics.HTTPProperties{ - ID: test.expHandlerID, - Service: test.expService, - } - mr.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + test.mock(mr) - // Create our echo instance with the middleware. + // Create our instance with the middleware. mdlw := middleware.New(middleware.Config{Recorder: mr}) e := echo.New() - e.POST("/test", getTestHandler(test.statusCode), echoMiddleware.Handler("", mdlw)) + req := test.req() + e.Add(req.Method, req.URL.Path, test.handler(), echoMiddleware.Handler("", mdlw)) // Make the request. resp := httptest.NewRecorder() - e.ServeHTTP(resp, test.req) + e.ServeHTTP(resp, test.req()) // Check. mr.AssertExpectations(t) - assert.Equal(test.statusCode, resp.Result().StatusCode) + assert.Equal(test.expRespCode, resp.Result().StatusCode) + gotBody, err := ioutil.ReadAll(resp.Result().Body) + require.NoError(err) + assert.Equal(test.expRespBody, string(gotBody)) }) } } diff --git a/middleware/gin/gin.go b/middleware/gin/gin.go index 863d6bd..06ca7a1 100644 --- a/middleware/gin/gin.go +++ b/middleware/gin/gin.go @@ -1,5 +1,4 @@ -// Package gin is a helper package to get a gin compatible -// handler/middleware from the standard net/http Middleware factory. +// Package gin is a helper package to get a gin compatible middleware. package gin import ( diff --git a/middleware/gin/gin_test.go b/middleware/gin/gin_test.go index 835add3..344a568 100644 --- a/middleware/gin/gin_test.go +++ b/middleware/gin/gin_test.go @@ -1,6 +1,7 @@ package gin_test import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" @@ -8,6 +9,7 @@ import ( "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" mmetrics "github.com/slok/go-http-metrics/internal/mocks/metrics" "github.com/slok/go-http-metrics/metrics" @@ -15,67 +17,104 @@ import ( ginmiddleware "github.com/slok/go-http-metrics/middleware/gin" ) -func getTestHandler(statusCode int) gin.HandlerFunc { - return func(c *gin.Context) { - c.String(statusCode, "Hello world") - } -} - func TestMiddleware(t *testing.T) { tests := map[string]struct { - handlerID string - statusCode int - req *http.Request - config middleware.Config - expHandlerID string - expService string - expMethod string - expStatusCode string + handlerID string + config middleware.Config + req func() *http.Request + mock func(m *mmetrics.Recorder) + handler func() gin.HandlerFunc + expRespCode int + expRespBody string }{ "A default HTTP middleware should call the recorder to measure.": { - statusCode: http.StatusAccepted, - req: httptest.NewRequest(http.MethodPost, "/test", nil), - expHandlerID: "/test", - expMethod: http.MethodPost, - expStatusCode: "202", + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(5)).Once() + + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() gin.HandlerFunc { + return func(c *gin.Context) { + c.String(202, "test1") + } + }, + expRespCode: 202, + expRespBody: "test1", + }, + + "A default HTTP middleware using JSON should call the recorder to measure (Regression test: https://github.com/slok/go-http-metrics/issues/31).": { + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(14)).Once() + + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() gin.HandlerFunc { + return func(c *gin.Context) { + c.JSON(202, map[string]string{"test": "one"}) + } + }, + expRespCode: 202, + expRespBody: `{"test":"one"}`, }, } for name, test := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) + require := require.New(t) // Mocks. mr := &mmetrics.Recorder{} - expHTTPReqProps := metrics.HTTPReqProperties{ - ID: test.expHandlerID, - Service: test.expService, - Method: test.expMethod, - Code: test.expStatusCode, - } - expHTTPProps := metrics.HTTPProperties{ - ID: test.expHandlerID, - Service: test.expService, - } - mr.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + test.mock(mr) // Create our instance with the middleware. mdlw := middleware.New(middleware.Config{Recorder: mr}) engine := gin.New() - engine.POST("/test", - ginmiddleware.Handler("", mdlw), - getTestHandler(test.statusCode)) + req := test.req() + engine.Handle(req.Method, req.URL.Path, + ginmiddleware.Handler(test.handlerID, mdlw), + test.handler()) // Make the request. resp := httptest.NewRecorder() - engine.ServeHTTP(resp, test.req) + engine.ServeHTTP(resp, req) // Check. mr.AssertExpectations(t) - assert.Equal(test.statusCode, resp.Result().StatusCode) + assert.Equal(test.expRespCode, resp.Result().StatusCode) + gotBody, err := ioutil.ReadAll(resp.Result().Body) + require.NoError(err) + assert.Equal(test.expRespBody, string(gotBody)) }) } } diff --git a/middleware/gorestful/gorestful.go b/middleware/gorestful/gorestful.go index fec6a93..4a2fec2 100644 --- a/middleware/gorestful/gorestful.go +++ b/middleware/gorestful/gorestful.go @@ -1,5 +1,4 @@ -// Package gorestful is a helper package to get a gorestful compatible -// handler/middleware from the standard net/http Middleware factory. +// Package gorestful is a helper package to get a gorestful compatible middleware. package gorestful import ( diff --git a/middleware/gorestful/gorestful_test.go b/middleware/gorestful/gorestful_test.go index fa7396c..fae067b 100644 --- a/middleware/gorestful/gorestful_test.go +++ b/middleware/gorestful/gorestful_test.go @@ -1,6 +1,7 @@ package gorestful_test import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" @@ -8,6 +9,7 @@ import ( gorestful "github.com/emicklei/go-restful" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" mmetrics "github.com/slok/go-http-metrics/internal/mocks/metrics" "github.com/slok/go-http-metrics/metrics" @@ -15,69 +17,77 @@ import ( gorestfulmiddleware "github.com/slok/go-http-metrics/middleware/gorestful" ) -func getTestHandler(statusCode int) gorestful.RouteFunction { - return gorestful.RouteFunction(func(_ *gorestful.Request, resp *gorestful.Response) { - _ = resp.WriteHeaderAndEntity(statusCode, "Hello world") - }) -} - func TestMiddleware(t *testing.T) { tests := map[string]struct { - handlerID string - statusCode int - req *http.Request - config middleware.Config - expHandlerID string - expService string - expMethod string - expStatusCode string + handlerID string + config middleware.Config + req func() *http.Request + mock func(m *mmetrics.Recorder) + handler func() gorestful.RouteFunction + expRespCode int + expRespBody string }{ "A default HTTP middleware should call the recorder to measure.": { - statusCode: http.StatusAccepted, - req: httptest.NewRequest(http.MethodPost, "/test", nil), - expHandlerID: "/test", - expMethod: http.MethodPost, - expStatusCode: "202", + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(5)).Once() + + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() gorestful.RouteFunction { + return gorestful.RouteFunction(func(_ *gorestful.Request, resp *gorestful.Response) { + resp.WriteHeader(202) + resp.Write([]byte("test1")) // nolint: errcheck + }) + }, + expRespCode: 202, + expRespBody: "test1", }, } for name, test := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) + require := require.New(t) // Mocks. mr := &mmetrics.Recorder{} - expHTTPReqProps := metrics.HTTPReqProperties{ - ID: test.expHandlerID, - Service: test.expService, - Method: test.expMethod, - Code: test.expStatusCode, - } - expHTTPProps := metrics.HTTPProperties{ - ID: test.expHandlerID, - Service: test.expService, - } - mr.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + test.mock(mr) // Create our instance with the middleware. mdlw := middleware.New(middleware.Config{Recorder: mr}) c := gorestful.NewContainer() - c.Filter(gorestfulmiddleware.Handler("", mdlw)) + c.Filter(gorestfulmiddleware.Handler(test.handlerID, mdlw)) ws := &gorestful.WebService{} ws.Produces(gorestful.MIME_JSON) - ws.Route(ws.POST("/test").To(getTestHandler(test.statusCode))) + req := test.req() + ws.Route(ws.Method(req.Method).Path(req.URL.Path).To(test.handler())) c.Add(ws) // Make the request. resp := httptest.NewRecorder() - c.ServeHTTP(resp, test.req) + c.ServeHTTP(resp, req) // Check. mr.AssertExpectations(t) - assert.Equal(test.statusCode, resp.Result().StatusCode) + assert.Equal(test.expRespCode, resp.Result().StatusCode) + gotBody, err := ioutil.ReadAll(resp.Result().Body) + require.NoError(err) + assert.Equal(test.expRespBody, string(gotBody)) }) } } diff --git a/middleware/httprouter/httprouter.go b/middleware/httprouter/httprouter.go index edece1b..f037e9b 100644 --- a/middleware/httprouter/httprouter.go +++ b/middleware/httprouter/httprouter.go @@ -1,5 +1,4 @@ -// Package httprouter is a helper package to get a httprouter compatible -// handler/middleware from the standatd net/http Middleware factory. +// Package httprouter is a helper package to get a httprouter compatible middleware. package httprouter import ( @@ -14,7 +13,7 @@ import ( // Handler returns a httprouter.Handler measuring middleware. func Handler(handlerID string, next httprouter.Handle, m middleware.Middleware) httprouter.Handle { return func(w http.ResponseWriter, r *http.Request, p httprouter.Params) { - // Dummy handler to wrap httprouter Handle type + // Dummy handler to wrap httprouter Handle type. h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { next(w, r, p) }) diff --git a/middleware/httprouter/httprouter_test.go b/middleware/httprouter/httprouter_test.go index 1e12df3..bcb47e7 100644 --- a/middleware/httprouter/httprouter_test.go +++ b/middleware/httprouter/httprouter_test.go @@ -1,6 +1,7 @@ package httprouter_test import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" @@ -8,6 +9,7 @@ import ( "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" mmetrics "github.com/slok/go-http-metrics/internal/mocks/metrics" "github.com/slok/go-http-metrics/metrics" @@ -15,65 +17,75 @@ import ( httproutermiddleware "github.com/slok/go-http-metrics/middleware/httprouter" ) -func getTestHandler(statusCode int) httprouter.Handle { - return httprouter.Handle(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) { - w.WriteHeader(statusCode) - }) -} - func TestMiddleware(t *testing.T) { tests := map[string]struct { - handlerID string - statusCode int - req *http.Request - config middleware.Config - expHandlerID string - expService string - expMethod string - expStatusCode string + handlerID string + config middleware.Config + req func() *http.Request + mock func(m *mmetrics.Recorder) + handler func() httprouter.Handle + expRespCode int + expRespBody string }{ "A default HTTP middleware should call the recorder to measure.": { - statusCode: http.StatusAccepted, - req: httptest.NewRequest(http.MethodPost, "/test", nil), - expHandlerID: "/test", - expMethod: http.MethodPost, - expStatusCode: "202", + handlerID: "", + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(5)).Once() + + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() httprouter.Handle { + return httprouter.Handle(func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + w.WriteHeader(202) + w.Write([]byte("test1")) // nolint: errcheck + }) + }, + expRespCode: 202, + expRespBody: "test1", }, } for name, test := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) + require := require.New(t) // Mocks. mr := &mmetrics.Recorder{} - expHTTPReqProps := metrics.HTTPReqProperties{ - ID: test.expHandlerID, - Service: test.expService, - Method: test.expMethod, - Code: test.expStatusCode, - } - expHTTPProps := metrics.HTTPProperties{ - ID: test.expHandlerID, - Service: test.expService, - } - mr.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + test.mock(mr) // Create our instance with the middleware. mdlw := middleware.New(middleware.Config{Recorder: mr}) r := httprouter.New() - r.POST("/test", httproutermiddleware.Handler("", getTestHandler(test.statusCode), mdlw)) + h := httproutermiddleware.Handler(test.handlerID, test.handler(), mdlw) + req := test.req() + r.Handle(req.Method, req.URL.Path, h) // Make the request. resp := httptest.NewRecorder() - r.ServeHTTP(resp, test.req) + r.ServeHTTP(resp, req) // Check. mr.AssertExpectations(t) - assert.Equal(test.statusCode, resp.Result().StatusCode) + assert.Equal(test.expRespCode, resp.Result().StatusCode) + gotBody, err := ioutil.ReadAll(resp.Result().Body) + require.NoError(err) + assert.Equal(test.expRespBody, string(gotBody)) }) } } diff --git a/middleware/negroni/negroni.go b/middleware/negroni/negroni.go index b374ea5..066094e 100644 --- a/middleware/negroni/negroni.go +++ b/middleware/negroni/negroni.go @@ -1,5 +1,4 @@ -// Package negroni is a helper package to get a negroni compatible -// handler/middleware from the standard net/http Middleware factory. +// Package negroni is a helper package to get a negroni compatible middleware. package negroni import ( diff --git a/middleware/negroni/negroni_test.go b/middleware/negroni/negroni_test.go index e9666d8..03058c4 100644 --- a/middleware/negroni/negroni_test.go +++ b/middleware/negroni/negroni_test.go @@ -1,12 +1,14 @@ package negroni_test import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "github.com/urfave/negroni" mmetrics "github.com/slok/go-http-metrics/internal/mocks/metrics" @@ -15,66 +17,73 @@ import ( negronimiddleware "github.com/slok/go-http-metrics/middleware/negroni" ) -func getTestHandler(statusCode int) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(statusCode) - }) -} - func TestMiddleware(t *testing.T) { tests := map[string]struct { - handlerID string - statusCode int - req *http.Request - config middleware.Config - expHandlerID string - expService string - expMethod string - expStatusCode string + handlerID string + config middleware.Config + req func() *http.Request + mock func(m *mmetrics.Recorder) + handler func() http.Handler + expRespCode int + expRespBody string }{ "A default HTTP middleware should call the recorder to measure.": { - statusCode: http.StatusAccepted, - req: httptest.NewRequest(http.MethodPost, "/test", nil), - expHandlerID: "/test", - expMethod: http.MethodPost, - expStatusCode: "202", + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(5)).Once() + + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(202) + w.Write([]byte("test1")) // nolint: errcheck + }) + }, + expRespCode: 202, + expRespBody: "test1", }, } for name, test := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) + require := require.New(t) // Mocks. mr := &mmetrics.Recorder{} - expHTTPReqProps := metrics.HTTPReqProperties{ - ID: test.expHandlerID, - Service: test.expService, - Method: test.expMethod, - Code: test.expStatusCode, - } - expHTTPProps := metrics.HTTPProperties{ - ID: test.expHandlerID, - Service: test.expService, - } - mr.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + test.mock(mr) // Create our negroni instance with the middleware. mdlw := middleware.New(middleware.Config{Recorder: mr}) n := negroni.Classic() - n.Use(negronimiddleware.Handler("", mdlw)) - n.UseHandler(getTestHandler(test.statusCode)) + n.Use(negronimiddleware.Handler(test.handlerID, mdlw)) + n.UseHandler(test.handler()) // Make the request. resp := httptest.NewRecorder() - n.ServeHTTP(resp, test.req) + n.ServeHTTP(resp, test.req()) // Check. mr.AssertExpectations(t) - assert.Equal(test.statusCode, resp.Result().StatusCode) + assert.Equal(test.expRespCode, resp.Result().StatusCode) + gotBody, err := ioutil.ReadAll(resp.Result().Body) + require.NoError(err) + assert.Equal(test.expRespBody, string(gotBody)) }) } } diff --git a/middleware/std/std.go b/middleware/std/std.go index f91b24c..0e9fd29 100644 --- a/middleware/std/std.go +++ b/middleware/std/std.go @@ -1,3 +1,4 @@ +// Package std is a helper package to get a standard `http.Handler` compatible middleware. package std import ( diff --git a/middleware/std/std_test.go b/middleware/std/std_test.go index f927bfc..fa6c0d4 100644 --- a/middleware/std/std_test.go +++ b/middleware/std/std_test.go @@ -1,11 +1,14 @@ package std_test import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" mmetrics "github.com/slok/go-http-metrics/internal/mocks/metrics" "github.com/slok/go-http-metrics/metrics" @@ -13,98 +16,72 @@ import ( stdmiddleware "github.com/slok/go-http-metrics/middleware/std" ) -func getFakeHandler(statusCode int, responseBody string) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(statusCode) - _, _ = w.Write([]byte(responseBody)) - }) -} - func TestMiddleware(t *testing.T) { tests := map[string]struct { - handlerID string - body string - statusCode int - req *http.Request - config middleware.Config - expHandlerID string - expService string - expMethod string - expSize int64 - expStatusCode string + handlerID string + config middleware.Config + req func() *http.Request + mock func(m *mmetrics.Recorder) + handler func() http.Handler + expRespCode int + expRespBody string }{ "A default HTTP middleware should call the recorder to measure.": { - statusCode: http.StatusAccepted, - body: "Я бэтмен", - req: httptest.NewRequest(http.MethodGet, "/test", nil), - expHandlerID: "/test", - expService: "", - expSize: 15, - expMethod: http.MethodGet, - expStatusCode: "202", - }, + req: func() *http.Request { + return httptest.NewRequest(http.MethodPost, "/test", nil) + }, + mock: func(m *mmetrics.Recorder) { + expHTTPReqProps := metrics.HTTPReqProperties{ + ID: "/test", + Service: "", + Method: "POST", + Code: "202", + } + m.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() + m.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, int64(15)).Once() - "Using custom ID in the middleware should call the recorder to measure with that ID.": { - handlerID: "customID", - body: "I'm Batman", - statusCode: http.StatusTeapot, - req: httptest.NewRequest(http.MethodPost, "/test", nil), - expHandlerID: "customID", - expService: "", - expSize: 10, - expMethod: http.MethodPost, - expStatusCode: "418", - }, - - "Using grouped status code should group the status code.": { - config: middleware.Config{GroupedStatus: true}, - statusCode: http.StatusGatewayTimeout, - req: httptest.NewRequest(http.MethodPatch, "/test", nil), - expHandlerID: "/test", - expService: "", - expMethod: http.MethodPatch, - expStatusCode: "5xx", - }, - - "Using the service middleware option should set the service on the metrics.": { - config: middleware.Config{Service: "Yoda"}, - statusCode: http.StatusContinue, - body: "May the force be with you", - req: httptest.NewRequest(http.MethodGet, "/test", nil), - expHandlerID: "/test", - expService: "Yoda", - expSize: 25, - expMethod: http.MethodGet, - expStatusCode: "100", + expHTTPProps := metrics.HTTPProperties{ + ID: "/test", + Service: "", + } + m.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() + m.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + }, + handler: func() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(202) + w.Write([]byte("Я бэтмен")) // nolint: errcheck + }) + }, + expRespCode: 202, + expRespBody: "Я бэтмен", }, } for name, test := range tests { t.Run(name, func(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + // Mocks. mr := &mmetrics.Recorder{} - expHTTPReqProps := metrics.HTTPReqProperties{ - ID: test.expHandlerID, - Service: test.expService, - Method: test.expMethod, - Code: test.expStatusCode, - } - expHTTPProps := metrics.HTTPProperties{ - ID: test.expHandlerID, - Service: test.expService, - } - mr.On("ObserveHTTPRequestDuration", mock.Anything, expHTTPReqProps, mock.Anything).Once() - mr.On("ObserveHTTPResponseSize", mock.Anything, expHTTPReqProps, test.expSize).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, 1).Once() - mr.On("AddInflightRequests", mock.Anything, expHTTPProps, -1).Once() + test.mock(mr) - // Make the request. + // Create our negroni instance with the middleware. test.config.Recorder = mr m := middleware.New(test.config) - h := stdmiddleware.Handler(test.handlerID, m, getFakeHandler(test.statusCode, test.body)) - h.ServeHTTP(httptest.NewRecorder(), test.req) + h := stdmiddleware.Handler(test.handlerID, m, test.handler()) + + // Make the request. + resp := httptest.NewRecorder() + h.ServeHTTP(resp, test.req()) + // Check. mr.AssertExpectations(t) + assert.Equal(test.expRespCode, resp.Result().StatusCode) + gotBody, err := ioutil.ReadAll(resp.Result().Body) + require.NoError(err) + assert.Equal(test.expRespBody, string(gotBody)) }) } } From 5303b8a95f8bad821087f887305fa1cac7e0c151 Mon Sep 17 00:00:00 2001 From: Xabier Larrakoetxea Date: Mon, 1 Jun 2020 13:36:33 +0200 Subject: [PATCH 2/3] Add coverage Signed-off-by: Xabier Larrakoetxea --- .gitignore | 3 ++- Makefile | 13 +++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index a725465..5e1d2f9 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ -vendor/ \ No newline at end of file +vendor/ +.test_coverage.txt \ No newline at end of file diff --git a/Makefile b/Makefile index 79a3ec0..dfbe76e 100644 --- a/Makefile +++ b/Makefile @@ -1,10 +1,11 @@ -UNIT_TEST_CMD := go test `go list ./... | grep -v test\/integration` -race -INTEGRATION_TEST_CMD := go test ./test/integration -race -tags='integration' -BENCHMARK_CMD := go test `go list ./... | grep -v vendor` -benchmem -bench=. -CHECK_CMD = golangci-lint run -E goimports -DEPS_CMD := go mod tidy -MOCKS_CMD := go generate ./internal/mocks +UNIT_TEST_CMD := go test `go list ./... | grep -v test\/integration` -race -coverprofile=.test_coverage.txt && \ + go tool cover -func=.test_coverage.txt | tail -n1 | awk '{print "Total test coverage: " $$3}' +INTEGRATION_TEST_CMD := go test ./test/integration -race -tags='integration' +BENCHMARK_CMD := go test `go list ./... | grep -v vendor` -benchmem -bench=. +CHECK_CMD := golangci-lint run -E goimports +DEPS_CMD := go mod tidy +MOCKS_CMD := go generate ./internal/mocks help: ## Show this help. @echo "Help" From 5202197ce8ad72f390f915002a09bcdc8e7dde1a Mon Sep 17 00:00:00 2001 From: Xabier Larrakoetxea Date: Mon, 1 Jun 2020 13:37:29 +0200 Subject: [PATCH 3/3] Fix test Signed-off-by: Xabier Larrakoetxea --- middleware/echo/echo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/middleware/echo/echo_test.go b/middleware/echo/echo_test.go index be9a8ee..3ada1eb 100644 --- a/middleware/echo/echo_test.go +++ b/middleware/echo/echo_test.go @@ -74,7 +74,7 @@ func TestMiddleware(t *testing.T) { mdlw := middleware.New(middleware.Config{Recorder: mr}) e := echo.New() req := test.req() - e.Add(req.Method, req.URL.Path, test.handler(), echoMiddleware.Handler("", mdlw)) + e.Add(req.Method, req.URL.Path, test.handler(), echoMiddleware.Handler(test.handlerID, mdlw)) // Make the request. resp := httptest.NewRecorder()