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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 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
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
46 changes: 37 additions & 9 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ import (
"encoding/xml"
"errors"
"fmt"
"io"
"net"
"net/http"
"net/http/httputil"
"os"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -901,13 +903,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 @@ -946,12 +968,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, os.ErrDeadlineExceeded
}
Comment on lines +971 to +979
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 12, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use zero value for Timeout to disable timeout for clarity

Using -1 to disable the timeout may be unintuitive for users. In Go, it's conventional to use a zero time.Duration to represent the absence of a timeout. Adjusting this will make the API more user-friendly and align with Go standards.

Update the comments and logic as follows:

-// Config is optional and defaults to a 1s error on timeout,
-// -1 timeout will disable it completely.
+// Config is optional and defaults to a 1-second timeout with error.
+// A Timeout of zero disables the timeout.

...

-if cfg.Timeout >= 0 {
+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, os.ErrDeadlineExceeded
        }
    }
} else {
    // Without timeout
    err = <-channel
}

This change enhances readability and aligns with common Go practices regarding timeouts.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

Choose a reason for hiding this comment

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

@grivera64 pls check this

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ReneWerner87 I believe that using 0 for no timeout would be fine here. I don't believe that users will intentionally want to instant-timeout a test as possible with the current configuration, and making this change will reduce the number of people that make the mistake. As 0 is the default, you could even not include the Timeout field to imply no timeout in the test.

I can make this change and add it to the PR.

}
} else {
// Without timeout
Expand All @@ -969,6 +994,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
39 changes: 34 additions & 5 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"net"
"net/http"
"net/http/httptest"
"os"
"reflect"
"regexp"
"runtime"
Expand Down Expand Up @@ -1124,7 +1125,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,
Copy link
Member

Choose a reason for hiding this comment

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

@grivera64 if failOnTimeout is false, do we really need a -1 for the timeout? couldn't we do this implicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ReneWerner87 FailOnTimeout does not directly relate to the Timeout field, but the naming may mislead users to believe so.

When true, FailOnTimeout tells the test to return an error after timing out. If FailOnTimeout is false, the test will return the most up-to-date response before timing out without returning an error. If there is no timeout (Timeout: -1), then the value of FailOnTimeout doesn't affect the test. For clarity, I kept FailOnTimeout as false since there is no timeout that can lead to a failing test in the first place.

Maybe a less misleading name for the field would be something like IgnoreResponseOnTimeout or something along those lines.

What are your thoughts about this?

})
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")

Expand All @@ -1133,7 +1137,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 +1439,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,8 +1470,27 @@ func Test_App_Test_timeout(t *testing.T) {
return nil
})

_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), 100*time.Millisecond)
require.Equal(t, errors.New("test: timeout error after 100ms"), err)
_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{
Timeout: 100 * time.Millisecond,
FailOnTimeout: true,
})
require.Equal(t, os.ErrDeadlineExceeded, 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) {
Expand Down
5 changes: 4 additions & 1 deletion ctx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3243,7 +3243,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
37 changes: 36 additions & 1 deletion docs/whats_new.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ We have made several changes to the Fiber app, including:

### Methods changes

- Test -> timeout changed to 1 second
- Test -> Replaced timeout with a config parameter
- Listen -> has a config parameter
- Listener -> has a config parameter

Expand Down Expand Up @@ -184,6 +184,41 @@ To enable the routing changes above we had to slightly adjust the signature of t
+ Add(methods []string, path string, handler Handler, middleware ...Handler) Router
```

### Test Config

The `app.Test()` method now allows users to customize their test configurations:

<details>
<summary>Example</summary>

```go
// Create a test app with a handler to test
app := fiber.New()
app.Get("/", func(c fiber.Ctx) {
return c.SendString("hello world")
})

// Define the HTTP request and custom TestConfig to test the handler
req := httptest.NewRequest(MethodGet, "/", nil)
testConfig := fiber.TestConfig{
Timeout: -1,
FailOnTimeout: false,
}

// Test the handler using the request and testConfig
resp, err := app.Test(req, testConfig)
```
Comment on lines +194 to +210
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve the example to showcase key features.

The current example could better demonstrate the practical benefits of TestConfig, particularly for streaming responses and timeout handling.

 // Create a test app with a handler to test
 app := fiber.New()
-app.Get("/", func(c fiber.Ctx) {
-  return c.SendString("hello world")
+app.Get("/stream", func(c fiber.Ctx) error {
+  c.Set("Content-Type", "text/event-stream")
+  c.Set("Transfer-Encoding", "chunked")
+  
+  // Simulate a streaming response
+  return c.SendStreamWriter(func(w *bufio.Writer) {
+    for i := 0; i < 5; i++ {
+      fmt.Fprintf(w, "data: Message %d\n\n", i)
+      w.Flush()
+      time.Sleep(100 * time.Millisecond)
+    }
+  })
 })

 // Define the HTTP request and custom TestConfig to test the handler
-req := httptest.NewRequest(MethodGet, "/", nil)
+req := httptest.NewRequest(MethodGet, "/stream", nil)
 testConfig := fiber.TestConfig{
-  Timeout:       -1,
-  FailOnTimeout: false,
+  // Allow 1 second for the stream to complete
+  Timeout: 1 * time.Second,
+  // Preserve the partial response even if timeout occurs
+  FailOnTimeout: false,
 }

 // Test the handler using the request and testConfig
 resp, err := app.Test(req, testConfig)
+if err != nil {
+  log.Fatal(err)
+}
+
+// Read and verify the streamed response
+body, _ := io.ReadAll(resp.Body)
+fmt.Println(string(body))
📝 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
```go
// Create a test app with a handler to test
app := fiber.New()
app.Get("/", func(c fiber.Ctx) {
return c.SendString("hello world")
})
// Define the HTTP request and custom TestConfig to test the handler
req := httptest.NewRequest(MethodGet, "/", nil)
testConfig := fiber.TestConfig{
Timeout: -1,
FailOnTimeout: false,
}
// Test the handler using the request and testConfig
resp, err := app.Test(req, testConfig)
```
```go
// Create a test app with a handler to test
app := fiber.New()
app.Get("/stream", func(c fiber.Ctx) error {
c.Set("Content-Type", "text/event-stream")
c.Set("Transfer-Encoding", "chunked")
// Simulate a streaming response
return c.SendStreamWriter(func(w *bufio.Writer) {
for i := 0; i < 5; i++ {
fmt.Fprintf(w, "data: Message %d\n\n", i)
w.Flush()
time.Sleep(100 * time.Millisecond)
}
})
})
// Define the HTTP request and custom TestConfig to test the handler
req := httptest.NewRequest(MethodGet, "/stream", nil)
testConfig := fiber.TestConfig{
// Allow 1 second for the stream to complete
Timeout: 1 * time.Second,
// Preserve the partial response even if timeout occurs
FailOnTimeout: false,
}
// Test the handler using the request and testConfig
resp, err := app.Test(req, testConfig)
if err != nil {
log.Fatal(err)
}
// Read and verify the streamed response
body, _ := io.ReadAll(resp.Body)
fmt.Println(string(body))
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does sound like a much better example to show off, but I think it would be best to add this example use case in this separate PR:

🔥 Feature (v3): Add buffered streaming support #3131

Before doing so, I think we would need to merge these changes first so we can add unit tests using the new fiber.TestConfig{} feature from this PR.

Please let me know your thoughts!

Copy link
Contributor

Choose a reason for hiding this comment

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

@grivera64, that sounds like a great plan. Adding the example in PR #3131 after merging this PR makes sense. Let me know if you need any assistance!

(^.^)


</details>

To provide configurable testing capabilities, we had to change
the signature of the `Test` method.

```diff
- Test(req *http.Request, timeout ...time.Duration) (*http.Response, error)
+ Test(req *http.Request, config ...fiber.TestConfig) (*http.Response, error)
```

---

## 🧠 Context
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
Loading
Loading