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

🔥 Feature: Add TestConfig to app.Test() for configurable testing #3161

Merged
merged 27 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
00405f7
🔥 Feature: Add thread-safe reading from a closed testConn
grivera64 Oct 1, 2024
71c153b
🔥 Feature: Add TestConfig to app.Test()
grivera64 Oct 2, 2024
073ec97
📚 Doc: Add more details about TestConfig in docs
grivera64 Oct 3, 2024
228392e
Merge branch 'gofiber:main' into feature/add-app-test-config
grivera64 Oct 4, 2024
6c006a0
🩹 Fix: Correct testConn tests
grivera64 Oct 8, 2024
b07e05d
🎨 Style: Respect linter in Add App Test Config
grivera64 Oct 8, 2024
3c4017e
Merge branch 'gofiber:main' into feature/add-app-test-config
grivera64 Oct 8, 2024
6112c04
Merge branch 'gofiber:main' into feature/add-app-test-config
grivera64 Oct 9, 2024
4a97d7b
🎨 Styles: Update app.go to respect linter
grivera64 Oct 9, 2024
3639a78
Merge branch 'main' into feature/add-app-test-config
grivera64 Oct 12, 2024
4a2a77d
Merge branch 'main' into feature/add-app-test-config
grivera64 Oct 16, 2024
3776184
♻️ Refactor: Rename TestConfig's ErrOnTimeout to FailOnTimeout
grivera64 Oct 23, 2024
78e32a7
🩹 Fix: Fix typo in TestConfig struct comment in app.go
grivera64 Oct 23, 2024
20bac97
Merge branch 'main' into feature/add-app-test-config
grivera64 Nov 12, 2024
ca3efd2
♻️ Refactor: Change app.Test() fail on timeouterror to os.ErrDeadline…
grivera64 Nov 12, 2024
1220df7
♻️ Refactor:Update middleware that use the same TestConfig to use a g…
grivera64 Nov 12, 2024
498df60
🩹 Fix: Update error from FailOnTimeout to os.ErrDeadlineExceeded in t…
grivera64 Nov 12, 2024
438b630
🩹 Fix: Remove errors import from middlware/proxy/proxy_test.go
grivera64 Nov 12, 2024
0326c07
📚 Doc: Add `app.Test()` config changes to docs/whats_new.md
grivera64 Nov 13, 2024
6b6674f
Merge branch 'main' into feature/add-app-test-config
grivera64 Nov 17, 2024
334dbe8
♻ Refactor: Change app.Test() and all uses to accept 0 as no timeout …
grivera64 Nov 17, 2024
b389aaa
📚 Doc: Add TestConfig option details to docs/whats_new.md
grivera64 Nov 19, 2024
98d2fcc
🎨 Styles: Update docs/whats_new.md to respect markdown-lint
grivera64 Nov 19, 2024
be650d6
Merge branch 'main' into feature/add-app-test-config
grivera64 Nov 19, 2024
250b836
🎨 Styles: Update docs/whats_new.md to use consistent style for TestCo…
grivera64 Nov 19, 2024
7bae815
Merge branch 'main' into feature/add-app-test-config
gaby Nov 20, 2024
b6daa78
Merge branch 'main' into feature/add-app-test-config
grivera64 Nov 22, 2024
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
45 changes: 36 additions & 9 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"encoding/xml"
"errors"
"fmt"
"io"
"net"
"net/http"
"net/http/httputil"
Expand Down Expand Up @@ -864,13 +865,33 @@ func (app *App) Hooks() *Hooks {
return app.hooks
}

// TestConfig is a struct holding Test settings
type TestConfig struct {
grivera64 marked this conversation as resolved.
Show resolved Hide resolved
// Timeout defines the maximum duration a
// test can run before timing out.
// Default: time.Second
Timeout time.Duration

// FailOnTimeout specifies whether the test
// should return a timeout error if the HTTP response
// exceeds the Timeout duration.
// Default: true
FailOnTimeout bool
}

// Test is used for internal debugging by passing a *http.Request.
// Timeout is optional and defaults to 1s, -1 will disable it completely.
func (app *App) Test(req *http.Request, timeout ...time.Duration) (*http.Response, error) {
// Set timeout
to := 1 * time.Second
if len(timeout) > 0 {
to = timeout[0]
// Config is optional and defaults to a 1s error on timeout,
// -1 timeout will disable it completely.
func (app *App) Test(req *http.Request, config ...TestConfig) (*http.Response, error) {
// Default config
cfg := TestConfig{
Timeout: time.Second,
FailOnTimeout: true,
}

// Override config if provided
if len(config) > 0 {
cfg = config[0]
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}

// Add Content-Length if not provided with body
Expand Down Expand Up @@ -909,12 +930,15 @@ func (app *App) Test(req *http.Request, timeout ...time.Duration) (*http.Respons
}()

// Wait for callback
if to >= 0 {
if cfg.Timeout >= 0 {
// With timeout
select {
case err = <-channel:
case <-time.After(to):
return nil, fmt.Errorf("test: timeout error after %s", to)
case <-time.After(cfg.Timeout):
conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
if cfg.FailOnTimeout {
return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we could replace with os.ErrDeadlineExceeded() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaby I believe we can, ErrDeadlineExceeded seems to be used by the net package according to the os package comment. To follow common convention, we can return this error.

Should we also wrap this error with any extra details about the timeout? Like for example:

fmt.Errorf("%w: failed after %s", os.ErrDeadlineExceeded, cfg.Timeout)
// Returns wrapped error: "i/o timeout: failed after 1s"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaby I've change it to use os.ErrDeadlineExceeded as the error type. Thanks for the recommendation!

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance timeout error message and use os.ErrDeadlineExceeded.

The timeout error handling could be improved by:

  1. Using os.ErrDeadlineExceeded as the base error
  2. Providing more context in the error message

Apply this diff to enhance the error handling:

- return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout)
+ return nil, fmt.Errorf("%w: request timed out waiting for response after %s", os.ErrDeadlineExceeded, cfg.Timeout)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if cfg.Timeout >= 0 {
// With timeout
select {
case err = <-channel:
case <-time.After(to):
return nil, fmt.Errorf("test: timeout error after %s", to)
case <-time.After(cfg.Timeout):
conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
if cfg.FailOnTimeout {
return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout)
}
if cfg.Timeout >= 0 {
// With timeout
select {
case err = <-channel:
case <-time.After(cfg.Timeout):
conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
if cfg.FailOnTimeout {
return nil, fmt.Errorf("%w: request timed out waiting for response after %s", os.ErrDeadlineExceeded, cfg.Timeout)
}

}
} else {
// Without timeout
Expand All @@ -932,6 +956,9 @@ func (app *App) Test(req *http.Request, timeout ...time.Duration) (*http.Respons
// Convert raw http response to *http.Response
res, err := http.ReadResponse(buffer, req)
if err != nil {
if errors.Is(err, io.ErrUnexpectedEOF) {
return nil, errors.New("test: got empty response")
}
return nil, fmt.Errorf("failed to read response: %w", err)
}

Expand Down
36 changes: 32 additions & 4 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,10 @@ func Test_Test_Timeout(t *testing.T) {

app.Get("/", testEmptyHandler)

resp, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), -1)
resp, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{
Timeout: -1,
FailOnTimeout: false,
grivera64 marked this conversation as resolved.
Show resolved Hide resolved
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")

Expand All @@ -1133,7 +1136,10 @@ func Test_Test_Timeout(t *testing.T) {
return nil
})

_, err = app.Test(httptest.NewRequest(MethodGet, "/timeout", nil), 20*time.Millisecond)
_, err = app.Test(httptest.NewRequest(MethodGet, "/timeout", nil), TestConfig{
Timeout: 20 * time.Millisecond,
FailOnTimeout: true,
})
require.Error(t, err, "app.Test(req)")
}

Expand Down Expand Up @@ -1432,7 +1438,10 @@ func Test_App_Test_no_timeout_infinitely(t *testing.T) {
})

req := httptest.NewRequest(MethodGet, "/", nil)
_, err = app.Test(req, -1)
_, err = app.Test(req, TestConfig{
Timeout: -1,
FailOnTimeout: true,
})
}()

tk := time.NewTimer(5 * time.Second)
Expand Down Expand Up @@ -1460,10 +1469,29 @@ func Test_App_Test_timeout(t *testing.T) {
return nil
})

_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), 100*time.Millisecond)
_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{
Timeout: 100 * time.Millisecond,
FailOnTimeout: true,
})
require.Equal(t, errors.New("test: timeout error after 100ms"), err)
}

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

app := New()
app.Get("/", func(_ Ctx) error {
time.Sleep(1 * time.Second)
return nil
})

_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{
Timeout: 100 * time.Millisecond,
FailOnTimeout: false,
})
require.Equal(t, errors.New("test: got empty response"), err)
}

func Test_App_SetTLSHandler(t *testing.T) {
t.Parallel()
tlsHandler := &TLSHandler{clientHelloInfo: &tls.ClientHelloInfo{
Expand Down
5 changes: 4 additions & 1 deletion ctx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3142,7 +3142,10 @@ func Test_Static_Compress(t *testing.T) {

req := httptest.NewRequest(MethodGet, "/file", nil)
req.Header.Set("Accept-Encoding", algo)
resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})

require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
Expand Down
30 changes: 28 additions & 2 deletions docs/api/app.md
Original file line number Diff line number Diff line change
Expand Up @@ -537,10 +537,10 @@ func (app *App) SetTLSHandler(tlsHandler *TLSHandler)

## Test

Testing your application is done with the **Test** method. Use this method for creating `_test.go` files or when you need to debug your routing logic. The default timeout is `1s` if you want to disable a timeout altogether, pass `-1` as a second argument.
Testing your application is done with the **Test** method. Use this method for creating `_test.go` files or when you need to debug your routing logic. The default timeout is `1s`. If you want to disable a timeout altogether, pass a `TestConfig` struct with `Timeout: -1`.

```go title="Signature"
func (app *App) Test(req *http.Request, msTimeout ...int) (*http.Response, error)
func (app *App) Test(req *http.Request, config ...TestConfig) (*http.Response, error)
```

```go title="Examples"
Expand All @@ -566,6 +566,32 @@ if resp.StatusCode == fiber.StatusOK {
}
```

If not provided, TestConfig is set to the following defaults:

```go title="Default TestConfig"
config := fiber.TestConfig{
Timeout: time.Second(),
FailOnTimeout: true,
}
```

:::caution

This is **not** the same as supplying an empty `TestConfig{}` to
`app.Test(), but rather be the equivalent of supplying:

```go title="Empty TestConfig"
cfg := fiber.TestConfig{
Timeout: 0,
FailOnTimeout: false,
}
```

This would make a Test that instantly times out,
which would always result in a "test: empty response" error.

:::

## Hooks

Hooks is a method to return [hooks](./hooks.md) property.
Expand Down
33 changes: 28 additions & 5 deletions helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -613,13 +613,36 @@ func isNoCache(cacheControl string) bool {
}

type testConn struct {
r bytes.Buffer
w bytes.Buffer
r bytes.Buffer
w bytes.Buffer
isClosed bool
sync.Mutex
}

func (c *testConn) Read(b []byte) (int, error) { return c.r.Read(b) } //nolint:wrapcheck // This must not be wrapped
func (c *testConn) Write(b []byte) (int, error) { return c.w.Write(b) } //nolint:wrapcheck // This must not be wrapped
func (*testConn) Close() error { return nil }
func (c *testConn) Read(b []byte) (int, error) {
c.Lock()
defer c.Unlock()

return c.r.Read(b) //nolint:wrapcheck // This must not be wrapped
}

func (c *testConn) Write(b []byte) (int, error) {
c.Lock()
defer c.Unlock()

if c.isClosed {
return 0, errors.New("testConn is closed")
}
return c.w.Write(b) //nolint:wrapcheck // This must not be wrapped
}

func (c *testConn) Close() error {
c.Lock()
defer c.Unlock()

c.isClosed = true
return nil
}

func (*testConn) LocalAddr() net.Addr { return &net.TCPAddr{Port: 0, Zone: "", IP: net.IPv4zero} }
func (*testConn) RemoteAddr() net.Addr { return &net.TCPAddr{Port: 0, Zone: "", IP: net.IPv4zero} }
Expand Down
42 changes: 42 additions & 0 deletions helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,48 @@ func Test_Utils_TestConn_Deadline(t *testing.T) {
require.NoError(t, conn.SetWriteDeadline(time.Time{}))
}

func Test_Utils_TestConn_ReadWrite(t *testing.T) {
t.Parallel()
conn := &testConn{}

// Verify read of request
_, err := conn.r.Write([]byte("Request"))
require.NoError(t, err)

req := make([]byte, 7)
_, err = conn.Read(req)
require.NoError(t, err)
require.Equal(t, []byte("Request"), req)

// Verify write of response
_, err = conn.Write([]byte("Response"))
require.NoError(t, err)

res := make([]byte, 8)
_, err = conn.w.Read(res)
require.NoError(t, err)
require.Equal(t, []byte("Response"), res)
}

func Test_Utils_TestConn_Closed_Write(t *testing.T) {
t.Parallel()
conn := &testConn{}

// Verify write of response
_, err := conn.Write([]byte("Response 1\n"))
require.NoError(t, err)

// Close early, write should fail
conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
_, err = conn.Write([]byte("Response 2\n"))
require.Error(t, err)

res := make([]byte, 11)
_, err = conn.w.Read(res)
require.NoError(t, err)
require.Equal(t, []byte("Response 1\n"), res)
}

func Test_Utils_IsNoCache(t *testing.T) {
t.Parallel()
testCases := []struct {
Expand Down
30 changes: 24 additions & 6 deletions middleware/compress/compress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ func Test_Compress_Gzip(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
req.Header.Set("Accept-Encoding", "gzip")

resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, "gzip", resp.Header.Get(fiber.HeaderContentEncoding))
Expand Down Expand Up @@ -72,7 +75,10 @@ func Test_Compress_Different_Level(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
req.Header.Set("Accept-Encoding", algo)

resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, algo, resp.Header.Get(fiber.HeaderContentEncoding))
Expand All @@ -99,7 +105,10 @@ func Test_Compress_Deflate(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
req.Header.Set("Accept-Encoding", "deflate")

resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, "deflate", resp.Header.Get(fiber.HeaderContentEncoding))
Expand All @@ -123,7 +132,10 @@ func Test_Compress_Brotli(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
req.Header.Set("Accept-Encoding", "br")

resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, "br", resp.Header.Get(fiber.HeaderContentEncoding))
Expand All @@ -147,7 +159,10 @@ func Test_Compress_Zstd(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
req.Header.Set("Accept-Encoding", "zstd")

resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, "zstd", resp.Header.Get(fiber.HeaderContentEncoding))
Expand All @@ -171,7 +186,10 @@ func Test_Compress_Disabled(t *testing.T) {
req := httptest.NewRequest(fiber.MethodGet, "/", nil)
req.Header.Set("Accept-Encoding", "br")

resp, err := app.Test(req, 10*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, "", resp.Header.Get(fiber.HeaderContentEncoding))
Expand Down
5 changes: 4 additions & 1 deletion middleware/idempotency/idempotency_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ func Test_Idempotency(t *testing.T) {
if idempotencyKey != "" {
req.Header.Set("X-Idempotency-Key", idempotencyKey)
}
resp, err := app.Test(req, 15*time.Second)
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 15 * time.Second,
FailOnTimeout: true,
})
require.NoError(t, err)
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
Expand Down
Loading
Loading