ronantakizawa/codereview-bench huggingface dataset:
Repository: labstack/echo
File: middleware/proxy_test.go (Go)

Code before:
	assert.Condition(t, func() bool {
		return expected[body]
	})

	for _, target := range targets {
		assert.True(t, rb.RemoveTarget(target.Name))
	}

	assert.False(t, rb.RemoveTarget("unknown target"))

	// Round-robin
	rrb := NewRoundRobinBalancer(targets)
	e = echo.New()
	e.Use(Proxy(rrb))

	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	body = rec.Body.String()
	assert.Equal(t, "target 1", body)

	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	body = rec.Body.String()
	assert.Equal(t, "target 2", body)

	// Provider
	e = echo.New()
	tp := &testProvider{commonBalancer: new(commonBalancer)}
	tp.targets = targets
	e.Use(Proxy(tp))
	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	body = rec.Body.String()
	assert.Equal(t, "target 2", body)

	// ModifyResponse
	e = echo.New()
	e.Use(ProxyWithConfig(ProxyConfig{
		Balancer: rrb,
		ModifyResponse: func(res *http.Response) error {
			res.Body = ioutil.NopCloser(bytes.NewBuffer([]byte("modified")))
			res.Header.Set("X-Modified", "1")
			return nil
		},
	}))

	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	assert.Equal(t, "modified", rec.Body.String())
	assert.Equal(t, "1", rec.Header().Get("X-Modified"))


Review comment (suggestion):
please create separate function for this testcase. These tests (functions) that contain actually multiple testcases that sometimes reuse objects are bad practice. If test cases are isolated it is easier to test them separately and overall have less sideeffects.

Code after:
	e = echo.New()
	e.Use(Proxy(rrb))

	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	body = rec.Body.String()
	assert.Equal(t, "target 1", body)

	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	body = rec.Body.String()
	assert.Equal(t, "target 2", body)

	// ModifyResponse
	e = echo.New()
	e.Use(ProxyWithConfig(ProxyConfig{
		Balancer: rrb,
		ModifyResponse: func(res *http.Response) error {
			res.Body = ioutil.NopCloser(bytes.NewBuffer([]byte("modified")))
			res.Header.Set("X-Modified", "1")
			return nil
		},
	}))

	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	assert.Equal(t, "modified", rec.Body.String())
	assert.Equal(t, "1", rec.Header().Get("X-Modified"))

	// ProxyTarget is set in context
	contextObserver := func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) (err error) {
			next(c)
			assert.Contains(t, targets, c.Get("target"), "target is not set in context")
			return nil
		}
	}
	rrb1 := NewRoundRobinBalancer(targets)

	e = echo.New()
	e.Use(contextObserver)
	e.Use(Proxy(rrb1))
	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
}

type testProvider struct {
	*commonBalancer
	target *ProxyTarget
	err    error
}
Repository: labstack/echo
File: middleware/context_timeout.go (Go)

Code before:
	Timeout time.Duration
}

var (
	// DefaultContextTimeoutErrorHandler is default error handler of ContextTimeout middleware.
	DefaultContextTimeoutErrorHandler = func(err error, c echo.Context) error {
		if err != nil {
			if errors.Is(err, context.DeadlineExceeded) {
				return echo.ErrServiceUnavailable
			}
			return err
		}
		return nil
	}

	// DefaultContextTimeoutConfig is the default ContextTimeoutConfig middleware config.
	DefaultContextTimeoutConfig = ContextTimeoutConfig{
		Skipper:      DefaultSkipper,
		Timeout:      0,
		ErrorHandler: DefaultContextTimeoutErrorHandler,
	}
)

// ContextTimeout returns a middleware which returns error (503 Service Unavailable error) to client
// when underlying method returns context.DeadlineExceeded error.
func ContextTimeout() echo.MiddlewareFunc {
	return ContextTimeoutWithConfig(DefaultContextTimeoutConfig)
}

// ContextTimeoutWithConfig returns a Timeout middleware with config.
func ContextTimeoutWithConfig(config ContextTimeoutConfig) echo.MiddlewareFunc {
	return config.ToMiddleware()
}

// ToMiddleware converts Config to middleware.
func (config ContextTimeoutConfig) ToMiddleware() echo.MiddlewareFunc {
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			if (config.Skipper != nil && config.Skipper(c)) || config.Timeout == 0 {
				return next(c)
			}

			timeoutContext, cancel := context.WithTimeout(c.Request().Context(), config.Timeout)
			defer cancel()

			c.SetRequest(c.Request().WithContext(timeoutContext))

			err := next(c)
			if err != nil {
				if config.ErrorHandler != nil {
					return config.ErrorHandler(err, c)

Review comment (suggestion):
This function should have 1 argument `timeout time.Duration`. Creating this middleware with timeout as 0 makes no sense.

Code after:
	}
	return mw
}

// ToMiddleware converts Config to middleware.
func (config ContextTimeoutConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
	if config.Timeout == 0 {
		return nil, errors.New("timeout must be set")
	}
	if config.Skipper == nil {
		config.Skipper = DefaultSkipper
	}
	if config.ErrorHandler == nil {
		config.ErrorHandler = func(err error, c echo.Context) error {
			if err != nil && errors.Is(err, context.DeadlineExceeded) {
				return echo.ErrServiceUnavailable.WithInternal(err)
			}
			return err
		}
	}

	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			if config.Skipper(c) {
				return next(c)
			}

			timeoutContext, cancel := context.WithTimeout(c.Request().Context(), config.Timeout)
			defer cancel()

			c.SetRequest(c.Request().WithContext(timeoutContext))

			if err := next(c); err != nil {
				return config.ErrorHandler(err, c)
			}
			return nil
		}
	}, nil
}
Repository: labstack/echo
File: middleware/context_timeout.go (Go)

Code before:
			return err
		}
		return nil
	}

	// DefaultContextTimeoutConfig is the default ContextTimeoutConfig middleware config.
	DefaultContextTimeoutConfig = ContextTimeoutConfig{
		Skipper:      DefaultSkipper,
		Timeout:      0,
		ErrorHandler: DefaultContextTimeoutErrorHandler,
	}
)

// ContextTimeout returns a middleware which returns error (503 Service Unavailable error) to client
// when underlying method returns context.DeadlineExceeded error.
func ContextTimeout() echo.MiddlewareFunc {
	return ContextTimeoutWithConfig(DefaultContextTimeoutConfig)
}

// ContextTimeoutWithConfig returns a Timeout middleware with config.
func ContextTimeoutWithConfig(config ContextTimeoutConfig) echo.MiddlewareFunc {
	return config.ToMiddleware()
}

// ToMiddleware converts Config to middleware.
func (config ContextTimeoutConfig) ToMiddleware() echo.MiddlewareFunc {
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			if (config.Skipper != nil && config.Skipper(c)) || config.Timeout == 0 {
				return next(c)
			}

			timeoutContext, cancel := context.WithTimeout(c.Request().Context(), config.Timeout)
			defer cancel()

			c.SetRequest(c.Request().WithContext(timeoutContext))

			err := next(c)
			if err != nil {
				if config.ErrorHandler != nil {
					return config.ErrorHandler(err, c)
				} else {
					return DefaultContextTimeoutErrorHandler(err, c)
				}
			}
			return nil
		}
	}
}


Review comment (suggestion):
this method should return `(echo.MiddlewareFunc, error)` as `ToMiddleware()` are meant to be way to create middlewares with possibility of errors. See how these errors should be handled by other creator functions https://github.com/labstack/echo/blob/24a30611dfc07e427dc771a16ef9bb0dd94c4c2e/middleware/request_logger.go#L215-L220

Code after:
		config.Skipper = DefaultSkipper
	}
	if config.ErrorHandler == nil {
		config.ErrorHandler = func(err error, c echo.Context) error {
			if err != nil && errors.Is(err, context.DeadlineExceeded) {
				return echo.ErrServiceUnavailable.WithInternal(err)
			}
			return err
		}
	}

	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			if config.Skipper(c) {
				return next(c)
			}

			timeoutContext, cancel := context.WithTimeout(c.Request().Context(), config.Timeout)
			defer cancel()

			c.SetRequest(c.Request().WithContext(timeoutContext))

			if err := next(c); err != nil {
				return config.ErrorHandler(err, c)
			}
			return nil
		}
	}, nil
}
Repository: labstack/echo
File: middleware/context_timeout_test.go (Go)

Code before:
	})(c)

	assert.NoError(t, err)
}

func TestContextTimeoutRecoversPanic(t *testing.T) {
	t.Parallel()
	e := echo.New()
	e.Use(Recover()) // recover middleware will handler our panic
	e.Use(ContextTimeoutWithConfig(ContextTimeoutConfig{
		Timeout: 50 * time.Millisecond,
	}))

	e.GET("/", func(c echo.Context) error {
		panic("panic!!!")
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	assert.NotPanics(t, func() {
		e.ServeHTTP(rec, req)
	})
}

func TestContextTimeoutDataRace(t *testing.T) {
	t.Parallel()

	timeout := 1 * time.Millisecond
	m := ContextTimeoutWithConfig(ContextTimeoutConfig{
		Timeout: timeout,
		//ErrorMessage: "Timeout! change me",
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	e := echo.New()
	c := e.NewContext(req, rec)

	err := m(func(c echo.Context) error {

		if err := sleepWithContext(c.Request().Context(), timeout); err != nil {
			return err
		}
		return c.String(http.StatusOK, "Hello, World!")
	})(c)

	assert.NoError(t, err)

	if rec.Code == http.StatusServiceUnavailable {

Review comment (bug):
I do not understand what does this test tests?   

If I am not incorrect when Contexts timeout is almost the same or is the same as waiting time (what `sleepWithContext` does) the result depends how fast scheduler reacts (it is run in different coroutine).  So sometimes you see `context.DeadlineExceeded` and sometimes handler can finish before scheduler manages to switch to coroutine that sends cancel message to context.



Code after:
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	e := echo.New()
	c := e.NewContext(req, rec)

	err := m(func(c echo.Context) error {
		if err := sleepWithContext(c.Request().Context(), time.Duration(20*time.Millisecond)); err != nil {
			return err
		}
		return c.String(http.StatusOK, "Hello, World!")
	})(c)

	assert.IsType(t, &echo.HTTPError{}, err)
	assert.Error(t, err)
	assert.Equal(t, http.StatusServiceUnavailable, err.(*echo.HTTPError).Code)
	assert.Equal(t, "Service Unavailable", err.(*echo.HTTPError).Message)
}

func TestContextTimeoutCanHandleContextDeadlineOnNextHandler(t *testing.T) {
	t.Parallel()

	timeoutErrorHandler := func(err error, c echo.Context) error {
		if err != nil {
			if errors.Is(err, context.DeadlineExceeded) {
				return &echo.HTTPError{
					Code:    http.StatusServiceUnavailable,
					Message: "Timeout! change me",
				}
			}
			return err
		}
		return nil
	}

	timeout := 10 * time.Millisecond
	m := ContextTimeoutWithConfig(ContextTimeoutConfig{
		Timeout:      timeout,
		ErrorHandler: timeoutErrorHandler,
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	e := echo.New()
	c := e.NewContext(req, rec)

	err := m(func(c echo.Context) error {
		// NOTE: when difference between timeout duration and handler execution time is almost the same (in range of 100microseconds)
Repository: labstack/echo
File: middleware/context_timeout_test.go (Go)

Code before:
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	e := echo.New()
	c := e.NewContext(req, rec)

	err := m(func(c echo.Context) error {

		if err := sleepWithContext(c.Request().Context(), timeout); err != nil {
			return err
		}
		return c.String(http.StatusOK, "Hello, World!")
	})(c)

	assert.NoError(t, err)

	if rec.Code == http.StatusServiceUnavailable {
		assert.Equal(t, "Timeout! change me", rec.Body.String())
	} else {
		assert.Equal(t, "Hello, World!", rec.Body.String())
	}
}

func TestContextTimeoutWithErrorMessage(t *testing.T) {
	t.Parallel()

	timeoutErrorHandler := func(err error, c echo.Context) error {
		if err != nil {
			if errors.Is(err, context.DeadlineExceeded) {
				return &echo.HTTPError{
					Code:    http.StatusServiceUnavailable,
					Message: "Timeout! change me",
				}
			}
			return err
		}
		return nil
	}

	timeout := 1 * time.Millisecond
	m := ContextTimeoutWithConfig(ContextTimeoutConfig{
		Timeout:      timeout,
		ErrorHandler: timeoutErrorHandler,
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	e := echo.New()

Review comment (suggestion):
I think copying that other `Timeout()` middleware tests is unnecessary. This variant of timeout is miles simpler and does no include things run in different coroutine.  Most of the tests that other timeout middleware has are because things are run in different coroutine.

Code after:
			return err
		}
		return nil
	}

	timeout := 10 * time.Millisecond
	m := ContextTimeoutWithConfig(ContextTimeoutConfig{
		Timeout:      timeout,
		ErrorHandler: timeoutErrorHandler,
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()

	e := echo.New()
	c := e.NewContext(req, rec)

	err := m(func(c echo.Context) error {
		// NOTE: when difference between timeout duration and handler execution time is almost the same (in range of 100microseconds)
		// the result of timeout does not seem to be reliable - could respond timeout, could respond handler output
		// difference over 500microseconds (0.5millisecond) response seems to be reliable

		if err := sleepWithContext(c.Request().Context(), time.Duration(20*time.Millisecond)); err != nil {
			return err
		}

		// The Request Context should have a Deadline set by http.ContextTimeoutHandler
		if _, ok := c.Request().Context().Deadline(); !ok {
			assert.Fail(t, "No timeout set on Request Context")
		}
		return c.String(http.StatusOK, "Hello, World!")
	})(c)

	assert.IsType(t, &echo.HTTPError{}, err)
	assert.Error(t, err)
	assert.Equal(t, http.StatusServiceUnavailable, err.(*echo.HTTPError).Code)
	assert.Equal(t, "Timeout! change me", err.(*echo.HTTPError).Message)
}

func sleepWithContext(ctx context.Context, d time.Duration) error {
	timer := time.NewTimer(d)

	defer func() {
		_ = timer.Stop()
	}()

	select {
	case <-ctx.Done():
		return context.DeadlineExceeded
	case <-timer.C:
		return nil
Repository: labstack/echo
File: middleware/static_windows.go (Go)

Code before:
package middleware

import (
	"os"
)

// We ignore these errors as there could be handler that matches request path.
//
// As of 1.20 on Windows filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean
// which is more suitable for path coming from web but this has some caveats on Windows. When we eventually end up in
// os related filesystem Open methods we are getting different errors as earlier versions. As of 1.20 path checks are
// more strict on path you provide and consider path with [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC)
// but missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`.
//
// So for 1.20@Windows we need to consider it as same not exist so we can continue next middleware/handler and not error
// which would result status 500 instead of potential route hit or 404.
func isIgnorableOpenFileError(err error) bool {
	if os.IsNotExist(err) {
		return true
	}
	errTxt := err.Error()
	return errTxt == "http: invalid or unsafe file path" || errTxt == "invalid path"
}


Review comment (suggestion):
Suggestion:

// As of Go 1.20 filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean 
// on Windows which has some caveats. The Open methods might return different errors than earlier versions and 
// as of 1.20 path checks are more strict on the provided path and considers [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC)
// paths with missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`.
//
// For 1.20@Windows we need to treat those errors the same as `fs.ErrNotExists` so we can continue handling
// errors in the middleware/handler chain. Otherwise we might end up with status 500 instead of finding a route 
// or return 404 not found.

Code after:
package middleware

import (
	"os"
)

// We ignore these errors as there could be handler that matches request path.
//
// As of Go 1.20 filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean
// on Windows which has some caveats. The Open methods might return different errors than earlier versions and
// as of 1.20 path checks are more strict on the provided path and considers [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC)
// paths with missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`.
//
// For 1.20@Windows we need to treat those errors the same as `fs.ErrNotExists` so we can continue handling
// errors in the middleware/handler chain. Otherwise we might end up with status 500 instead of finding a route
// or return 404 not found.
func isIgnorableOpenFileError(err error) bool {
	if os.IsNotExist(err) {
		return true
	}
	errTxt := err.Error()
	return errTxt == "http: invalid or unsafe file path" || errTxt == "invalid path"
}
Repository: labstack/echo
File: middleware/recover.go (Go)

Code before:
					}

					if config.LogErrorFunc != nil {
						err = config.LogErrorFunc(c, err, stack)
					} else if !config.DisablePrintStack {
						msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack[:length])
						switch config.LogLevel {
						case log.DEBUG:
							c.Logger().Debug(msg)
						case log.INFO:
							c.Logger().Info(msg)
						case log.WARN:
							c.Logger().Warn(msg)
						case log.ERROR:
							c.Logger().Error(msg)
						case log.OFF:
							// None.
						default:
							c.Logger().Print(msg)
						}
					}

					if(!config.DisableErrorHandler) {
						c.Error(err)
					}
					returnErr = err
				}
			}()
			return next(c)
		}
	}
}


Review comment (suggestion):
I think calling error( and setting that return error should be mutally exclusive - this is how it works today (error is "swallowed"

```go
					if(!config.DisableErrorHandler) {
						c.Error(err)
					} else {
					returnErr = err
}
```

and that first test can be changed to check that no error is returned
```go
func TestRecover(t *testing.T) {
	e := echo.New()
	buf := new(bytes.Buffer)
	e.Logger.SetOutput(buf)
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)
	h := Recover()(func(c echo.Context) error {
		panic("test")
	})
	err := h(c)
	assert.NoError(t, err)
	assert.Equal(t, http.StatusInternalServerError, rec.Code)
	assert.Contains(t, buf.String(), "PANIC RECOVER")
}
```

Code after:
						stack = stack[:length]
					}

					if config.LogErrorFunc != nil {
						err = config.LogErrorFunc(c, err, stack)
					} else if !config.DisablePrintStack {
						msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack[:length])
						switch config.LogLevel {
						case log.DEBUG:
							c.Logger().Debug(msg)
						case log.INFO:
							c.Logger().Info(msg)
						case log.WARN:
							c.Logger().Warn(msg)
						case log.ERROR:
							c.Logger().Error(msg)
						case log.OFF:
							// None.
						default:
							c.Logger().Print(msg)
						}
					}

					if(!config.DisableErrorHandler) {
						c.Error(err)
					} else {
						returnErr = err
					}
				}
			}()
			return next(c)
		}
	}
}
Repository: labstack/echo
File: middleware/recover_test.go (Go)

Code before:
		assert.Equal(t, http.StatusInternalServerError, rec.Code)

		output := buf.String()
		assert.Contains(t, output, "PANIC RECOVER")
		assert.Contains(t, output, `"level":"ERROR"`)
	})
}

func TestRecoverWithDisabled_ErrorHandler(t *testing.T) {
	e := echo.New()
	buf := new(bytes.Buffer)
	e.Logger.SetOutput(buf)
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)

	config := DefaultRecoverConfig
	config.DisableErrorHandler = true
	h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error {
		panic("test")
	}))
	err := h(c)

	assert.Equal(t, http.StatusOK, rec.Code)
	assert.Contains(t, buf.String(), "PANIC RECOVER")
	assert.NotNil(t, err)
}


Review comment (suggestion):
lets check error value also
```go
	assert.EqualError(t, err, "test")
```

Code after:
		h(c)
		assert.Equal(t, http.StatusInternalServerError, rec.Code)

		output := buf.String()
		assert.Contains(t, output, "PANIC RECOVER")
		assert.Contains(t, output, `"level":"ERROR"`)
	})
}

func TestRecoverWithDisabled_ErrorHandler(t *testing.T) {
	e := echo.New()
	buf := new(bytes.Buffer)
	e.Logger.SetOutput(buf)
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)

	config := DefaultRecoverConfig
	config.DisableErrorHandler = true
	h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error {
		panic("test")
	}))
	err := h(c)

	assert.Equal(t, http.StatusOK, rec.Code)
	assert.Contains(t, buf.String(), "PANIC RECOVER")
	assert.EqualError(t, err, "test")
}
Repository: labstack/echo
File: middleware/proxy_test.go (Go)

Code before:

	targetUrl, server = newServer(http.StatusBadRequest)
	defer server.Close()
	goodTargetWith40X := &ProxyTarget{
		Name: "Good with 40X",
		URL:  targetUrl,
	}

	targetUrl, _ = url.Parse("http://127.0.0.1:27121")
	badTarget := &ProxyTarget{
		Name: "Bad",
		URL:  targetUrl,
	}

	var alwaysRetryFilter ProxyRetryFilter = func(c echo.Context) bool { return true }
	var neverRetryFilter ProxyRetryFilter = func(c echo.Context) bool { return false }

	testCases := []struct {
		name             string
		retryCount       int
		retryFilters     []ProxyRetryFilter
		targets          []*ProxyTarget
		expectedResponse int
	}{
		{
			"retry count 0 does not attempt retry on fail",
			0,
			nil,
			[]*ProxyTarget{
				badTarget,
				goodTarget,
			},
			http.StatusBadGateway,
		},
		{
			"retry count 1 does not attempt retry on success",
			1,
			nil,
			[]*ProxyTarget{
				goodTarget,
			},
			http.StatusOK,
		},
		{
			"retry count 1 does retry on handler return true",
			1,
			[]ProxyRetryFilter{
				alwaysRetryFilter,
			},
			[]*ProxyTarget{
				badTarget,

Review comment (suggestion):
Could you please use field names. This way these structs are more future proof - when new fields are added the code would still compile, without names we would get compile errors.

Code after:
	}

	targetURL, server = newServer(http.StatusBadRequest)
	defer server.Close()
	goodTargetWith40X := &ProxyTarget{
		Name: "Good with 40X",
		URL:  targetURL,
	}

	targetURL, _ = url.Parse("http://127.0.0.1:27121")
	badTarget := &ProxyTarget{
		Name: "Bad",
		URL:  targetURL,
	}

	alwaysRetryFilter := func(c echo.Context, e error) bool { return true }
	neverRetryFilter := func(c echo.Context, e error) bool { return false }

	testCases := []struct {
		name             string
		retryCount       int
		retryFilters     []func(c echo.Context, e error) bool
		targets          []*ProxyTarget
		expectedResponse int
	}{
		{
			name: "retry count 0 does not attempt retry on fail",
			targets: []*ProxyTarget{
				badTarget,
				goodTarget,
			},
			expectedResponse: http.StatusBadGateway,
		},
		{
			name:       "retry count 1 does not attempt retry on success",
			retryCount: 1,
			targets: []*ProxyTarget{
				goodTarget,
			},
			expectedResponse: http.StatusOK,
		},
		{
			name:       "retry count 1 does retry on handler return true",
			retryCount: 1,
			retryFilters: []func(c echo.Context, e error) bool{
				alwaysRetryFilter,
			},
			targets: []*ProxyTarget{
				badTarget,
				goodTarget,
			},